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: Stop branches from sliding back on jj abandon, delete them instead #3505

Open
matts1 opened this issue Apr 14, 2024 · 18 comments
Open

FR: Stop branches from sliding back on jj abandon, delete them instead #3505

matts1 opened this issue Apr 14, 2024 · 18 comments
Labels
enhancement New feature or request

Comments

@matts1
Copy link
Collaborator

matts1 commented Apr 14, 2024

Is your feature request related to a problem? Please describe.
Branches currently are designed to slide back when a change is deleted. @arxanas wrote:

I don't think branches should slide backwards to begin with. git-branchless deletes the branch, and I'm actually surprised to hear that Mercurial slides it backwards. Sapling also deletes the branch (regardless of whether you address the commit via hash or branch):

I have yet to see a use case which benefits from the sliding mechanism, but see many use cases that break because of it.

Consider:

◉  pvlpykyo msta 5 days ago foo 13bbdfc2
│  My local commit
◉  vzokzlpu msta 5 days ago main a2016214
│  immutable head here

If I were to write jj abandon foo, then the branch foo would instead move to main. The same thing would happen if I were to submit foo, then later run jj rebase --skip-empty.

This is a problem for multiple use cases. The first is when you want to associate each commit with a change (eg. make one commit branch be crrev.com/c/123)

Describe alternatives you've considered
I made #3482 earlier. However, this would be my preferred solution.

Additional context
Add any other context or screenshots about the feature request here.

@yuja
Copy link
Collaborator

yuja commented Apr 15, 2024

Some related discussion: #1734 (comment)

fwiw, I'm +1 for this change. If jj abandon shows status/warning message about deleted branches, it wouldn't be scary to automatically remove the associated branches (which could be later pushed to remotes.)

@matts1
Copy link
Collaborator Author

matts1 commented Apr 15, 2024

That comment sounds pretty reasonable to me, but I think that you're right that if we show some kind of warning, that should eliminate those concerns.

@ilyagr as the original author of the comment, do you feel that if we added a warning you'd be ok with having it deleted?

@ilyagr
Copy link
Collaborator

ilyagr commented Apr 15, 2024

I do often use branches in a way where branches sliding back is convenient, as I explained in #1734 (comment).

I also wonder if this should be considered together with topics (#3402). I haven't thought about that carefully, but the cases I can immediately think of where it's convenient for branches to slide back treat branches as topics, which is the kind of thinking heavily suggested by Git. This is also how I treat them 90% of the time. But perhaps splitting branch use-cases into "topic" (which I'm imagining as a set of consecutive commits, exported to Git as a branch for the tip) and "nice name for a commit" would be natural.

Overall, I don't have a conclusion. Changing the behavior with no replacement seems like it would be quite inconvenient for me. It also seems to me that this would make harder the use-cases where a branch represents a PR and PRs often consist of multiple commits. OTOH, I know that @yuja makes such PRs and doesn't like the sliding-back behavior.

It would help if I had a clearer understanding of what use-cases would be made easier by changing the behavior (the two that are clear to me are single-commit PRs and associating changelist numbers with commits) and how the changed behavior would support the use-cases that are currently easy.


Whether a warning is sufficient is a separate question. We have to make an educated guess whether it'd be sufficient to ensure that users don't delete important branches by accident. I don't feel like I have a clear opinion one way or another at the moment.

@necauqua
Copy link
Collaborator

+1 from me, actually

If jj abandon shows status/warning message about deleted branches, it wouldn't be scary

Yep, because you could jj undo it if you didn't expect that.

[..] whether it'd be sufficient to ensure that users don't delete important branches by accident

Well that's what I thought of above, jj undo mostly negates that

Moving the branch backwards only makes sense to me if branches auto-advanced, since then you'd ofren have a commit that you abandon have branches.

The typical workflow is I make a few commits on top of some commit with a branch and then only before actually having to push to a git remote do I do the jj branch set xyz -r @- dance, or just jj git push -c in case of PRs

@emesterhazy
Copy link
Collaborator

This seems related to two other issues regarding the movement of branches:

  1. What should jj split do with branches? #3419
  2. FR: --advance-branches flag for jj commit #2338

I think I would personally prefer if jj never moved branches automatically and allowed me to move them freely without requiring the --allow-backwards flag. This means that jj split would conflict branches instead of moving them, among other things.

There are probably others who would prefer a more "git compatible" workflow where the branch moves forwards and backwards as commits are added and abandoned.

Maybe we can have two modes of operation controlled by a config setting. This is the direction #3129 is moving in as an experiment. Or maybe it would be better to add a separate mechanism like "aliases" that never move implicitly and then change branches so that they do move implicitly.

In any case, I think we're going to have lots of users in both camps, so we should think about how we can support both.

@PhilipMetzger PhilipMetzger added the enhancement New feature or request label Apr 15, 2024
@ilyagr
Copy link
Collaborator

ilyagr commented Apr 15, 2024

[..] whether it'd be sufficient to ensure that users don't delete important branches by accident

Well that's what I thought of above, jj undo mostly negates that

I agree that if there's a warning, and the user notices it immediately, undo will help.

I am more concerned about deleting the branch on the remote, if you didn't notice the warning. The branch deletion could then have happened many operations ago, and might not be easy to undo.

The typical workflow is I make a few commits on top of some commit with a branch and then only before actually having to push to a git remote do I do the jj branch set xyz -r @- dance, or just jj git push -c in case of PRs

This is a good point, thanks! This is not my workflow currently, but potentially it might work well enough.

Maybe we can have two modes of operation controlled by a config setting. This is the direction #3129 is moving in as an experiment. Or maybe it would be better to add a separate mechanism like "aliases" that never move implicitly and then change branches so that they do move implicitly.

I think this is too large a change to be governed by a config setting. For example, it will be hard to follow bug reports if people don't remember to share how they configured jj abandon. I'd prefer an option to jj abandon that performs the non-default thing.

(Update: though I also worry how many other commands we'd want to or need to add this option to. rebase --skip-empty is a candidate, though I don't use it enough to have a clear opinion).


Update: See Yuya's follow-up comment for an important correction/answer to this section.

Another question: what if the working copy is at a "discardable" commit (an empty commit with no descendants), has a branch at it, and you jj new another_commit. Currently, the discardable commit would be abandoned and the branch would move up. Do we want a branch to disappear in this case?


Another thought: if we delete branches liberally, the fact that jj branch set refuses to create branches would become annoying. We'd probably have to unify it with jj branch create again. I know that some people already want that; but the typoer that I am, I'd be a bit sad about that. (I don't think this is a major issue, though).


Overall, considering all the support this idea is getting, I personally would be OK with trying this as long as there's an option to jj abandon to slide branches back, and as long as we have an answer to the "discardable commits" issue and other such issues we can think of.

However, if people do not feel this is urgent, I also would prefer not rushing into this and spending some time figuring out if we can come up with a better design. I prefer a slower approach to the "try it and see if users complain" approach in this case, though again, I'm OK with doing things faster if people strongly prefer it (e.g. are really inconvenienced by the current behavior).

I still feel that while this is an improvement who dislike branches and use them minimally, it would be a step back for people who use the GitHub PR model and like PRs with many commit, which is an area where jj currently shines IMO. At this point, this is more of a feeling for me than a fully reasoned-out opinion.

@yuja
Copy link
Collaborator

yuja commented Apr 16, 2024

I also worry how many other commands we'd want to or need to add this option to. rebase --skip-empty is a candidate, though I don't use it enough to have a clear opinion.

Yeah, I'm not sure whether branches should be deleted implicitly. From implementation standpoint, whether it should be handled by cmd_abandon() or rebaser.

Another question: what if the working copy is at a "discardable" commit (an empty commit with no descendants), has a branch at it, and you jj new another_commit. Currently, the discardable commit would be abandoned and the branch would move up. Do we want a branch to disappear in this case?

The current behavior is not abandoning the old working-copy commit if it had a branch #1854. This may change in future if we find a better branch movement model.

@ilyagr
Copy link
Collaborator

ilyagr commented Apr 16, 2024

The current behavior is not abandoning the old working-copy commit if it had a branch #1854. This may change in future if we find a better branch movement model.

Thanks for the correction, good to know! This pretty much solves that issue

Yeah, I'm not sure whether branches should be deleted implicitly. From implementation standpoint, whether it should be handled by cmd_abandon() or rebaser.

I don't follow exactly, what do you mean by "deleted implicitly"?


After thinking about the issue of accidentally deleting branches on remote, I became more skeptical about a warning being sufficient. This is one of the few things that can lose valuable data unexpectedly (in Git, and as long as the remote is Git-based) and that jj will not help you to undo. I would really like for jj to evoke the feeling of "my data is safe, and it will be hard for me to lose it accidentally". Currently, jj git push will never delete a branch on a remote unless you did jj branch delete, I think, which helps make it easy for a user to come up with workflows that feel safe. I'm not sure there's a way around this without changing the idea significantly.

I still believe there is a problem here with the current state of jj that needs solving, but it doesn't feel urgent to me, and I'm leaning against the idea of "just have abandon delete the branch" as the solution. I'm hoping we'll get better, more complete ideas with time. (Or, perhaps, other variations of this idea are already in people's minds and I didn't understand them well enough).

If enough people feel that this is urgent, I'm happy to be overruled. It's possible that if we experiment with this idea, it'll turn out that I'm overly cautious. I might also be biased on the safety issue since I'm not inconvenienced by the current behavior.

@yuja
Copy link
Collaborator

yuja commented Apr 17, 2024

Yeah, I'm not sure whether branches should be deleted implicitly. From implementation standpoint, whether it should be handled by cmd_abandon() or rebaser.

I don't follow exactly, what do you mean by "deleted implicitly"?

Commits get abandoned by e.g. --skip-empty. If I do jj abandon main..@, I expect branches within the range will be deleted. I'm not sure if I expect the same behavior with jj rebase --skip-empty.

If we expand the model of #1854, rebase --skip-empty wouldn't discard commits if local branch exists. I don't think it will be a good model implementation-wise, though.

@ilyagr
Copy link
Collaborator

ilyagr commented Apr 17, 2024

Ah, I see, that's a good question.

@ilyagr
Copy link
Collaborator

ilyagr commented Apr 17, 2024

What seems quite safe is to add a --delete-branches/-D option to jj abandon. I don't think this resolves for the long-term the issue of how to model branches so that they are more intuitive for more use-cases, but it might help people for now.

@ilyagr
Copy link
Collaborator

ilyagr commented Apr 19, 2024

Re the safety issue (as opposed to the workflows issue), if we did decide to delete branches on jj abandon, we could make it pretty safe by having jj git push --all and jj git push -b 'glob:...' no longer delete branches by default.

Depending on just how safe we want to be, we could either make the user specify a --delete flag to allow deletions, or we could have a --delete flag that only allows deletions and does not permit pushing other branch changes. Then, the user will be essentially forced to list every branch they want to delete.

This might be wrong, by my sense from https://sapling-scm.com/docs/commands/push is that this is what Sapling does. (As mentioned before in this thread and in https://sapling-scm.com/docs/commands/hide, their hide command deletes local "bookmarks"; I'm not sure whether this is new or has always been the case).

@yuja
Copy link
Collaborator

yuja commented Apr 19, 2024

Re the safety issue (as opposed to the workflows issue), if we did decide to delete branches on jj abandon, we could make it pretty safe by having jj git push --all and jj git push -b 'glob:...' no longer delete branches by default.

I don't think jj git push after jj abandon --delete-branches is particularly a footgun. It's equally unsafe to push the ref after it's slid down by jj abandon. That said, I'm not against changing jj git push behavior because pushing deleted refs wouldn't be that common.

@ilyagr
Copy link
Collaborator

ilyagr commented Apr 19, 2024

It's equally unsafe to push the ref after it's slid down by jj abandon

I see it as follows:

  • After a ref is slid down by jj abandon, jj git push might lose you one commit on the remote that you already expressed disinterest in.

  • If jj abandon deleted the ref, and jj git push pushed the deletion automatically, you might lose a whole bunch of commits unexpectedly. For example, if you do your work on a dev branch, you might lose the entire dev branch.

  • If you manually specify jj abandon --delete-branches, then you confirm that you're aware that's going to happen, so it's probably OK.

So, it seems to me like it matters a lot what the default behaviors are.

@yuja
Copy link
Collaborator

yuja commented Apr 20, 2024

So, it seems to me like it matters a lot what the default behaviors are.

The default could be an error if no --delete-branches nor --allow-backwards is specified. I personally prefer --delete-branches by default, and a warning about tracked remotes should be good enough to signal user about future branch removal.

FWIW, jj abandon main..@ will move the inner branches back to main, and jj git push will effectively delete them at all.

@ilyagr
Copy link
Collaborator

ilyagr commented Apr 20, 2024

FWIW, jj abandon main..@ will move the inner branches back to main, and jj git push will effectively delete them at all.

That is true, but I think that command feels a lot more dangerous and is a lot less common than jj abandon single_commit, which makes it more acceptable. It at least has the user refer to the commits that might now disappear from the remote. I don't want jj abandon single_commit to be as dangerous as jj abandon main..single_commit with respect to remotes.

@ilyagr ilyagr changed the title FR: Don't make branches slide back FR: Stop branches from sliding back on jj abandon, delete them instead Apr 20, 2024
@martinvonz
Copy link
Owner

I think we should try to not think too much about git interop when deciding how this works. If we think it's more natural for jj abandon single_commit to delete branches pointing to it, then we can make it do that. If that feels risky w.r.t. git interop, then we make the git interop (i.e. jj git push) be more careful (probably by requiring --delete-branches). Does that make sense?

@arxanas
Copy link
Collaborator

arxanas commented Apr 20, 2024

I thought it had been written in this thread somewhere, but I don't see it right now: it would be helpful if the branch deletion warning also indicated that some number of branches would be deleted on the next push as well (if it doesn't already do that).


We could consider this from the perspective of how topics intuitively work (/should work), and port the behavior to branches somehow (or change the jj model, use topics natively, and import/export branches somehow).

  • Some number of commits belong to a topic, and abandoning one of those commits doesn't automatically abandon the whole topic.
    • If there is a parent commit in the same topic, then the imaginarily-exported Git branch would probably be repointed to that parent commit as the new topic head commit.
    • If not, then there are no more commits in the topic (probably?), so the topic/branch would be deleted.
    • Empty commits being skipped behaves the same.
  • This also answers the jj split question: a split commit surely has both of its successors join the same topic (by default), so then the Git branch would point to the new topic head, which would be the child commit.

The confusing cases from the implementation perspective are when multiple branches point to the same commit, which doesn't exactly have a topic analogue.

I would say those cases are the exception. In such cases, branches don't implement the "feature branching" model — they implement something else that we should consider entirely separately. I think there are two main cases:

  • When you create a new feature branch "off of" another branch, the Git implementation requires you to create the branch first, and only then commit to it. I think it's actually pretty strange that they didn't collapse it into a helper operation. Whom does the intermediate state benefit?
    • Consider the git checkout -b command, which many people (including me) use — why not git commit -b?
    • I experimented with this workflow in git-branchless (git record accepts --create) and I think it's perfectly fine. Maybe a little better because you have to think about less ambient state, but a little worse because the first commit to a branch is treated differently than the later commits (assuming that you don't pass the same branch name to each commit operation).
    • In jj+topics, it would probably be even easier. If you make a commit in Git to the wrong branch, then you have to 1) rewind the old branch and 2) create the new branch. With topics and one-topic-per-commit, then you could actually change the current commit's topic, in one operation, which is the operation you were trying to logically do anyways.
    • The sliding behavior would be essentially irrelevant here.
  • When you're using long-lived development branches (like stable + devel).
    • This is quite different than feature branching. It handles the case of merging in changes, rather than branching out changes, and I think it makes perfect sense to use different workflows for the two.
    • Git happens to rely on the same auto-moving behavior of branches to handle both. But these branches are a lot more like tags/pointers to a part of the commit graph than feature branches/topics.
    • The sliding behavior is not really relevant for the merge operation itself. It becomes relevant for consumers when they want to consume the newly-merged changes (i.e. jj git sync). Then the sliding behavior kicks in, and works only if there are still some unmerged commits on that branch. Otherwise, the merged branch gets slid onto main and sticks around undesiredly.

When you consider the sliding behavior for the feature branch workflow only, it's clear that it doesn't really add value by itself; it's a hack to work around the lack of principled feature branch tracking available in Git.

If there's a meaningful change as a result of the above analysis, it would be that branches should slide back until they hit another branch, in which case they're deleted.

  • I don't like it because of the complexity and difficulty of reasoning, but I think it would actually address most workflows in practice...

Re liberal branch deleting, you should be able to use jj branch set --create in all cases?


Re the CLI option:

  • Not deleting branches was the git-branchless default for some time, and you had to pass -D/--delete-branches.
  • I eventually changed the default to delete branches, so that you now have to write --no-delete-branches.
  • I do multi-commit stacked Github PRs and the issue in question doesn't really come up for me.
    • Sure, I occasionally delete an entire commit from a PR/stack, but
      • it simply doesn't happen that often, honestly.
      • Splitting happens much more frequently, for example.
      • I don't think it's worth optimizing for this workflow much.
    • I'll just manually move the branch before abandoning the commit. It's a little tedious, but it doesn't happen often, so it's fine.
    • I use git rebase -i extensively, but I absolutely don't rely on sliding branch behavior. It's really complicated, and the complexity is emergent rather than fundamental,
      • which I say after having implemented git-branchless to emit interactive rebase plans for git-rebase to consume, and having had to work around it.

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

8 participants