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

Option to rebase/amend verbatim (like "ours" merge strategy) #1027

Open
ilyagr opened this issue Jan 14, 2023 · 20 comments
Open

Option to rebase/amend verbatim (like "ours" merge strategy) #1027

ilyagr opened this issue Jan 14, 2023 · 20 comments
Labels
enhancement New feature or request

Comments

@ilyagr
Copy link
Contributor

ilyagr commented Jan 14, 2023

I think it would be nice for jj rebase and jj amend to have a --verbatim-rebase option that causes the rebase to preserve the contents of the commits being rebased (as opposed to the diff between them and their parents). For jj rebase, this flag could simply be --verbatim.

In hg and git, I think, this is done via the "ours" merge strategy.

Currently, I don't know a non-awkward way to do this. The best way, I think, is to jj duplicate the tip of the rebase, then rebase, and then jj restore --from duplicate, and finally jj abandon duplicate. Or you can jj rebase, then jj obslog and finally jj restore --from last_obslog_commit.

One example when this is useful is when reordering commits. If there's a conflict in the middle, you'd be able to jj update middle, resolve the conflict, and then jj amend --verbatim-rebase.

@ilyagr ilyagr added the enhancement New feature or request label Jan 14, 2023
@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 14, 2023

Alternatively, there could be a better way to restore from the pre-rebase version of a rebased commit.

It shouldn't be hard to add a revset operator for "go back in the obslog", but I don't know if that will always do the right thing, especially if the idea from #963 (comment) is implemented.

@arxanas
Copy link
Contributor

arxanas commented Apr 18, 2023

On the topic of naming, git-branchless calls this git amend --reparent.

@ilyagr
Copy link
Contributor Author

ilyagr commented Apr 18, 2023

I like the explanation you have in the docs. I'm not sure --reparent makes sense to me though. Why that word? The option changes the child, not the parent.

So far, the only other idea that came to my mind is --freeze-children.

@ilyagr
Copy link
Contributor Author

ilyagr commented Apr 18, 2023

Also, I kind of like the word "verbatim". Is it confusing in some way?

@martinvonz
Copy link
Member

Why that word?

I've used the word "reparent" before. I used it because the operation changes the (descendants') parent pointers without changing anything else in the commit (well, it does change the commit signature too).

Also, I kind of like the word "verbatim". Is it confusing in some way?

I wouldn't say it's misleading, it's just not clear to me what it means. It could mean to apply the differences verbatim, although I don't know what that would actually mean.

@ilyagr
Copy link
Contributor Author

ilyagr commented Apr 18, 2023

I'm not sure I agree, but this makes sense.

Another name this made me think of is --verbatim-children. I'm not sure whether that's clearer or more confusing.

Idea # 3: Perhaps jj amend --reparent-children could also work.

A related consideration is that for rebase -r, there are two different things you might want to do: freeze the commit you are moving or freeze its children (or both). So, rebase -r could accept both a --verbatim option and --verbatim-children. --reparent and --reparent-children might also work.

The other versions of rebase would only accept --verbatim/--reparent.

@ilyagr
Copy link
Contributor Author

ilyagr commented Apr 18, 2023

My problem with reparent is the same as your problem with verbatim: every rebase changes the parent, not just the one with --reparent. --just-reparent sounds weird. OTOH, I believe the users would quickly get used to either "reparent" or "verbatim".

@joyously
Copy link

Your first sentence here says "causes the rebase to preserve the contents of the commits being rebased" so how about --preserve

@arxanas
Copy link
Contributor

arxanas commented Apr 18, 2023

My problem with reparent is the same as your problem with verbatim: every rebase changes the parent, not just the one with --reparent.

"Reparenting" is an alternative to "rebasing", rather than a supplementary mode. Rebasing involves the application of patches, while reparenting means only that the parent edge is updated.

My other candidate for an option name was also --verbatim, so I think it's not unintuitive. I ended up not choosing it for some vague reasons, like

  • that the -v/-V short options are more conventionally related to verbose output or inverting output. (Anyways, --reparent's short option of -r/-R is already taken for jj. I didn't end up assigning it a short option; I don't think I do it often enough in practice that it needs one.)
  • that "verbatim" maybe suggested something to do with text, like the commit message or the patch contents

--preserve also seems fairly logical to me. (It also has the advantage that -p might be an available short option 😂.)

All of "reparent", "verbatim", and "preserve" have the problem that they apply to the descendants of the to-be-amended commit, not the commit itself, so none are exactly accurate. A flag name like --adopt would be accurate, but I think less intuitive than any of those three options unless we heavily rely on the ancestry analogies elsewhere in jj.

Of course, there is a simple data-driven solution: deploy all three as aliases to Google internally, collect telemetry on the most common invocation, and use that 😉.

@martinvonz
Copy link
Member

I've wanted jj squash --preserve-descendants many times recently.

I wonder which other commands should have this option. Here are all our current commands. I've indicated which ones can result in rebasing we a * (I still included other commands so you can double-check):

* abandon
backout
branch
cat
checkout
* chmod
* commit
config
* describe
diff
* diffedit
duplicate
edit
files
git remote add
git remote remove
git remote rename
git remote list
git clone
git export
* git fetch
* git import
git push
init
interdiff
log
merge
* move
new
obslog
operation log
* operation restore
* operation undo
* rebase
* resolve
* restore
show
sparse
* split
* squash
status
util completion
util mangen
util config-schema
* undo
* unsquash
* untrack
version
workspace add
workspace forget
workspace list
workspace root
workspace update-stale
help

There's also the auto-rebasing that happens if you're editing a non-head, but I think it's safe to rule that out. The same applies to auto-rebase after resolving conflicts from concurrent operations.

I think we can also safely eliminate git fetch, git import, operation restore, operation undo - it's quite confusing to think about what it would mean to preserve descendants' contents in those cases.

Finally, some of them already always preserve the descendants' content, which eliminates commit, describe, and split. What remains is:

abandon
chmod
diffedit
move
rebase
resolve
restore
squash
unsquash
untrack

I can see it being useful to preserve descendants' contents for each of these. I think we should add this flag we're talking to all of them. I wonder if it should even be a top-level flag (like --at-operation and --ignore-working-copy), but it's probably better to start with adding it just to the commands above, to reduce the risk of confusing users.

I think the only existing command that rewrites (non-head) commits in two different places is move. We could potentially have two separate flags for that (--preserve-source-descendants and --preserve-destination-descendants or something), but I think that's too much complexity for little benefit.

I think we can implement this feature by adding a MutableRepo::reparent_descendants() in addition to the existing MutableRepo::rebase_descendants().

@ilyagr ilyagr mentioned this issue Dec 12, 2023
@sunshowers
Copy link

I'll mention that Git's merge strategies both have an ours strategy (discard one side entirely) and an ours option to the standard strategy (discard one side in case there's a conflict in a file).

@martinvonz
Copy link
Member

I'll mention that Git's merge strategies both have an ours strategy (discard one side entirely) and an ours option to the standard strategy (discard one side in case there's a conflict in a file).

I usually want to keep one side completely unchanged, but good point that the ours option (resolving conflict hunks in favor of one side) can also be useful.

@ilyagr
Copy link
Contributor Author

ilyagr commented Aug 8, 2024

Regarding naming, how about rebase --restore-contents or just --restore, and squash --restore-descendants? The intent is to think of the command as a normal rebase or squash followed by a restore from the current version, just as what you'd do manually.

So, the docs for jj rebase would have:

--restore-contents
    Rebase without modifying the contents of the commits

    This is equivalent to recording the current commit ids of the commits being rebased,
    rebasing, and then doing `jj restore --from current_commit_id --into new_commit_id`.

    aliases: --restore

Whereas jj squash would have

--restore-descendants
    Squash without modifying the contents of the children of the `--into` revision

    `jj squash --into X --restore-descendants` is a shorthand for recording the current
    commit ids of the children of X, doing the `jj squash`, and then doing `jj restore
    --from current_child_commit_id --into new_child_commit_id`.

    aliases: --restore

WDYT?

@matts1
Copy link
Contributor

matts1 commented Aug 8, 2024

It's certainly an interesting idea. I find it to overcomplicate things a lot, but I also think that how complicated someone finds it will depend upon their mental model of a VCS, which if experience has taught me anything, greatly differs from person to person.

In my mental model, I switch between thinking of a commit as a tree and as a patch as required, so to me

  • jj rebase is "apply the patch X top of Y"
  • jj rebase --reparent is simply "move this commit X on top of Y in the commit graph"
  • jj rebase --restore-descendants is "apply this patch X on top of Y, find the previous value of X in the obslog, then restore it"
  • jj squash is "remove a patch from X and add the patch into Y", with an implicit evolve afterwards (I used a lot of mercurial, so internally I just treat it as an hg evolve after every operation)
  • jj squash --no-evolve or --no-rebase would probably make the most sense to me, since it's just saying "skip the thing you'd normally do", but it also may be confused with whether you don't evolve the children of the source or the destination. Note that this same problem occurs with --restore-descendants
  • jj squash --reparent would make some sense, but not as much as --no-rebase to me
  • jj squash --restore-descendants would be "remove a patch from X and add it into Y, perform your implicit evolve, and then restore changes from the obslog.

Obviously, everything I'm saying is my own mental model. My point is that I believe that this decision should be based on:

  • What mental model do users have?
  • What mental model do we want users to have?
    • I mention this because I've frequently seen that users coming from git have different mental models. Do we want to work to support their mental models, or try and push them towards one that we prefer?
  • How simply do these concepts map to these mental models?

@ilyagr
Copy link
Contributor Author

ilyagr commented Sep 3, 2024

My goal with --restore-contents is to make jj's terminology somewhat self-contained. A user of jj is likely to have used and learned about jj restore by the time they care about this option.

I feel like many of your suggestions rely on some prior learning from outside-of-jj (and also outside-of-basic-git). They will be good for people with a certain kind of experience, but maybe not in general.

Without further context, --reparent means to me "like --rebase but we're using a synonym, so it's going to be a little different". This is not terrible, and I'm aware that some people like it. However, I have not seen this word used outside of git-branchless (I think other people have?), and to me it does not explain "different how".

--no-evolve is pretty clear to people who have used hg evolve, but would be cryptic to others. It also becomes a worse option if #4146 goes in, since that would make "evolve" mean something different in jj-land.

I don't like --no-rebase because this makes it unclear whether the children are moved to become children of the new version of the commit.

So, if enough people feel like it's intuitive, I'd go with --reparent and --reparent-descendants. Otherwise, I'd go with --restore-contents and --restore-descendants (at least from these options), because I know how to unambiguously explain it. (To me, just saying "Rebase without modifying the contents of the commits" seems a tiny bit problematic since it's not entirely clear whether we're thinking of the commit as a snapshot or as a patch. We could add the word "snapshot" to the explanation, but would everyone know what it means?)

@samueltardieu
Copy link
Contributor

samueltardieu commented Sep 17, 2024

I toyed with it in #4489 for diffedit. I wonder if there are cases where we might want to rebase some descendants and reparent others, or if for the commands that have been identified by @martinvonz earlier in this thread this will be one or the other. For example, when rebasing under a commit, we might want to keep it unchanged, as well as the children of where the commits were extracted from.

@samueltardieu
Copy link
Contributor

samueltardieu commented Sep 17, 2024

I tried the same approach with jj abandon --preserve-descendants, and it is interesting to see the abandoned commit content be "squashed" into each of the abandoned commit children.

@matts1
Copy link
Contributor

matts1 commented Sep 17, 2024

I toyed with it in #4489 for diffedit. I wonder if there are cases where we might want to rebase some descendants and reparent others, or if for the commands that have been identified by @martinvonz earlier in this thread this will be one or the other. For example, when rebasing under a commit, we might want to keep it unchanged, as well as the children of where the commits were extracted from.

There are 3 cases here:

  • Preserve all descendants (handled by jj abandon foo --preserve-descendants)
  • Preserve only a single descendant (handled by jj squash -r <specific child of foo>)
  • Preserve some descendants
    • I think this is sufficiently niche that jj rebase -s <children you don't want to preserve> -d 'parents(foo)' && jj abandon --preserve-descendants foo should be sufficient

@neongreen
Copy link
Contributor

Re/ names — has the --restore-descendants ship sailed?

Maybe it's just me, but I find pretty much all options mentioned here (--reparent, --verbatim, --preserve|--preserve-descendants) more intuitive.

The --restore-descendants name only makes sense to my brain if I think "ok, first do the thing (squash/rebase/etc), then undo the rebase with 'restore'". But that's just.. an implementation detail? It could be a shorthand for squash && restore, but it could just as well be a shorthand for squash --no-rebase-afterwards (@matts1's "skip the thing you normally do").

@martinvonz
Copy link
Member

Re/ names — has the --restore-descendants ship sailed?

Not if you ask me. The flag is new since 0.22.0 (october). We can add flag with a better name and deprecate that old one.

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