[v2,5/5] checkout: fix interaction between --conflict and --merge

Commit Message

Phillip Wood March 14, 2024, 5:05 p.m. UTC

From: Phillip Wood <[e-mail]>

When using "git checkout" to recreate merge conflicts or merge
uncommitted changes when switching branch "--conflict" sensibly implies
"--merge". Unfortunately the way this is implemented means that "git
checkout --conflict=diff3 --no-merge" implies "--merge" violating the
usual last-one-wins rule. Fix this by only overriding the value of
opts->merge if "--conflicts" comes after "--no-merge" or "-[-no]-merge"
is not given on the command line.

The behavior of "git checkout --merge --no-conflict" is unchanged and
will still merge on the basis that the "-[-no]-conflict" options are
primarily intended to affect the conflict style and so "--no-conflict"
should cancel a previous "--conflict" but not override "--merge".

Of the four new tests the second one tests the behavior change
introduced by this commit, the other three check that this commit does
not regress the existing behavior.

Signed-off-by: Phillip Wood <[e-mail]>
---
 builtin/checkout.c | 10 +++++---
 t/t7201-co.sh      | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 3 deletions(-)

Comments

Junio C Hamano March 14, 2024, 5:32 p.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> From: Phillip Wood <[e-mail]>
>
> When using "git checkout" to recreate merge conflicts or merge
> uncommitted changes when switching branch "--conflict" sensibly implies
> "--merge". Unfortunately the way this is implemented means that "git
> checkout --conflict=diff3 --no-merge" implies "--merge" violating the
> usual last-one-wins rule. Fix this by only overriding the value of
> opts->merge if "--conflicts" comes after "--no-merge" or "-[-no]-merge"
> is not given on the command line.

That smells like a convoluted logic but I think I can buy the
argument. If "--conflict=diff3" implies "--conflict=diff3 --merge",
then "--conflict=diff3 --no-merge" should imply "--conflict=diff3
--merge --no-merge" and the latter two cancels out with the
last-one-wins rule, leaving only "--conflict=diff3" that does not
imply anything about "--merge".  The conflict style specification
does not have any effect when we are not recreating any merge, so
all of them are ignored in the end.  So, it probably makes sense,
even though I find it highly confusing.

Is it likely that "--conflict=diff3 --no-merge" signals that the
user is confused and it is safer to abort the operation before doing
further harm, though, I wonder?

Thanks.

Patch

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d6ab3b1d665..b8ce1904897 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -100,7 +100,7 @@  struct checkout_opts {
 	struct tree *source_tree;
 };
 
-#define CHECKOUT_OPTS_INIT { .conflict_style = -1 }
+#define CHECKOUT_OPTS_INIT { .conflict_style = -1, .merge = -1 }
 
 struct branch_info {
 	char *name; /* The short name used */
@@ -1635,6 +1635,9 @@  static int parse_opt_conflict(const struct option *o, const char *arg, int unset
 	opts->conflict_style = parse_conflict_style_name(arg);
 	if (opts->conflict_style < 0)
 		return error(_("unknown conflict style '%s'"), arg);
+	/* --conflict overrides a previous --no-merge */
+	if (!opts->merge)
+		opts->merge = -1;
 
 	return 0;
 }
@@ -1742,8 +1745,9 @@  static int checkout_main(int argc, const char **argv, const char *prefix,
 			opts->show_progress = isatty(2);
 	}
 
-	if (opts->conflict_style >= 0)
-		opts->merge = 1; /* implied */
+	/* --conflicts implies --merge */
+	if (opts->merge == -1)
+		opts->merge = opts->conflict_style >= 0;
 
 	if (opts->force) {
 		opts->discard_changes = 1;
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index e1f85a91565..42352dc0dbe 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -631,6 +631,66 @@  test_expect_success 'checkout --conflict=diff3' '
 	test_cmp merged file
 '
 
+test_expect_success 'checkout --conflict=diff3 --no-conflict does not merge' '
+	setup_conflicting_index &&
+	echo "none of the above" >expect &&
+	cat expect >fild &&
+	cat expect >file &&
+	test_must_fail git checkout --conflict=diff3 --no-conflict -- fild file 2>err &&
+	test_cmp expect file &&
+	test_cmp expect fild &&
+	echo "error: path ${SQ}file${SQ} is unmerged" >expect &&
+	test_cmp expect err
+'
+
+test_expect_success 'checkout --conflict=diff3 --no-merge does not merge' '
+	setup_conflicting_index &&
+	echo "none of the above" >expect &&
+	cat expect >fild &&
+	cat expect >file &&
+	test_must_fail git checkout --conflict=diff3 --no-merge -- fild file 2>err &&
+	test_cmp expect file &&
+	test_cmp expect fild &&
+	echo "error: path ${SQ}file${SQ} is unmerged" >expect &&
+	test_cmp expect err
+'
+
+test_expect_success 'checkout --no-merge --conflict=diff3 does merge' '
+	setup_conflicting_index &&
+	echo "none of the above" >fild &&
+	echo "none of the above" >file &&
+	git checkout --no-merge --conflict=diff3 -- fild file &&
+	echo "ourside" >expect &&
+	test_cmp expect fild &&
+	cat >expect <<-\EOF &&
+	<<<<<<< ours
+	ourside
+	||||||| base
+	original
+	=======
+	theirside
+	>>>>>>> theirs
+	EOF
+	test_cmp expect file
+'
+
+test_expect_success 'checkout --merge --conflict=diff3 --no-conflict does merge' '
+	setup_conflicting_index &&
+	echo "none of the above" >fild &&
+	echo "none of the above" >file &&
+	git checkout --merge --conflict=diff3 --no-conflict -- fild file &&
+	echo "ourside" >expect &&
+	test_cmp expect fild &&
+	cat >expect <<-\EOF &&
+	<<<<<<< ours
+	ourside
+	=======
+	theirside
+	>>>>>>> theirs
+	EOF
+	test_cmp expect file
+'
+
 test_expect_success 'checkout with invalid conflict style' '
 	test_must_fail git checkout --conflict=bad 2>actual -- file &&
 	echo "error: unknown conflict style ${SQ}bad${SQ}" >expect &&