Assorted fixes revolving around rebase and merges

From: Johannes Schindelin <[email protected]>

Every once in a while, the interactive rebase makes sure that no stale files are lying around. These days, we need to include MERGE_HEAD into that set of files, as the merge command will generate them.

Signed-off-by Johannes Schindelin <[email protected]>

---
 sequencer.c | 2 ++
 1 file changed, 2 insertions(+)

sequencer.c +2 -0

index 7a9cd81afb..2f526390ac 100644
@@ -3459,6 +3459,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 			unlink(rebase_path_author_script());
 			unlink(rebase_path_stopped_sha());
 			unlink(rebase_path_amend());
+			unlink(git_path_merge_head(the_repository));
 			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
 
 			if (item->command == TODO_BREAK)

@@ -3829,6 +3830,7 @@ static int commit_staged_changes(struct replay_opts *opts,
 			   opts, flags))
 		return error(_("could not commit staged changes."));
 	unlink(rebase_path_amend());
+	unlink(git_path_merge_head(the_repository));
 	if (final_fixup) {
 		unlink(rebase_path_fixup_msg());
 		unlink(rebase_path_squash_msg());

From: Johannes Schindelin <[email protected]>

The scripted version of the rebase used to execute git reset --hard when skipping or aborting. When we ported this to C, we did update the worktree and some reflogs, but we failed to imitate git reset --hard's behavior regarding files in .git/ such as MERGE_HEAD.

Let's address this oversight.

Signed-off-by Johannes Schindelin <[email protected]>

---
 builtin/rebase.c | 3 +++
 1 file changed, 3 insertions(+)

builtin/rebase.c +2 -0

index 0ee06aa363..017dce1b50 100644
@@ -23,6 +23,7 @@
 #include "revision.h"
 #include "commit-reach.h"
 #include "rerere.h"
+#include "branch.h"
 
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "

@@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 		if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
 			die(_("could not discard worktree changes"));
+		remove_branch_state();
 		if (read_basic_state(&options))
 			exit(1);
 		goto run_rebase;

builtin/rebase.c +0 -0

 		ret = finish_rebase(&options);
 		goto cleanup;
 	}

From: Johannes Schindelin <[email protected]>

When calling merge on a branch that has already been merged, that merge is skipped quietly, but currently a MERGE_HEAD file is being left behind and will then be grabbed by the next pick (that did not want to create a *merge* commit).

Demonstrate this.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3430-rebase-merges.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

t/t3430-rebase-merges.sh +0 -0

index aa7bfc88ec..1f08a33687 100755
@@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' '
 	grep "G: +G" actual
 '

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 7 lines · writes:

From: Johannes Schindelin <[email protected]>

When calling merge on a branch that has already been merged, that merge is skipped quietly, but currently a MERGE_HEAD file is being left behind and will then be grabbed by the next pick (that did not want to create a *merge* commit).

Quoted 6 lines

Demonstrate this.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3430-rebase-merges.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

For a trivially small change/fix like this, it is OK and even preferrable to make 1+2 a single step, as applying t/ part only to try to see the breakage (or "am"ing everything and then "diff | apply -R" the part outside t/ for the same purpose) is easy enough.

Because the patch 2 with your method ends up showing only the test set-up part in the context by changing _failure to _success, without showing what end-user visible breakage the step fixed, which usually comes near the end of the added test piece. A single patch that gives tests that ought to succeed would not force the readers to switch between patches 1 and 2 while reading the fix.

Of course, the above would not apply for a more involved case where the actual fix to the code needs to span multiple patches.

Thanks.

Quoted 7 lines

t/t3430-rebase-merges.sh +0 -0

index aa7bfc88ec..1f08a33687 100755
@@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' '
 	grep "G: +G" actual
 '

t/t3430-rebase-merges.sh test_expect_success 'with --autosquash and --exec' ' · line 407

Quoted 10 lines

t/t3430-rebase-merges.sh +9 -0

 
+test_expect_failure '--continue after resolving conflicts after a merge' '
+	git checkout -b already-has-g E &&
+	git cherry-pick E..G &&
+	test_commit H2 &&
+
+	git checkout -b conflicts-in-merge H &&
+	test_commit H2 H2.t conflicts H2-conflict &&
+	test_must_fail git rebase -r already-has-g &&
+	grep conflicts H2.t &&

Is this making sure that the above test_must_fail succeeded because of a conflict and not due to any other failure? I would have used "ls-files -u H2.t" to see if the index is unmerged, which probably is a more direct way to test what this is trying to test, but if we are in the conflicted state, the one side of << == >> has this string (the other has "H2" in it, presumably?), so in practice this should be good enough.

t/t3430-rebase-merges.sh test_expect_success 'with --autosquash and --exec' ' · line 409

Quoted 2 lines

t/t3430-rebase-merges.sh +2 -0

+	echo resolved >H2.t &&
+	git add -u &&

and we resolve to continue.

t/t3430-rebase-merges.sh test_expect_success 'with --autosquash and --exec' ' · line 411

Quoted 2 lines

t/t3430-rebase-merges.sh +2 -0

+	git rebase --continue &&
+	test_must_fail git rev-parse --verify HEAD^2 &&

Even if we made an octopus by mistake, the above will catch it, which is good.

t/t3430-rebase-merges.sh test_expect_success 'with --autosquash and --exec' ' · line 415

Quoted 4 lines

t/t3430-rebase-merges.sh +3 -0

+	test_path_is_missing .git/MERGE_HEAD
+'
+
 test_done

And from the proposed log message, I am reading that the last two things (i.e. resulting tip is a child with a single parent and there is no leftover MERGE_HEAD file) fail without the fix.

This is enough material to convince me or anybody that the bug is worth fixing. Thanks for being careful noticing a glitch during your real (and otherwise unrelated to the bug) work and following through.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 28 lines · writes:

From: Johannes Schindelin <[email protected]>

The scripted version of the rebase used to execute git reset --hard when skipping or aborting. When we ported this to C, we did update the worktree and some reflogs, but we failed to imitate git reset --hard's behavior regarding files in .git/ such as MERGE_HEAD.

Let's address this oversight.

Signed-off-by Johannes Schindelin <[email protected]>

---
 builtin/rebase.c | 3 +++
 1 file changed, 3 insertions(+)

builtin/rebase.c +1 -0

index 0ee06aa363..017dce1b50 100644
@@ -23,6 +23,7 @@
 #include "revision.h"
 #include "commit-reach.h"
 #include "rerere.h"
+#include "branch.h"
 
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "

@@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1024

Quoted 11 lines

builtin/rebase.c +2 -0

 		if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
 			die(_("could not discard worktree changes"));
+		remove_branch_state();
 		if (read_basic_state(&options))
 			exit(1);
 		goto run_rebase;

@@ -1019,6 +1021,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			       options.head_name, 0, NULL, NULL) < 0)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head));
+		remove_branch_state();

Hmph. Among 5 or so callsites of reset_head(), only these two places need it, so reset_head() is clearly not a substitute for "reset --hard HEAD", and it sometimes used to switch branches or something?

Perhaps we may need to rename it to avoid confusion but it can wait until we actually decide to make it externally available. Until then, it's OK as-is, I would think.

Quoted 4 lines

builtin/rebase.c +0 -0

 		ret = finish_rebase(&options);
 		goto cleanup;
 	}

Quoted 4 lines

For a trivially small change/fix like this, it is OK and even preferrable to make 1+2 a single step, as applying t/ part only to try to see the breakage (or "am"ing everything and then "diff | apply -R" the part outside t/ for the same purpose) is easy enough.

I disagree. It helps both development and porting to different branches to be able to cherry-pick the regression test individually. Please do not ask me to violate this hard-learned principle.

Quoted 8 lines · Johannes Schindelin

Quoted 4 lines

For a trivially small change/fix like this, it is OK and even preferrable to make 1+2 a single step, as applying t/ part only to try to see the breakage (or "am"ing everything and then "diff | apply -R" the part outside t/ for the same purpose) is easy enough.

I disagree. It helps both development and porting to different branches to be able to cherry-pick the regression test individually. Please do not ask me to violate this hard-learned principle.

A trivially small change/fix like this, by definition (of "trivial" and "small" ness), it is trivial to develop and port to different branches a single patch, and test with just one half (either the test part or the code-change part) of the change reversed, to ensure that the codebase is broken without the code-change and to demonstrate that the code-change does fix the problem revealed by the test change. And "porting" by cherry-picking a single patch is always easier than two patch series.

So you may disagree all you want in your project, but do not make reviewer's lives unnecessarily harder in this project.

You misunderstand. In this case it is crucial to read the regression test first. The fix does not make much sense before one understands the condition under which the order of the code statements matters.