martinvonz / jj

A Git-compatible VCS that is both simple and powerful
https://martinvonz.github.io/jj/
Apache License 2.0
9.43k stars 324 forks source link

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

Open ilyagr opened 1 year ago

ilyagr commented 1 year ago

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 commented 1 year ago

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 https://github.com/martinvonz/jj/issues/963#issuecomment-1368708868 is implemented.

arxanas commented 1 year ago

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

ilyagr commented 1 year ago

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 commented 1 year ago

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

martinvonz commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

arxanas commented 1 year ago

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

--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 commented 1 year ago

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().

sunshowers commented 9 months ago

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 commented 9 months ago

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 commented 3 months ago

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 commented 3 months ago

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

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

ilyagr commented 2 months ago

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 https://github.com/martinvonz/jj/pull/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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:

neongreen commented 1 week ago

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 commented 1 week ago

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.