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: jj backout should apply working copy commit as descendant of the revert commit #2802

Open
fleimgruber opened this issue Jan 10, 2024 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@fleimgruber
Copy link

fleimgruber commented Jan 10, 2024

This originated from a conversation on discord

Is your feature request related to a problem? Please describe.
As a user, if I jj backout -r A from an empty working copy revision, the "revert" commit A' is made as a descendant of the working copy revision. I expected the working copy revision to be the descendant of the revert commit A'.

As a user, currently I need to jj rebase -r @ -d A' to achieve the expected result.

Describe the solution you'd like
The working copy revision is the descendant of the revert commit after a jj backout.

Describe alternatives you've considered

Additional context
From:

A-B-@(empty)

I expected

A-B-A'-@(empty)

but got

A-B-@(empty)-A'

@PhilipMetzger PhilipMetzger added the enhancement New feature or request label Jan 10, 2024
@martinvonz martinvonz added the good first issue Good for newcomers label Jan 11, 2024
@yuja
Copy link
Collaborator

yuja commented Jan 11, 2024

fwiw, rebase avoids this problem by requiring -d/--destination flag. I agree the current default isn't useful, but I don't know if it's good idea to add special case for the empty @.

#1188 is somewhat related, which suggests --insert-before.

@ilyagr
Copy link
Collaborator

ilyagr commented Jan 11, 2024

I wouldn't mind making --destination mandatory as Yuya suggested. I use backout very rarely, so having it be verbose is probably fine.

I can think of a few other options:

  1. We could have jj backout -r A result in A-B-A'-@ even if @ is not empty. This could be generalized to Yuya's second suggestion above: we could introduce jj backout --insert-before and make the default jj backout --insert-before @. Or, we could just have it be the default behavior without creating a new flag.

  2. We could change the default of -d to @- instead of @. Then, you'd get:

A-B-A'
   \-@

This would be weird if @ is a merge commit. I'd be fine with jj backout erroring out in this case, suggesting an explicit -d.

  1. We could change the default of -d to @- and add a --checkout or --no-checkout option to jj backout (not sure what would be the default; probably --checkout). Then, jj backout --no-checkout would work as in point 2. above. jj backout --checkout would result in:
A-B-A'-@(empty)
   \-X

where X is the old working-copy, if it was non-empty.

@ilyagr
Copy link
Collaborator

ilyagr commented Jan 11, 2024

In fact, jj backout --insert-before @ is no better than jj backout --destination @- --checkout if @ is a merge commit. So, I think I prefer either making -d mandatory or one of the last two options in my previous comment. -d would still be required if @ is a merge commit.

We can also make it mandatory now and then consider implementing something better later.

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

No branches or pull requests

5 participants