feat(move): implement move --reparent like amend --reparent

[bryango]

Closes #1537. This is done in the following commits:

1. refactor out the amend --reparent logic into a .reparent_subtree method for the RebasePlanBuilder
2. move the --reparent flag into MoveOptions which may be generalized for move and possibly restack, split etc in the future (currently not implemented and would be a no-op) (now implemented, see below)
3. actually implement --reparent for the move command with a few builder.reparent_subtree calls. Currently --reparent conflicts with --fixup since I am not yet sure how it should be implemented (help wanted) the behavior for --fixup --parent may be ambiguous or confusing for users
4. add tests for move --reparent and showcase its intended behavior.
5. refactor: use eyre::bail! instead of assert!
6. implement restack --reparent
7. implement split --reparent (for --stack or --detach)
8. implement sync --reparent

I would like to test this more in real life and see if it's actually working, but I think it would be good to post it here early to gather some feedback: is this feature desired? Would this be the correct way to implement it? I have very limited knowledge on the codebase so all feedbacks are much welcomed. Thank you!

[claytonrcarter]

Thank you for this. The code looks simple enough, and solid. And the test seems thorough enough for the questions that I came in with.

Currently --reparent conflicts with --fixup since I am not yet sure how it should be implemented

I suspect that it would be fine to make --reparent and --fixup conflict, since it's not at all clear to me what the intended result should be. (ie if you are reparenting the source commit, then you're replacing the destination commit with the source ... I think?; or would you be doing the fixup and then reparenting the original child commits of the target onto the newly squashed commit. (effectively reverting the squashed changes))

would like to test this more in real life and see if it's actually working

The tests seem to confirm the behavior, so I think this would be OK to merge (pending the review comments), but it would also be interesting to see how it works for you in real life.

[bryango]

Thank you so much for taking a look! I agree with all the review comment and will try to address them in the coming week (because of day job 😢

About the comments from the original issue #1537 (comment): yes indeed --insert --reparent feels a bit awkward and I'd like to see if it is actually useful in real life.

• One thing that I like about --reparent is the guarantee that all contents (blobs?) associated with the descendants remain invariant before and after the move.

In the (not so practical) --insert --reparent example suggested there, starting from A <- B <- C and perform move --exact C --dest A --insert --reparent would actually lead to:

A <- C' (=B+C) <- B(without C) <- C(=B+C)

Note that the final C commit persists after the "move" (it's not actually moved haha) and has identical contents as before the move. This can be seen from the test here:

git-branchless/git-branchless/tests/test_move.rs

Lines 6409 to 6441 in 515faad

	// test --reparent with --insert
	{
	let (stdout, _stderr) = git.branchless(
	"move",
	&[
	"--source",
	&test3_oid.to_string(),
	"--dest",
	"master",
	"--reparent",
	"--insert",
	],
	)?;
	insta::assert_snapshot!(stdout, @r"
	Attempting rebase in-memory...
	[1/4] Committed as: 3f0558d create test3_will_also_contain_test2_when_reparented.txt
	[2/4] Committed as: e1e0b99 create test2_will_also_contain_test1_when_reparented.txt
	[3/4] Committed as: 4b5cd3e create test3_will_also_contain_test2_when_reparented.txt
	[4/4] Committed as: fee6ba0 create test1.txt
	branchless: processing 4 rewritten commits
	branchless: running command: <git-executable> checkout e1e0b9952583334793f781ef25a6ce8d861cf85f
	O f777ecc (master) create initial.txt
	|
	o 3f0558d create test3_will_also_contain_test2_when_reparented.txt
	|\
	| o fee6ba0 create test1.txt
	|
	@ e1e0b99 create test2_will_also_contain_test1_when_reparented.txt
	|
	o 4b5cd3e create test3_will_also_contain_test2_when_reparented.txt
	In-memory rebase succeeded.
	");
	}

• The idea is that one can fearlessly manipulate the history without worrying about the final contents (C in this case) being changed. I think this is something powerful about --reparent. After rearranging histories some descendants may become empty and automatically drop out from the history, but during the whole process the final contents never changes, which is a nice property to have.

• --insert --reparent above is indeed a bit of an edge case. I think the most powerful use case of --reparent is actually as an alternative for --merge (that's also why the option is placed next to --merge haha). Consider:

A <- B(diff: v1.0 -> v1.1) = trunk (immutable remote)
   \
    <- B'(diff: v1.0 -> v1.2) <- C = dev

move -b dev -d trunk would introduce conflicts that must be resolved manually, but
move -b dev -d trunk --reparent would lead to the graph:

A <- B(diff: v1.0 -> v1.1) <- B'(diff: v1.1 -> v1.2) <- C = dev

where B' has identical contents as before but becomes a "hard"-rebase on top of B, and the diff associated with B' now auto-magically becomes the intended v1.1 -> v1.2. So move --reparent can be a less confusing replacement for some "auto accept all incoming changes" version of git rebase / merge. This behavior can probably be emulated by some combinations of git reset / checkout -- with some confusing flags but I haven't been able to figure out how to do this safely and cleanly in vanilla git 🤣

[bryango]

I have implemented the consistent behavior for --reparent for all commands that accept MoveOptions. This includes move, restack, split, sync. Tests are added which verify the intended behavior. Note that for git-branchless test fix the behavior was already similar to --reparent, namely the flag is always implicitly assumed.

[claytonrcarter]

Thank you again @bryango for working on this.

I would like us to back out the changes to sync, split and restack for now. Let's move them into a different PR for now. I think that I would prefer to add --reparent only to move for now, and then see what feedback (if any) comes in the next release before extending it into more commands.

My concern is basically the same as I mentioned before: the behavior may be surprising, especially in the commands like sync and restack that are supposed to be more automatic and/or "hands off". And since both of those commands are just special cases of move, there is still a way for users to have this behavior if they wish.

As for split, I'm left wondering if a different name for the flag may be appropriate. For example, if --discard --reparent will remove the file from one commit but leave it in the child, it may be that --commute-down may be a more descriptive name. ?? 🤷

Then again, I tend to think of commits in terms of diffs, which isn't accurate. I'm open to compromise on this though. I don't want to dampen your enthusiasm!

[claytonrcarter]

I would like us to back out the changes to sync, split and restack for now. Let's move them into a different PR for now. I think that I would prefer to add --reparent only to move for now, and then see what feedback (if any) comes in the next release before extending it into more commands.

@bryango Please disregard this ☝️ comment of mine. The flags are opt-in only, so there should be no risk of unwanted misuse, and I'm OK to merge this PR for all affected commands. Once it's released, we'll find out if my concerns about confusion and awkward flag names are founded or not! 😄

I think that the other comments still need to be addressed before merge, though.

I may try to make a release soon, do you think you'll have a chance to work on this again soon?

[bryango]

I would like us to back out the changes to sync, split and restack for now. Let's move them into a different PR for now. I think that I would prefer to add --reparent only to move for now, and then see what feedback (if any) comes in the next release before extending it into more commands.

@bryango Please disregard this ☝️ comment of mine. The flags are opt-in only, so there should be no risk of unwanted misuse, and I'm OK to merge this PR for all affected commands. Once it's released, we'll find out if my concerns about confusion and awkward flag names are founded or not! 😄

I think that the other comments still need to be addressed before merge, though.

I may try to make a release soon, do you think you'll have a chance to work on this again soon?

Thank you for the reviews! I will try to work on this again but a) I am slow, and b) I would like to fix all the correctness issues before it is out, so if this couldn't make into the release, I would try to target the following one. Thanks!

[bryango]

@claytonrcarter I have (hopefully) addressed all the issues in the new commits. In particular,

• there is a merge from master to resolve conflicts (I avoided rebasing so that the commit history is more tractable, but feel free to rebase before merging), and
• there is a final commit to fix some test snapshots that I missed.

[claytonrcarter]

Sorry for the delay on this. I will try to get to it soon. In the meantime, I've rebased it onto current master.

[claytonrcarter]

Thank you, @bryango! Thank you for your patience on this and very sorry for the drawn out review. 😞 This has merged! 🚀 Can't wait to play with it.

This final version is identical to the version you pushed to 0b0a5fc in February except that I did some squashing and absorbing of some of the later commits into the relevant earlier commits. 💄

[bryango]

Awesome! Thank you so much for maintaining git-branchless!