-
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
FR: Stop branches from sliding back on jj abandon
, delete them instead
#3505
Comments
Some related discussion: #1734 (comment) fwiw, I'm +1 for this change. If |
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? |
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. |
+1 from me, actually
Yep, because you could
Well that's what I thought of above, 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 |
This seems related to two other issues regarding the movement of branches: I think I would personally prefer if 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. |
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.
This is a good point, thanks! This is not my workflow currently, but potentially it might work well enough.
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 (Update: though I also worry how many other commands we'd want to or need to add this option to. 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 Another thought: if we delete branches liberally, the fact that 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 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. |
Yeah, I'm not sure whether branches should be deleted implicitly. From implementation standpoint, whether it should be handled by
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
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 I still believe there is a problem here with the current state of 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. |
Commits get abandoned by e.g. If we expand the model of #1854, |
Ah, I see, that's a good question. |
What seems quite safe is to add a |
Re the safety issue (as opposed to the workflows issue), if we did decide to delete branches on Depending on just how safe we want to be, we could either make the user specify a 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 |
I don't think |
I see it as follows:
So, it seems to me like it matters a lot what the default behaviors are. |
The default could be an error if no FWIW, |
That is true, but I think that command feels a lot more dangerous and is a lot less common than |
jj abandon
, delete them instead
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 |
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).
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 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.
Re liberal branch deleting, you should be able to use Re the CLI option:
|
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 have yet to see a use case which benefits from the sliding mechanism, but see many use cases that break because of it.
Consider:
If I were to write
jj abandon foo
, then the branchfoo
would instead move tomain
. The same thing would happen if I were to submit foo, then later runjj 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.
The text was updated successfully, but these errors were encountered: