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

FR: better design for ignoring branches #1734

Closed
6 tasks
ilyagr opened this issue Jun 24, 2023 · 21 comments
Closed
6 tasks

FR: better design for ignoring branches #1734

ilyagr opened this issue Jun 24, 2023 · 21 comments
Labels
enhancement New feature or request

Comments

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 24, 2023

As discussed in #1714, jj currently does not have good support for a situation where the user wants to ignore a large number of branches on the remote. Here is a draft design outline for a possible fix.

This design assumes that we stay with hg-like concept of branches: a branch is assumed to correspond to a branch of the same name on any remote.

Phase 1

  • We rename jj branch forget to jj branch ignore and have it persistently forget branches. Ignored branches are neither imported by jj git import nor fetched by jj git fetch. From the user perspective, both the branch and its remote-tracking branches disappear (but see Phase 2).

  • We have jj branch ignore --list to list ignored branches.

  • We introduce a jj branch unignore command to remove branches from the list of forgotten branches.

  • We should make sure that jj branch create/set and other potential ways of creating branches automatically un-ignore the branch. This could be done on jj git export/jj git push.

Technically, the BranchTarget struct will gain a new enum field that says whether this branch is ignored.

In colocated repos, jj git export will remove the ignored branch from the git repo. We can have jj git push remove the ignored branch from the git repo's list of remote-tracking branches.

Phase 2

The usecase here is less clear to me, so this outline is a bit vague.

  • Add a --local option to jj git ignore that causes only the local branch to disappear. jj git fetch still imports the remote-tracking branches, but does not recreate the local branch. The local branch can be recreated with jj branch set or by jj git unignore; jj git fetch.

  • In a colocated repo, a "locally unignored" branch should have the git repo's local branches removed but the remote-tracking branches kept.

@yuja
Copy link
Collaborator

yuja commented Jun 24, 2023

Ignored branches are neither imported by jj git import nor fetched by jj git fetch.

Does it mean "ignored branches" are effectively negative refspec set by default?
For that purpose, we can probably add config option.

In my usecase, I want to see other people's branches, but I'm not willing to maintain such branches (or branch conflicts especially) forever. And I want to freely get rid of them.

$ jj branch forget their_branch  # to not move the local branch
$ jj abandon ...

That's why I said I would want jj branch ignore --local behavior described in "Phase 2". As I said before, my pain point will also be addressed if jj abandon leaves their_branch at the original position, or demote it to ignore --local state.

I think git-like branch model will work better for my usecase (because there will be no local branch upfront), but I'm not strongly against hg-like model.

Add a --local option to jj git ignore that causes only the local branch to disappear. jj git fetch still imports the remote-tracking branches, but does not recreate the local branch.

Somewhat related: with git.auto-local-branch = false, fetched branches will get to this state.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 24, 2023

Does it mean "ignored branches" are effectively negative refspec set by default? For that purpose, we can probably add config option.

I see essentially two options here: either jj branch ignore 'feature_*' ignores all the currently existing branches that match the glob or it records the glob and ignores any future branches matching the glob as well. You suggest having it be a config option, which is similar to the latter option (and simpler) but IMO less convenient.

I suggested the first option here because it seemed simpler than the second (fewer places to change in the implementation) and convenient enough for the usecases that come to my mind (see #1714 (comment)). If we go with the second option, it seems like it would be difficult for us to help the user to tell whether any new branches were silently ignored since a bit of time ago.

That's why I said I would want jj branch ignore --local behavior described in "Phase 2"

Fair enough. I find it strange that the usecases that seem so natural to me seem awkward to you, and vice-versa. This makes it hard for me to judge which of your suggestions or their variations seems best to me.

One idea that makes me more comfortable with this is that we could have jj branch list not list these ignore --local branches by default.

Elsewhere, you suggested making a special command to get rid of remote branches. That could go with making jj branch ignore --local the default.

The alternative of abandon not moving branches, or a flag to abandon that makes it ignore branches, also seems curious. However, it seems to me to be a bit hard for a user to keep track of since the concepts of ignored branches start to heavily interact with the concept of hidden commits.

I'll try to pay more attention to where that might be useful in my workflow. I also wonder what other people think.

I think I can live with many of these options. For now, my preference would still be to have jj branch ignore that gets rid of everything, jj branch ignore --local, and maybe jj branch ignore --remotes and/or jj branch ignore --remote specific_remote, mainly since this seems to me to be a model that's relatively straightforward for somebody to understand.

Update: If we change the default of jj branch list, changing the default of jj branch ignore to have the --local behavior doesn't seem problematic to me. We could then have jj branch ignore --everything. In fact, my main difficulty with this is that I don't like the --everything name (since that won't touch the git ref in colocated repos).

Somewhat related: with git.auto-local-branch = false, fetched branches will get to this state.

I want to get rid of that config. My feeling is that it's an inferior workaround, and only exists because people don't have a good way to get rid of branches they don't want. Of course, I could be missing somebody's usecase.

I think git-like branch model will work better for my usecase (because there will be no local branch upfront),

Acknowledged.

Very subjectively implementing that feels unwieldy to me. I don't want to rethink the entire fetch/push/import/export logic. At least at this moment, I find my mind resistant to even think about it seriously, and I don't feel motivated enough to challenge this resistance.

I find the hg branch model pleasant for my usecases, even though it's obviously less flexible.

This doesn't necessarily mean it's not a good idea to switch, and it could be easier than I fear once somebody starts the work. We should keep the idea in mind, and especially the usecases that would be easier that way.

@yuja
Copy link
Collaborator

yuja commented Jun 25, 2023

Does it mean "ignored branches" are effectively negative refspec set by default? For that purpose, we can probably add config option.

I see essentially two options here: either jj branch ignore 'feature_*' ignores all the currently existing branches that match the glob or it records the glob and ignores any future branches matching the glob as well. You suggest having it be a config option, which is similar to the latter option (and simpler) but IMO less convenient.

I suggested the first option here because it seemed simpler than the second (fewer places to change in the implementation) and convenient enough for the usecases that come to my mind (see #1714 (comment)). If we go with the second option, it seems like it would be difficult for us to help the user to tell whether any new branches were silently ignored since a bit of time ago.

AFAIK, the second option I proposed is a natural extension to jj git fetch --branch. It will gain the default configured by user, and some flag or syntax to represent a negative pattern.

It won't help my usecase, but I feel my requirement is conceptually different from yours.

I find it strange that the usecases that seem so natural to me seem awkward to you, and vice-versa.

Maybe because I rely on anonymous branches for my changes? When I get branch conflicts, it's 100% someone else's branch, and I just don't wanna be bothered by that. :)

Somewhat related: with git.auto-local-branch = false, fetched branches will get to this state.

I want to get rid of that config. My feeling is that it's an inferior workaround, and only exists because people don't have a good way to get rid of branches they don't want. Of course, I could be missing somebody's usecase.

According to #894, exporting tons of local branches is the problem. I think we'll need a way to ban local branch creation upfront (because export is automated in colocated repo.)


The alternative of abandon not moving branches, or a flag to abandon that makes it ignore branches, also seems curious. However, it seems to me to be a bit hard for a user to keep track of since the concepts of ignored branches start to heavily interact with the concept of hidden commits.

A bit off-topic: Is it convenient that jj abandon moves branch ref backwards? I pretty much want to get rid of the branch ref in that case.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 25, 2023

A bit off-topic: Is it convenient that jj abandon moves branch ref backwards? I pretty much want to get rid of the branch ref in that case.

Good question! I think there are two essentially different situations where I use jj abandon:

  • The usual case where I have some commits I want to get rid of entirely, usually when they are leaves in the commit tree. In this case, I 100% agree that it is not at all convenient for jj abandon to move the branch backwards.

  • As part of polishing or editing a chain of commits. Then, I would often do things like split a commit, and then abandon one of the half (perhaps after some testing of the first half).

There, it is essential that the children of the abandoned commit are rebased onto its parents. Usually this doesn't raise any questions about branches, but sometimes I the commit I want to abandon when thinking of commits this way may have a branch commit, or might even be a leaf.

In this case, it makes perfect sense for abandon to move the branches back.

Also, if the branch is not on a leaf commit, it's unclear what else to do with the branch but move it back.


While, the way I described it, the second case might seem rare, I think it's pretty important -- consistent tools for editing commit chains before sending a PR are important to jj. Clearly, having the same command for both cases is imperfect, but I don't have a proposal for how to improve the situation right now. But I'm happy to now be consciously aware of the problem.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 25, 2023

Re your other points, I don't have many clear thoughts about most.

Maybe because I rely on anonymous branches for my changes? When I get branch conflicts, it's 100% someone else's branch, and I just don't wanna be bothered by that. :)

This is likely it. I never use anonymous branches, not after I realized that I'd occasionally have to copy-paste their full names. I think I could benefit from them, maybe, but I find jj's support for them not nearly good enough to be practical for me and I'm a bit confused how anybody manages to use them productively in the current state (even though you and Martin obviously do).

@martinvonz
Copy link
Owner

I never use anonymous branches, not after I realized that I'd occasionally have to copy-paste their full names.

Perhaps this is off topic, but when do you need to use the full name? What do you even need by "their full name"? The change id?

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 25, 2023

when do you need to use the full name?

To move them to other commits (which also breaks the change of correspondence) if e.g. the PR turns out to be two commits instead of one (in which case I could use split, but it doesn't always feel natural) or if I need to postpone a commit to a later PR. Also, to, delete them or forget them. I also used to need to do that to push them individually, but that's fixed now.

What do you even need by "their full name"

push-xyzstuv...

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 26, 2023

@yuja, I'm worried that thinking out the best way for things to work will take some time and effort. Would you be OK with implementing, for now, a minimal fix to the bug from #1714 that would not make your workflow worse?

I feel bad about that bug, and it bothers me a bit more since I added a hint to use jj branch forget in d01ecc5.

I'm thinking of making a fix that would be invisible to most users. It's essentially "phase 1", but I'm currently thinking that:

  • I'd keep the forget name. jj branch forget will mark branches as ignored, in addition to everything else it does. This is mainly to not have people learn a new command that we might later decide needs to be reworked or removed.
  • I'd add an jj branch unignore command.
  • I'll make jj git fetch print a hint about the unignore command if it refused to fetch some ignored branches.
  • I'd probably also add jj branch list --ignored.
  • I guess I would have to make sure that jj branch set & related commands un-ignore branches.

It seems to me that this much should be implementable relatively quickly (thought maybe I'm missing something? One thing that could sabotage this plan is if I underestimate the difficulty of the "quick fix"), before we resolve all the surrounding gnarly issues (import/export of remote-tracking branches, best UI, etc).


One potential problem I see is that it's not clear whether import/export should ignore ignored local branches (they should definitely ignore the remote-tracking branches). A user in a colocated repo that creates a branch (e.g. jj forget br; <many commands>; git switch -C br) might be confused if it's not imported.

@yuja
Copy link
Collaborator

yuja commented Jun 27, 2023

I'm worried that thinking out the best way for things to work will take some time and effort. Would you be OK with implementing, for now, a minimal fix to the bug from #1714 that would not make your workflow worse?

I'm okay with breaking my workflow (temporarily), and I'd prefer to keep a hotfix minimal than adding new "ignore" concept, which we aren't sure the model will work out.
So I'd suggest just fixing branch forget to respect the documented behavior. (I agree that anything we can do with undo should be doable differently, so we'll need something like branch forget anyway.)

Implementation-wise, my feeling for #1714 is that we're patching wrong place git::import_refs() for something else's (export?, fetch?, or "forget" model itself?) problem. It might be better to discard forgotten remote refs at export_refs() or just before fetch()ing.

@yuja
Copy link
Collaborator

yuja commented Jun 27, 2023

Related #1136

I find it strange that the usecases that seem so natural to me seem awkward to you, and vice-versa.

I tend to do fetch everything, and discard outdated/uninteresting branches when things gets messy. I think that's another difference from your workflow.

I'll play with git.auto-local-branch = false variant (auto-local-branch only for existing locals) to see if it's close to the model I want.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 28, 2023

It might be better to discard forgotten remote refs at export_refs()

I'm looking at this; it seems feasible and to do approximately what we want. For some reason, my draft version also breaks a test assertion at

assert!(git_repo.head_detached().unwrap());

@yuja
Copy link
Collaborator

yuja commented Jun 28, 2023

It might be better to discard forgotten remote refs at export_refs()

I'm looking at this; it seems feasible and to do approximately what we want. For some reason, my draft version also breaks a test assertion at

Appears that branches_to_update isn't fully migrated to "refs/heads/{branch}" string.
It might be easier to handle local/remote branches separately (because the source tables are separated), but I'm not sure.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 28, 2023

Thanks, good catch!

I'm not sure I follow your idea for handling them separately. I tried at first (not very hard, perhaps), but a lot of the code got repeated. Also, perhaps at some point jj will manipulate git-tracking branches in a way that it needs to export updates, not just deletions.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 30, 2023

t might be better to discard forgotten remote refs at export_refs()

There's still an annoying problem: in a normal repo, jj git export is not run automatically, so you still end up in the confusing state. See also the commit with "BUG DEMO" in the name in #1771. (That commit is gone now, see the "force jj git export..." commit of #1771 instead).

The simplest solution would be to run jj git export automatically, but it might be surprising to the user if their git repo's remote-tracking branch disappears. I guess this is rare enough that I might just put something like the following in jj branch forget's docs:

NOTE: jj branch forget runs jj git export automatically, even in non-colocated jj repos. If you are using a non-colocated repo and are counting on this command not touching the git repo's branch refs, please report your use case on our bug tracker.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 30, 2023

OK, I now implemented that in #1771. I think this problem was another reason I preferred the #1714 approach before, but now I see advantages and disadvantages to both. I think I'm OK with either at the moment.

@yuja
Copy link
Collaborator

yuja commented Jul 9, 2023

Note: This is orthogonal to the original "ignoring branches" idea. It's more like a git-like UX adaptation #1136 to JJ's branch model.


I'll play with git.auto-local-branch = false variant (auto-local-branch only for existing locals)

Tried this setup for a week, and I feel comfortable with that. There are only a few synced local branches ("main" and my ongoing PRs), and the others are remote only.
Pros:

  • I can freely abandon remote-only (or readonly) branches.
  • It's nice that the colocated git repo isn't polluted by local branches imported&exported by jj.

The downside is the same as #1136.

  • Remote-only branches are rendered as (deleted) in jj branch list.
  • Pushing such branch will delete the remote branch.
  • Turning git.auto-local-branch off and on will likely result in change-delete conflicts because the local branch is at "deleted" state.

To mitigate the last bullet point, we'll probably need a flag stored in view, not in config file.

Implementation idea

Add per-remote-branch "tracking" flag, something like:

struct BranchTarget {
    local_target: Option<RefTarget>,
    remotes: BTreeMap<String, {target: RefTarget, tracking: bool}>,
}

We could add a per-branch flag, but it doesn't make sense that a branch with no remotes can be set to "tracking". I also think per-remote-branch flag is technically correct.

A None branch with one or more tracking remotes is considered "deleted". Otherwise it is "absent".

  • "deleted": local_target == None && any(x.tracking for x in remotes)
  • "absent": local_target == None && !any(x.tracking for x in remotes)
  • "forgotten": local_target == None && remotes.is_empty() && still_in_git (pseudo state)

An "absent" branch cannot be pushed. Pushing "deleted" branch to non-tracking remote will probably delete the remote branch (just like pushing new branch to remote), but TBD. EDIT: Perhaps, it's better to reject any push if non-tracking remote branch already exists. (Pushing new branch is still allowed.)

When tracking flag will be set:

  • If git.auto-local-branch == true, fetched remote branches will be set to tracking = true.
  • Pushing local branch will set the remote counterpart to be tracking = true (no matter if git.auto-local-branch is true or false.)
  • There will be jj branch subcommands to track/untrack remote branch.
    (Newly tracked remote branch will be merged in with left = local, base = None, right = remote.)

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jul 10, 2023

Interesting! I haven't fully thought this through, but here are a few thoughts I had so far.

  • I'm not sure this is necessarily better, but we could store some of this config in .jj/config.toml instead of the view object. That would be similar to how git uses .git/config.

  • I ran into a situation where my main branch had a * next to it because of a remote I don't care about. This made me think:

    • Perhaps we could make it easy to untrack a branch on all remotes or to untrack a remote for all branches. I think of the auto-local-branch config as a hack, it is currently far too inflexible.
    • In addition untracking having an effect on fetch/push and jj branch list, we might want this untracking to affect the way branches are presented in jj log. I'm not sure whether there are more places it should affect.

I think all of these points are about the UI, and it seems likely that the design can support them. I like that in your design, the value of the auto-local-branch config affects very little.

  • If we store the tracking state in the View, we have to specify what happens if we merge a view with tracking set to false and a view with tracking set to true. I'm not sure there's a right answer here, but either answer shouldn't be terrible.

"absent": local_target == None && !any(x.tracking for x in remotes)

Why should an entire branch be marked as absent? I think we can allow a missing branch to be deleted on one remote and absent with respect to another remote.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jul 10, 2023

Also, your design is the opposite of the the one I was thinking of in the following way. I was thinking that a branch/remote pair should have one of the following states:

  1. Untracked
  2. Tracked but not yet fetched
  3. Tracked and having a RefTarget

Whereas in your design, the states are:

  1. Not yet fetched
  2. Not tracked and having a RefTarget
  3. Tracked and having a RefTarget

Perhaps we want to allow all four states, and have remotes: BTreeMap<String, {target: Option<RefTarget>, tracking: bool}>.

I'm not sure at the moment which of these is better. Perhaps you have some thoughts? Why not forget the RefTarget when marking a branch as untracked?

I guess, in your design, whether a non-existent branch/remote pair is tracked or not is essentially determined by the auto-local-branch config. I'm think it might be useful for a user to change the tracking state of a branch before fetching, but I'm not completely sure.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jul 10, 2023

I thought of a reason to allow untracked branches with a RefTarget as you suggested.

Let's say we have a colocated repo and an untracked branch in it, and the user does git fetch remote untracked (not jj git). Then, we have 2 choices:

  1. Have jj git import import untracked@remote as usual and mark it untracked. This branch will then have an untracked remote target.
  2. Change jj git import to ignore remotes for untracked branches.

The first option seems more user-friendly.

@yuja
Copy link
Collaborator

yuja commented Jul 10, 2023

  • we could store some of this config in .jj/config.toml instead of the view object.
  • Perhaps we could make it easy to untrack a branch on all remotes or to untrack a remote for all branches. I think of the auto-local-branch config as a hack, it is currently far too inflexible.

Right. We can add/extend auto-local-branch config to apply based on branches/remotes. Gatekeeping options (evaluated once at fetch/import boundary) can be stored in config file.

  • I ran into a situation where my main branch had a * next to it because of a remote I don't care about.
    • In addition untracking having an effect on fetch/push and jj branch list, we might want this untracking to affect the way branches are presented in jj log. I'm not sure whether there are more places it should affect.

Good idea. Non-tracking remote branches can be excluded there.

  • If we store the tracking state in the View, we have to specify what happens if we merge a view with tracking set to false and a view with tracking set to true. I'm not sure there's a right answer here, but either answer shouldn't be terrible.

Good point. I think it's practically okay to merge as new_tracking = old1 | old2 ....

"absent": local_target == None && !any(x.tracking for x in remotes)

Why should an entire branch be marked as absent? I think we can allow a missing branch to be deleted on one remote and absent with respect to another remote.

Ah, it's more like a UI stuff. I was thinking how it should be rendered in jj branch list.

Also, your design is the opposite of the the one I was thinking of in the following way. I was thinking that a branch/remote pair should have one of the following states:

  1. Untracked
  2. Tracked but not yet fetched
  3. Tracked and having a RefTarget

Whereas in your design, the states are:

  1. Not yet fetched
  2. Not tracked and having a RefTarget
  3. Tracked and having a RefTarget

Perhaps, the states without RefTarget can be determined by config file when fetch/importing.

"Not tracked and having a RefTarget" is the one we don't have right now. This state shouldn't be affected by editing config file because tracked remote RefTarget should have been merged in to local RefTarget.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jan 25, 2024

I feel like this FR is obsolete now that https://github.com/martinvonz/jj/blob/9e15fba4941e00685a998d774d85f3e0cdca739a/docs/design/tracking-branches.md is implemented. We can have further discussions in new issues.

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

No branches or pull requests

3 participants