Skip to content
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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

noahmayr
Copy link
Collaborator

@noahmayr noahmayr commented May 2, 2024

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:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@noahmayr noahmayr force-pushed the noahmayr/push-tpyswxozlxlo branch from 8037c1d to 8e3dbbf Compare May 2, 2024 17:00
@martinvonz
Copy link
Owner

there's probably some unused code in there from various attempts at keeping commit and view metadata in sync.

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.

Copy link
Collaborator

@PhilipMetzger PhilipMetzger left a 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.

lib/src/protos/git_store.proto Outdated Show resolved Hide resolved
@necauqua
Copy link
Collaborator

necauqua commented May 3, 2024

I think Mercurial restricts it to at most one topic per commit

I don't think completely copying mercurial was a goal in the first place.
The main discussion was "how much if at all do we want to restrict the idea of a topic being a non-git-but-string-tag on a commit" I think - having only one topic on a commit barely came up if at all.

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),
Copy link
Collaborator

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.

@noahmayr noahmayr force-pushed the noahmayr/push-tpyswxozlxlo branch 2 times, most recently from 93392c3 to 0785851 Compare May 4, 2024 16:05
@noahmayr
Copy link
Collaborator Author

noahmayr commented May 4, 2024

Why do we need to store the information in both places?

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.

Mercurial ignores topics once a commit has become public, which is the equivalent of immutable for us.

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.

Did it seem like the conclusion on #3402 was that there are use cases for multiple topics?

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. jj split).

Restoring the previous topics also only seems to work for jj op restore and not jj undo which I still need to look into.
Also not sure yet whether things like jj new --insert-before would need special handling to inherit the topics of the descendant instead.

@martinvonz
Copy link
Owner

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.

Why would you want to remove a topic from an immutable commit?

@joyously
Copy link

joyously commented May 4, 2024

Why would you want to remove a topic from an immutable commit?

If it's called work in progress?

@noahmayr noahmayr force-pushed the noahmayr/push-tpyswxozlxlo branch from 0785851 to 6de408e Compare May 4, 2024 19:05
@noahmayr
Copy link
Collaborator Author

noahmayr commented May 4, 2024

Why would you want to remove a topic from an immutable commit?

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 imutable_heads() changes which seems like unexpected behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants