"git pull" throws away dirty state

"git pull" throws away dirty state

[Linus Torvalds]

Ok, this is distressing, and I suspect it's another bug of mine due to 
unpack-trees changes, but before I delve into it deeper I thought I'd 
report it here and see if others see it too, and maybe it's due to 
something else..

I'm used to having dirty state in my tree, and still being able to do a 
lot of my normal work, very much including doing pulls from others. I 
expect that if the dirty state isn't relevant to the merge, it wil just 
remain, with a message like

	xyzzy: needs update
	Merge made by recursive.

and then after the merge my changes to xyzzy are still there.

That doesn't seem to work any more. The merge is successful, but it also 
updated the working tree, overwriting my dirty state!

Appended is a test-script for this behaviour, and I get:

	Before merge:
	diff --git a/a b/a
	index e965047..eacb93d 100644
	--- a/a
	+++ b/a
	@@ -1 +1,2 @@
	 Hello
	+Hi there
	a: needs update
	Merge made by recursive.
	 b |    1 +
	 1 files changed, 1 insertions(+), 0 deletions(-)
	After merge:

with the afte-merge diff being empty.

		Linus

---
#!/bin/sh

rm -rf test-repo
mkdir test-repo
cd test-repo

git init
echo Hello > a
echo Hi > b
git add a b
git commit -m "Initial commit"

git checkout -b newbranch
echo Hullo >> b
git commit -m "Change b in 'newbranch'" b

git checkout master
echo New file > c
git add c
git commit -m "Add new file"

echo Hi there >> a
echo Before merge:
git diff
git pull . newbranch
echo After merge:
git diff

Re: "git pull" throws away dirty state

[Linus Torvalds]

On Sun, 16 Mar 2008, Linus Torvalds wrote:
> 
> Ok, this is distressing, and I suspect it's another bug of mine due to 
> unpack-trees changes, but before I delve into it deeper I thought I'd 
> report it here and see if others see it too, and maybe it's due to 
> something else..

Nope, I bisected it down to

	34110cd4e394e3f92c01a4709689b384c34645d8 is first bad commit

	Make 'unpack_trees()' have a separate source and destination index

and I'm trying to figure out what part of that triggered this bug.

		Linus

Re: "git pull" throws away dirty state

[Nicolas Pitre]

On Sun, 16 Mar 2008, Linus Torvalds wrote:

> 
> 
> On Sun, 16 Mar 2008, Linus Torvalds wrote:
> > 
> > Ok, this is distressing, and I suspect it's another bug of mine due to 
> > unpack-trees changes, but before I delve into it deeper I thought I'd 
> > report it here and see if others see it too, and maybe it's due to 
> > something else..
> 
> Nope, I bisected it down to
> 
> 	34110cd4e394e3f92c01a4709689b384c34645d8 is first bad commit
> 
> 	Make 'unpack_trees()' have a separate source and destination index
> 
> and I'm trying to figure out what part of that triggered this bug.

We really should have more tests to cover all those bugs that were 
introduced and fixed lately.

Given that Git should work fine in some cases even with a dirty work 
tree by design, I'm a bit surprised that we don't have any test case 
covering that.


Nicolas

[PATCH] Don't update unchanged merge entries

[Linus Torvalds]

In commit 34110cd4e394e3f92c01a4709689b384c34645d8 ("Make 'unpack_trees()' 
have a separate source and destination index") I introduced a really 
stupid bug in that it would always add merged entries with the CE_UPDATE 
flag set. That caused us to always re-write the file, even when it was 
already up-to-date in the source index.

Not only is that really stupid from a performance angle, but more 
importantly it's actively wrong: if we have dirty state in the tree when 
we merge, overwriting it with the result of the merge will incorrectly 
overwrite that dirty state.

This trivially fixes the problem - simply don't set the CE_UPDATE flag 
when the merge result matches the old state.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

Ok, that was a really stupid one. 

On Sun, 16 Mar 2008, Linus Torvalds wrote:
> 
> Nope, I bisected it down to
> 
> 	34110cd4e394e3f92c01a4709689b384c34645d8 is first bad commit
> 
> 	Make 'unpack_trees()' have a separate source and destination index
> 
> and I'm trying to figure out what part of that triggered this bug.

 unpack-trees.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 0cdf198..46d4f6c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -593,6 +593,8 @@ static int verify_absent(struct cache_entry *ce, const char *action,
 static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 		struct unpack_trees_options *o)
 {
+	int update = CE_UPDATE;
+
 	if (old) {
 		/*
 		 * See if we can re-use the old CE directly?
@@ -603,6 +605,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 		 */
 		if (same(old, merge)) {
 			copy_cache_entry(merge, old);
+			update = 0;
 		} else {
 			if (verify_uptodate(old, o))
 				return -1;
@@ -615,7 +618,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 		invalidate_ce_path(merge, o);
 	}
 
-	add_entry(o, merge, CE_UPDATE, CE_STAGEMASK);
+	add_entry(o, merge, update, CE_STAGEMASK);
 	return 1;
 }
 

Re: [PATCH] Don't update unchanged merge entries

[Daniel Barkalow]

On Sun, 16 Mar 2008, Linus Torvalds wrote:

> In commit 34110cd4e394e3f92c01a4709689b384c34645d8 ("Make 'unpack_trees()' 
> have a separate source and destination index") I introduced a really 
> stupid bug in that it would always add merged entries with the CE_UPDATE 
> flag set. That caused us to always re-write the file, even when it was 
> already up-to-date in the source index.
> 
> Not only is that really stupid from a performance angle, but more 
> importantly it's actively wrong: if we have dirty state in the tree when 
> we merge, overwriting it with the result of the merge will incorrectly 
> overwrite that dirty state.
> 
> This trivially fixes the problem - simply don't set the CE_UPDATE flag 
> when the merge result matches the old state.

While you're at it, you should at least fix the comment. I actually think 
it would be better to have update start out 0 and be set to CE_UPDATE 
after verify_uptodate() and verify_absent(), since those checks are what 
verifies that using CE_UPDATE is okay.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Don't update unchanged merge entries

[Linus Torvalds]

On Sun, 16 Mar 2008, Daniel Barkalow wrote:
> 
> While you're at it, you should at least fix the comment. I actually think 
> it would be better to have update start out 0 and be set to CE_UPDATE 
> after verify_uptodate() and verify_absent(), since those checks are what 
> verifies that using CE_UPDATE is okay.

Well, I just made it match the old behavior. It used to be that the 
copy_cache_entry() would clear the CE_UPDATE bit in the target 'merge' 
entry, so I just cleared "update" there, the way we used to do it.

So now we actually *do* match the comment again - the bug was that we 
didn't match it before due to it all being a bit too subtle.

		Linus

Re: "git pull" throws away dirty state

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Sun, 16 Mar 2008, Linus Torvalds wrote:
>
>> On Sun, 16 Mar 2008, Linus Torvalds wrote:
>> > 
>> > Ok, this is distressing, and I suspect it's another bug of mine due to 
>> > unpack-trees changes, but before I delve into it deeper I thought I'd 
>> > report it here and see if others see it too, and maybe it's due to 
>> > something else..
>> 
>> Nope, I bisected it down to
>> 
>> 	34110cd4e394e3f92c01a4709689b384c34645d8 is first bad commit
>> 
>> 	Make 'unpack_trees()' have a separate source and destination index
>> 
>> and I'm trying to figure out what part of that triggered this bug.
>
> We really should have more tests to cover all those bugs that were 
> introduced and fixed lately.
>
> Given that Git should work fine in some cases even with a dirty work 
> tree by design, I'm a bit surprised that we don't have any test case 
> covering that.

Yeah, I was wondering why these are not covered by
t/t1000-read-tree-m-3way:

  "git read-tree -m O A B"

       O       A       B         result      index requirements
  -------------------------------------------------------------------
    12  exists  O!=A    O!=B      take A      must match A, if exists.
                        A==B
    ------------------------------------------------------------------
    13  exists  O!=A    O==B      take A      must match A, if exists.
    ------------------------------------------------------------------
    14  exists  O==A    O!=B      take B      if exists, must either (1)
                                             match A and be up-to-date,
                                             or (2) match B.
   ------------------------------------------------------------------

But this one tests only "read-tree -m", not "read-tree -m -u".  The tests
for "-m -u" we have in t1004 is not about being comprehensive like t1000
(which covers all cases in the case table) but seems to be more about
randomly selected cases.

Patches?

Re: [PATCH] Don't update unchanged merge entries

[Daniel Barkalow]

On Sun, 16 Mar 2008, Linus Torvalds wrote:

> On Sun, 16 Mar 2008, Daniel Barkalow wrote:
> > 
> > While you're at it, you should at least fix the comment. I actually think 
> > it would be better to have update start out 0 and be set to CE_UPDATE 
> > after verify_uptodate() and verify_absent(), since those checks are what 
> > verifies that using CE_UPDATE is okay.
> 
> Well, I just made it match the old behavior. It used to be that the 
> copy_cache_entry() would clear the CE_UPDATE bit in the target 'merge' 
> entry, so I just cleared "update" there, the way we used to do it.
> 
> So now we actually *do* match the comment again - the bug was that we 
> didn't match it before due to it all being a bit too subtle.

Well, the top part of the comment suggests that this is just an 
optimization (don't bother to write out a file that you know is 
unchanged), when it's actually necessary for correctness (since we don't 
know if the working tree matches the old index).

	-Daniel
*This .sig left intentionally blank*

Re: "git pull" throws away dirty state

[Junio C Hamano]

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

> Nicolas Pitre <nico@cam.org> writes:
>
>> We really should have more tests to cover all those bugs that were 
>> introduced and fixed lately.

Yeah.

>> Given that Git should work fine in some cases even with a dirty work 
>> tree by design, I'm a bit surprised that we don't have any test case 
>> covering that.

Here is one.  I'll change the "expect_failure" to "expect_success" and
squash it into Linus's patch.

---

 t/t1004-read-tree-m-u-wf.sh |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index d609a55..df58c8d 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh
@@ -116,4 +116,45 @@ test_expect_success 'three-way not complaining on an untracked file' '
 	git read-tree -m -u --exclude-per-directory=.gitignore branch-point master side
 '
 
+test_expect_success '3-way not overwriting local changes (setup)' '
+
+	git reset --hard &&
+	git checkout -b side-a branch-point &&
+	echo >>file1 "new line to be kept in the merge result" &&
+	git commit -a -m "side-a changes file1" &&
+	git checkout -b side-b branch-point &&
+	echo >>file2 "new line to be kept in the merge result" &&
+	git commit -a -m "side-b changes file2" &&
+	git checkout side-a
+
+'
+
+test_expect_failure '3-way not overwriting local changes (our side)' '
+
+	# At this point, file1 from side-a should be kept as side-b
+	# did not touch it.
+
+	git reset --hard &&
+
+	echo >>file1 "local changes" &&
+	git read-tree -m -u branch-point side-a side-b &&
+	grep "new line to be kept" file1 &&
+	grep "local changes" file1
+
+'
+
+test_expect_success '3-way not overwriting local changes (their side)' '
+
+	# At this point, file2 from side-b should be taken as side-a
+	# did not touch it.
+
+	git reset --hard &&
+
+	echo >>file2 "local changes" &&
+	test_must_fail git read-tree -m -u branch-point side-a side-b &&
+	! grep "new line to be kept" file2 &&
+	grep "local changes" file2
+
+'
+
 test_done

Re: [PATCH] Don't update unchanged merge entries

[Linus Torvalds]

On Sun, 16 Mar 2008, Daniel Barkalow wrote:
> 
> Well, the top part of the comment suggests that this is just an 
> optimization (don't bother to write out a file that you know is 
> unchanged), when it's actually necessary for correctness (since we don't 
> know if the working tree matches the old index).

Ahh, that part. Yeah, maybe we could expand/clarify it. I don't think the 
comment is wrong per se, but yes, I'm sure it could be improved. 

		Linus

Re: [PATCH] Don't update unchanged merge entries

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sun, 16 Mar 2008, Daniel Barkalow wrote:
>> 
>> Well, the top part of the comment suggests that this is just an 
>> optimization (don't bother to write out a file that you know is 
>> unchanged), when it's actually necessary for correctness (since we don't 
>> know if the working tree matches the old index).
>
> Ahh, that part. Yeah, maybe we could expand/clarify it. I don't think the 
> comment is wrong per se, but yes, I'm sure it could be improved. 

Will squash this in (together with the test updates I sent out earlier).

 unpack-trees.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index a72ac03..4b359e0 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -602,8 +602,8 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 		 * See if we can re-use the old CE directly?
 		 * That way we get the uptodate stat info.
 		 *
-		 * This also removes the UPDATE flag on
-		 * a match.
+		 * This also removes the UPDATE flag on a match; otherwise
+		 * we will end up overwriting local changes in the work tree.
 		 */
 		if (same(old, merge)) {
 			copy_cache_entry(merge, old);