"git pull" throws away dirty state

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

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

On Sun, 16 Mar 2008, Linus Torvalds wrote:

On Sun, 16 Mar 2008, Linus Torvalds wrote:

quoted

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

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;
 }
 

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*

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

On Sun, 16 Mar 2008, Linus Torvalds wrote:

On Sun, 16 Mar 2008, Daniel Barkalow wrote:

quoted

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*

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