-
Notifications
You must be signed in to change notification settings - Fork 319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
prototype implementation for topics #3402 #3613
base: main
Are you sure you want to change the base?
Conversation
8037c1d
to
8e3dbbf
Compare
Why do we need to store the information in both places? I would have thought that it would be enough to have it just on the commit. Mercurial ignores topics once a commit has become public, which is the equivalent of immutable for us. I'm guessing (I haven't checked the code) that you're storing topics in the view for quick access to all (?) topics. Maybe we can look up topics only among the mutable commits and get it fast enough? Also, I noticed in the code that there can be multiple topics per commit. I think Mercurial restricts it to at most one topic per commit. Did it seem like the conclusion on #3402 was that there are use cases for multiple topics? I didn't read all of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's off to a good start and all toplevel stuff SGTM. But if we want to have general metadata, we need change the approach.
I don't think completely copying mercurial was a goal in the first place. Other restrictions, like do we want to allow them being disjointed, or for a set of commits under a topic having >1 head, did come up, but imo we could always restrict them later (and I'm also against restrictions anyway). |
@@ -318,6 +318,7 @@ pub enum RevsetCommitRef { | |||
remote_pattern: StringPattern, | |||
}, | |||
Tags, | |||
Topics(StringPattern), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, Topics
will have to be a RevsetFilterPredicate
, not a resolvable commit ref. Suppose topics are relevant only in mutable commits, queries like heads(topics(..))
wouldn't need a persistent index.
93392c3
to
0785851
Compare
After some discussion with @PhilipMetzger we came to the conclusion that we can't actually store topics on commits, since a change to a commits topics would equal rebasing the commit meaning you couldn't even remove topics from commits when they become immutable without mutating them.
I'm not sure if that limitation makes sense here, I think the consensus on #3402 was that topics could be how jj represents branches which the git backend would then transparently map to branches so they can be made into PRs. Since we're not limiting branches from being put on immutable commits, I would not put that constraint on topics either.
Yes, noone has advocated for topics necessarily being exclusive while there have been votes for multiple topics. I updated the implementation to only store topics in the view. New commits get their parents topics, rewritten commits move their topics to their successor(s) (can be more than one for e.g. Restoring the previous topics also only seems to work for |
Why would you want to remove a topic from an immutable commit? |
If it's called |
0785851
to
6de408e
Compare
Well in general changing topics on immutable commits becomes impossible, I'm not sure if we want that limitation. If we work around that by simply ignoring/hiding topics on immutable commits, they would still resurface when the definition of |
This is still a very rough draft, but I think it's at least at a point where it can be played around with to move the discussion further along.
Any feedback is welcome, there's probably some unused code in there from various attempts at keeping commit and view metadata in sync.
First basic implementation of #3402 adding basic crud, revset and commit template integration. There's still a lot to be done though.
Checklist
If applicable:
CHANGELOG.md