[bug?] checkout -m doesn't work without a base version

[bug?] checkout -m doesn't work without a base version

[Pete Harlan]

Hi,

If during a merge I've resolved conflicts in foo.c but want to start
over with foo.c to resolve them differently, I can say "git checkout
-m foo.c" to restore it to its un-resolved state.

But this only works if there's a base version; if foo.c was added in
each branch, we get:

   error: path 'foo.c' does not have all three versions

Git didn't need all three versions to create the original conflicted
file, so why would it need them to recreate it?

(The message is the same if I explicitly tell Git I don't want diff3
via "git checkout --conflict=merge foo.c".)

If this is considered a bug worth fixing I'll write a test that it
fails; if it's expected behavior I think the docs should mention
that.

Thanks,

Pete Harlan
pgit@pcharlan.com

Re: [bug?] checkout -m doesn't work without a base version

[Junio C Hamano]

Pete Harlan <pgit@pcharlan.com> writes:

> But this only works if there's a base version; if foo.c was added in
> each branch, we get:
>
>    error: path 'foo.c' does not have all three versions
>
> Git didn't need all three versions to create the original conflicted
> file, so why would it need them to recreate it?

Because the original "merge" was a bit more carefully written but
"checkout -m" was written without worrying too much about "both sides
added differently" corner case and still being defensive about not doing
random thing upon getting an unexpected input state.

IOW, being lazy ;-)

How does this look?

-- >8 --
checkout -m: no need to insist on having all 3 stages

The content level merge machinery ll_merge() is prepared to merge
correctly in "both sides added differently" case by using an empty blob as
if it were the common ancestor. "checkout -m" could do the same, but didn't
bother supporting it and instead insisted on having all three stages.

Reported-by: Pete Harlan
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/checkout.c |   60 +++++++++++++++++++++++++++++++--------------------
 1 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a80772..923d040 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -114,16 +114,21 @@ static int check_stage(int stage, struct cache_entry *ce, int pos)
 		return error(_("path '%s' does not have their version"), ce->name);
 }
 
-static int check_all_stages(struct cache_entry *ce, int pos)
+static int check_stages(unsigned stages, struct cache_entry *ce, int pos)
 {
-	if (ce_stage(ce) != 1 ||
-	    active_nr <= pos + 2 ||
-	    strcmp(active_cache[pos+1]->name, ce->name) ||
-	    ce_stage(active_cache[pos+1]) != 2 ||
-	    strcmp(active_cache[pos+2]->name, ce->name) ||
-	    ce_stage(active_cache[pos+2]) != 3)
-		return error(_("path '%s' does not have all three versions"),
-			     ce->name);
+	unsigned seen = 0;
+	const char *name = ce->name;
+
+	while (pos < active_nr) {
+		ce = active_cache[pos];
+		if (strcmp(name, ce->name))
+			break;
+		seen |= (1 << ce_stage(ce));
+		pos++;
+	}
+	if ((stages & seen) != stages)
+		return error(_("path '%s' does not have all necessary versions"),
+			     name);
 	return 0;
 }
 
@@ -150,18 +155,27 @@ static int checkout_merged(int pos, struct checkout *state)
 	int status;
 	unsigned char sha1[20];
 	mmbuffer_t result_buf;
+	unsigned char threeway[3][20];
+	unsigned mode;
+
+	memset(threeway, 0, sizeof(threeway));
+	while (pos < active_nr) {
+		int stage;
+		stage = ce_stage(ce);
+		if (!stage || strcmp(path, ce->name))
+			break;
+		hashcpy(threeway[stage - 1], ce->sha1);
+		if (stage == 2)
+			mode = create_ce_mode(ce->ce_mode);
+		pos++;
+		ce = active_cache[pos];
+	}
+	if (is_null_sha1(threeway[1]) || is_null_sha1(threeway[2]))
+		return error(_("path '%s' does not have necessary versions"), path);
 
-	if (ce_stage(ce) != 1 ||
-	    active_nr <= pos + 2 ||
-	    strcmp(active_cache[pos+1]->name, path) ||
-	    ce_stage(active_cache[pos+1]) != 2 ||
-	    strcmp(active_cache[pos+2]->name, path) ||
-	    ce_stage(active_cache[pos+2]) != 3)
-		return error(_("path '%s' does not have all 3 versions"), path);
-
-	read_mmblob(&ancestor, active_cache[pos]->sha1);
-	read_mmblob(&ours, active_cache[pos+1]->sha1);
-	read_mmblob(&theirs, active_cache[pos+2]->sha1);
+	read_mmblob(&ancestor, threeway[0]);
+	read_mmblob(&ours, threeway[1]);
+	read_mmblob(&theirs, threeway[2]);
 
 	/*
 	 * NEEDSWORK: re-create conflicts from merges with
@@ -192,9 +206,7 @@ static int checkout_merged(int pos, struct checkout *state)
 	if (write_sha1_file(result_buf.ptr, result_buf.size,
 			    blob_type, sha1))
 		die(_("Unable to add merge result for '%s'"), path);
-	ce = make_cache_entry(create_ce_mode(active_cache[pos+1]->ce_mode),
-			      sha1,
-			      path, 2, 0);
+	ce = make_cache_entry(mode, sha1, path, 2, 0);
 	if (!ce)
 		die(_("make_cache_entry failed for path '%s'"), path);
 	status = checkout_entry(ce, state, NULL);
@@ -252,7 +264,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 			} else if (stage) {
 				errs |= check_stage(stage, ce, pos);
 			} else if (opts->merge) {
-				errs |= check_all_stages(ce, pos);
+				errs |= check_stages((1<<2) | (1<<3), ce, pos);
 			} else {
 				errs = 1;
 				error(_("path '%s' is unmerged"), ce->name);

Re: [bug?] checkout -m doesn't work without a base version

[Pete Harlan]

On 12/05/2011 10:58 AM, Junio C Hamano wrote:
> Pete Harlan <pgit@pcharlan.com> writes:
> 
>> But this only works if there's a base version; if foo.c was added in
>> each branch, we get:
>>
>>    error: path 'foo.c' does not have all three versions
>>
>> Git didn't need all three versions to create the original conflicted
>> file, so why would it need them to recreate it?
> 
> Because the original "merge" was a bit more carefully written but
> "checkout -m" was written without worrying too much about "both sides
> added differently" corner case and still being defensive about not doing
> random thing upon getting an unexpected input state.
> 
> IOW, being lazy ;-)
> 
> How does this look?

Wow, thanks for the quick response! That does indeed let me checkout
the file as expected.

I wrote a test (below) to be folded in with your patch, but the test
fails because it expects the restored file to be the same as the
originally-conflicted file, but the conflict-line labels change from
"HEAD" and "master":

  <<<<<<< HEAD
  in_topic
  =======
  in_master
  >>>>>>> master

to "ours" and "theirs".  (The same thing happens in the 3-way merge
case.)

If the label change is expected then I can rewrite the test to ignore
labels, or to expect "ours" and "theirs", whichever you think is best.
If the label change is unexpected, then I guess the test is good :)

--Pete

-- >8 --
Test 'checkout -m -- path' when path is a 2-way merge

Signed-off-by: Pete Harlan <pgit@pcharlan.com>
---
 t/t2023-checkout-m-twoway.sh |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100755 t/t2023-checkout-m-twoway.sh

diff --git a/t/t2023-checkout-m-twoway.sh b/t/t2023-checkout-m-twoway.sh
new file mode 100755
index 0000000..5b50360
--- /dev/null
+++ b/t/t2023-checkout-m-twoway.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='checkout -m -- conflicted/path/with/2-way/merge
+
+Ensures that checkout -m on a resolved file restores the conflicted file
+when it conflicted with a 2-way merge.'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_tick &&
+	test_commit initial_commit &&
+	git branch topic &&
+	test_commit added_in_master file.txt in_master &&
+	git checkout topic &&
+	test_commit added_in_topic file.txt in_topic
+'
+
+test_must_fail git merge master
+
+test_expect_success '-m restores 2-way conflicted+resolved file' '
+	cp file.txt file.txt.conflicted &&
+	echo resolved >file.txt &&
+	git add file.txt &&
+	git checkout -m -- file.txt &&
+	test_cmp file.txt.conflicted file.txt
+'
+
+test_done

Re: [bug?] checkout -m doesn't work without a base version

[Junio C Hamano]

Pete Harlan <pgit@pcharlan.com> writes:

> to "ours" and "theirs".  (The same thing happens in the 3-way merge
> case.)

That is entirely expected. "checkout -m" does not know or care _how_ the
index came to the conflicted state and reproduces the three-way conflicted
state in the file in the working tree solely from the contents of the
index which records only what the common thing looked like (stage #1),
what one side looked like (stage #2) and what the other side looked like
(stage #3) before the mergy operation began, so there is no way for it to
say "the other side came from foo/blah branch". There are even cases where
the conflict originally came not from any branch (think "am -3").

Re: [bug?] checkout -m doesn't work without a base version

[Pete Harlan]

On 12/08/2011 10:27 AM, Junio C Hamano wrote:
> Pete Harlan <pgit@pcharlan.com> writes:
> 
>> to "ours" and "theirs".  (The same thing happens in the 3-way merge
>> case.)
> 
> That is entirely expected. "checkout -m" does not know or care _how_ the
> index came to the conflicted state and reproduces the three-way conflicted
> state in the file in the working tree solely from the contents of the
> index which records only what the common thing looked like (stage #1),
> what one side looked like (stage #2) and what the other side looked like
> (stage #3) before the mergy operation began, so there is no way for it to
> say "the other side came from foo/blah branch". There are even cases where
> the conflict originally came not from any branch (think "am -3").

Thanks for taking the time to explain this.

Here's a test that strips branch info off the conflict lines before
verifying checkout -m, and it tests both the 2-way and 3-way cases.
The 3-way works before and after your patch; the 2-way fails before
your patch but passes after.  If the you think the test is worth
including feel free to fold it in to your patch.

--Pete

-------------------

>From a4522e06515231034c1ada65e1e91614a6c4809e Mon Sep 17 00:00:00 2001
From: Pete Harlan <pgit@pcharlan.com>
Date: Tue, 6 Dec 2011 23:01:28 -0800
Subject: [PATCH] Test 'checkout -m -- path'

Signed-off-by: Pete Harlan <pgit@pcharlan.com>
---
 t/t2023-checkout-m.sh |   47
+++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)
 create mode 100755 t/t2023-checkout-m.sh

diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh
new file mode 100755
index 0000000..1a40ce0
--- /dev/null
+++ b/t/t2023-checkout-m.sh
@@ -0,0 +1,47 @@
+#!/bin/sh
+
+test_description='checkout -m -- <conflicted path>
+
+Ensures that checkout -m on a resolved file restores the conflicted file'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_tick &&
+	test_commit both.txt both.txt initial &&
+	git branch topic &&
+	test_commit modified_in_master both.txt in_master &&
+	test_commit added_in_master each.txt in_master &&
+	git checkout topic &&
+	test_commit modified_in_topic both.txt in_topic &&
+	test_commit added_in_topic each.txt in_topic
+'
+
+test_must_fail git merge master
+
+clean_branchnames () {
+	# Remove branch names after conflict lines
+	sed 's/^\([<>]\{5,\}\) .*$/\1/'
+}
+
+test_expect_success '-m restores 2-way conflicted+resolved file' '
+	cp each.txt each.txt.conflicted &&
+	echo resolved >each.txt &&
+	git add each.txt &&
+	git checkout -m -- each.txt &&
+	clean_branchnames <each.txt >each.txt.cleaned &&
+	clean_branchnames <each.txt.conflicted >each.txt.conflicted.cleaned &&
+	test_cmp each.txt.conflicted.cleaned each.txt.cleaned
+'
+
+test_expect_success '-m restores 3-way conflicted+resolved file' '
+	cp both.txt both.txt.conflicted &&
+	echo resolved >both.txt &&
+	git add both.txt &&
+	git checkout -m -- both.txt &&
+	clean_branchnames <both.txt >both.txt.cleaned &&
+	clean_branchnames <both.txt.conflicted >both.txt.conflicted.cleaned &&
+	test_cmp both.txt.conflicted.cleaned both.txt.cleaned
+'
+
+test_done
-- 
1.7.8

Re: [bug?] checkout -m doesn't work without a base version

[Junio C Hamano]

Thanks, will queue.

[PATCH] t/t2023-checkout-m.sh: fix use of test_must_fail

[Ævar Arnfjörð Bjarmason]

Change an invocation of test_must_fail() to be inside a
test_expect_success() as is our usual pattern. Having it outside
caused our tests to fail under prove(1) since we wouldn't print a
newline before TAP output:

    CONFLICT (content): Merge conflict in both.txt
    # GETTEXT POISON #ok 2 - -m restores 2-way conflicted+resolved file

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t2023-checkout-m.sh |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh
index 1a40ce0..7e18985 100755
--- a/t/t2023-checkout-m.sh
+++ b/t/t2023-checkout-m.sh
@@ -17,7 +17,9 @@ test_expect_success setup '
 	test_commit added_in_topic each.txt in_topic
 '
 
-test_must_fail git merge master
+test_expect_success 'git merge master' '
+    test_must_fail git merge master
+'
 
 clean_branchnames () {
 	# Remove branch names after conflict lines
-- 
1.7.7.3

Re: [PATCH] t/t2023-checkout-m.sh: fix use of test_must_fail

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change an invocation of test_must_fail() to be inside a
> test_expect_success() as is our usual pattern. Having it outside
> caused our tests to fail under prove(1) since we wouldn't print a
> newline before TAP output:
>
>     CONFLICT (content): Merge conflict in both.txt
>     # GETTEXT POISON #ok 2 - -m restores 2-way conflicted+resolved file
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Thanks.

> ---
>  t/t2023-checkout-m.sh |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh
> index 1a40ce0..7e18985 100755
> --- a/t/t2023-checkout-m.sh
> +++ b/t/t2023-checkout-m.sh
> @@ -17,7 +17,9 @@ test_expect_success setup '
>  	test_commit added_in_topic each.txt in_topic
>  '
>  
> -test_must_fail git merge master
> +test_expect_success 'git merge master' '
> +    test_must_fail git merge master
> +'
>  
>  clean_branchnames () {
>  	# Remove branch names after conflict lines

Re: [bug?] checkout -m doesn't work without a base version

[Michael Schubert]

On 12/05/2011 07:58 PM, Junio C Hamano wrote:
> @@ -150,18 +155,27 @@ static int checkout_merged(int pos, struct checkout *state)
>  	int status;
>  	unsigned char sha1[20];
>  	mmbuffer_t result_buf;
> +	unsigned char threeway[3][20];
> +	unsigned mode;
> +
> +	memset(threeway, 0, sizeof(threeway));
> +	while (pos < active_nr) {
> +		int stage;
> +		stage = ce_stage(ce);
> +		if (!stage || strcmp(path, ce->name))
> +			break;
> +		hashcpy(threeway[stage - 1], ce->sha1);
> +		if (stage == 2)
> +			mode = create_ce_mode(ce->ce_mode);
> +		pos++;
> +		ce = active_cache[pos];
> +	}
> +	if (is_null_sha1(threeway[1]) || is_null_sha1(threeway[2]))
> +		return error(_("path '%s' does not have necessary versions"), path);
>  
> -	if (ce_stage(ce) != 1 ||
> -	    active_nr <= pos + 2 ||
> -	    strcmp(active_cache[pos+1]->name, path) ||
> -	    ce_stage(active_cache[pos+1]) != 2 ||
> -	    strcmp(active_cache[pos+2]->name, path) ||
> -	    ce_stage(active_cache[pos+2]) != 3)
> -		return error(_("path '%s' does not have all 3 versions"), path);
> -
> -	read_mmblob(&ancestor, active_cache[pos]->sha1);
> -	read_mmblob(&ours, active_cache[pos+1]->sha1);
> -	read_mmblob(&theirs, active_cache[pos+2]->sha1);
> +	read_mmblob(&ancestor, threeway[0]);
> +	read_mmblob(&ours, threeway[1]);
> +	read_mmblob(&theirs, threeway[2]);
>  
>  	/*
>  	 * NEEDSWORK: re-create conflicts from merges with
> @@ -192,9 +206,7 @@ static int checkout_merged(int pos, struct checkout *state)
>  	if (write_sha1_file(result_buf.ptr, result_buf.size,
>  			    blob_type, sha1))
>  		die(_("Unable to add merge result for '%s'"), path);
> -	ce = make_cache_entry(create_ce_mode(active_cache[pos+1]->ce_mode),
> -			      sha1,
> -			      path, 2, 0);
> +	ce = make_cache_entry(mode, sha1, path, 2, 0);
>  	if (!ce)
>  		die(_("make_cache_entry failed for path '%s'"), path);
>  	status = checkout_entry(ce, state, NULL);

gcc 4.6.2:

builtin/checkout.c: In function ‘cmd_checkout’:
builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized in this function [-Wuninitialized]
builtin/checkout.c:160:11: note: ‘mode’ was declared here

Re: [bug?] checkout -m doesn't work without a base version

[Junio C Hamano]

Michael Schubert <mschub@elegosoft.com> writes:

>> ...
>> +	memset(threeway, 0, sizeof(threeway));
>> +	while (pos < active_nr) {
>> +		int stage;
>> +		stage = ce_stage(ce);
>> +		if (!stage || strcmp(path, ce->name))
>> +			break;
>> +		hashcpy(threeway[stage - 1], ce->sha1);
>> +		if (stage == 2)
>> +			mode = create_ce_mode(ce->ce_mode);
>> +		pos++;
>> +		ce = active_cache[pos];
>> +	}
>> +	if (is_null_sha1(threeway[1]) || is_null_sha1(threeway[2]))
>> +		return error(_("path '%s' does not have necessary versions"), path);
>> ...
>> +	ce = make_cache_entry(mode, sha1, path, 2, 0);
>>  	if (!ce)
>>  		die(_("make_cache_entry failed for path '%s'"), path);
>>  	status = checkout_entry(ce, state, NULL);
>
> gcc 4.6.2:
>
> builtin/checkout.c: In function ‘cmd_checkout’:
> builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized in this function [-Wuninitialized]
> builtin/checkout.c:160:11: note: ‘mode’ was declared here

Isn't this just your gcc being overly cautious (aka "silly")?

The variable "mode" is assigned to when we see an stage #2 entry in the
loop, and we should have updated threeway[1] immediately before doing so.
If threeway[1] is not updated, we would have already returned before using
the variable in make_cache_entry().

Re: [bug?] checkout -m doesn't work without a base version

[Miles Bader]

Junio C Hamano <gitster@pobox.com> writes:
>> builtin/checkout.c: In function ‘cmd_checkout’:
>> builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized
>> in this function [-Wuninitialized]
>> builtin/checkout.c:160:11: note: ‘mode’ was declared here
>
> Isn't this just your gcc being overly cautious (aka "silly")?
>
> The variable "mode" is assigned to when we see an stage #2 entry in the
> loop, and we should have updated threeway[1] immediately before doing so.
> If threeway[1] is not updated, we would have already returned before using
> the variable in make_cache_entry().

Maybe that is actually guaranteed (I dunno), but it's certainly not
obvious from the code here, even to a human... any guarantee would
have to come from external invariants that the compiler doesn't know
about.

Given that, I think it's a fair warning, certainly not "silly."  This
aspect of the code doesn't seem easy to understand...

-Miles

-- 
`To alcohol!  The cause of, and solution to,
 all of life's problems' --Homer J. Simpson

Re: [bug?] checkout -m doesn't work without a base version

[Michael Schubert]

On 12/15/2011 05:20 AM, Miles Bader wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>>> builtin/checkout.c: In function ‘cmd_checkout’:
>>> builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized
>>> in this function [-Wuninitialized]
>>> builtin/checkout.c:160:11: note: ‘mode’ was declared here
>>
>> Isn't this just your gcc being overly cautious (aka "silly")?
>>
>> The variable "mode" is assigned to when we see an stage #2 entry in the
>> loop, and we should have updated threeway[1] immediately before doing so.
>> If threeway[1] is not updated, we would have already returned before using
>> the variable in make_cache_entry().
> 
> Maybe that is actually guaranteed (I dunno), but it's certainly not
> obvious from the code here, even to a human... any guarantee would
> have to come from external invariants that the compiler doesn't know
> about.
> 
> Given that, I think it's a fair warning, certainly not "silly."  This
> aspect of the code doesn't seem easy to understand...

Silly or not: That's what gcc 4.6.x is warning about with -Wuninitialized
(-Wall) set - I didn't set any additional options, just plain `make all`.

Re: [bug?] checkout -m doesn't work without a base version

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

> The variable "mode" is assigned to when we see an stage #2 entry in the
> loop, and we should have updated threeway[1] immediately before doing so.
> If threeway[1] is not updated, we would have already returned before using
> the variable in make_cache_entry().

How can you be sure that ce_stage(ce) ever returns 2?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [bug?] checkout -m doesn't work without a base version

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> The variable "mode" is assigned to when we see an stage #2 entry in the
>> loop, and we should have updated threeway[1] immediately before doing so.
>> If threeway[1] is not updated, we would have already returned before using
>> the variable in make_cache_entry().
>
> How can you be sure that ce_stage(ce) ever returns 2?

You cannot and and there are cases where you exit the loop without finding
a stage #2 entry. But in that case threeway[1] stays 0{40} and control is
returned to the caller without ever getting to the place where the
variable is used.

You could do the usual "unnecessary initialization" trick, though.

 builtin/checkout.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 31aa248..064e7a1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -152,7 +152,7 @@ static int checkout_merged(int pos, struct checkout *state)
 	unsigned char sha1[20];
 	mmbuffer_t result_buf;
 	unsigned char threeway[3][20];
-	unsigned mode;
+	unsigned mode = 0;
 
 	memset(threeway, 0, sizeof(threeway));
 	while (pos < active_nr) {

Re: [bug?] checkout -m doesn't work without a base version

[Ramsay Jones]

Junio C Hamano wrote:
> You could do the usual "unnecessary initialization" trick, though.

Yes, I wrote exactly this patch some days ago (when it first appeared
in pu), but haven't found the time to send it ... :-D

ATB,
Ramsay Jones