{"thread":{"id":"12715","subject":"\"git pull\" throws away dirty state","startedAt":"2008-03-16T18:08:03Z","lastAt":"2008-03-16T21:25:46Z","messageCount":11,"participants":["Linus Torvalds","Nicolas Pitre","Daniel Barkalow","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"72223","messageId":"alpine.LFD.1.00.0803161055510.3020@woody.linux-foundation.org","threadId":"12715","inReplyTo":null,"subject":"\"git pull\" throws away dirty state","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-16T18:08:03Z","receivedAt":"2008-03-16T18:08:03Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nOk, this is distressing, and I suspect it's another bug of mine due to \nunpack-trees changes, but before I delve into it deeper I thought I'd \nreport it here and see if others see it too, and maybe it's due to \nsomething else..\n\nI'm used to having dirty state in my tree, and still being able to do a \nlot of my normal work, very much including doing pulls from others. I \nexpect that if the dirty state isn't relevant to the merge, it wil just \nremain, with a message like\n\n\txyzzy: needs update\n\tMerge made by recursive.\n\nand then after the merge my changes to xyzzy are still there.\n\nThat doesn't seem to work any more. The merge is successful, but it also \nupdated the working tree, overwriting my dirty state!\n\nAppended is a test-script for this behaviour, and I get:\n\n\tBefore merge:\n\tdiff --git a/a b/a\n\tindex e965047..eacb93d 100644\n\t--- a/a\n\t+++ b/a\n\t@@ -1 +1,2 @@\n\t Hello\n\t+Hi there\n\ta: needs update\n\tMerge made by recursive.\n\t b |    1 +\n\t 1 files changed, 1 insertions(+), 0 deletions(-)\n\tAfter merge:\n\nwith the afte-merge diff being empty.\n\n\t\tLinus\n\n---\n#!/bin/sh\n\nrm -rf test-repo\nmkdir test-repo\ncd test-repo\n\ngit init\necho Hello > a\necho Hi > b\ngit add a b\ngit commit -m \"Initial commit\"\n\ngit checkout -b newbranch\necho Hullo >> b\ngit commit -m \"Change b in 'newbranch'\" b\n\ngit checkout master\necho New file > c\ngit add c\ngit commit -m \"Add new file\"\n\necho Hi there >> a\necho Before merge:\ngit diff\ngit pull . newbranch\necho After merge:\ngit diff\n"},{"id":"72224","messageId":"alpine.LFD.1.00.0803161123260.3020@woody.linux-foundation.org","threadId":"12715","inReplyTo":"alpine.LFD.1.00.0803161055510.3020@woody.linux-foundation.org","subject":"Re: \"git pull\" throws away dirty state","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-16T18:24:59Z","receivedAt":"2008-03-16T18:24:59Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 16 Mar 2008, Linus Torvalds wrote:\n> \n> Ok, this is distressing, and I suspect it's another bug of mine due to \n> unpack-trees changes, but before I delve into it deeper I thought I'd \n> report it here and see if others see it too, and maybe it's due to \n> something else..\n\nNope, I bisected it down to\n\n\t34110cd4e394e3f92c01a4709689b384c34645d8 is first bad commit\n\n\tMake 'unpack_trees()' have a separate source and destination index\n\nand I'm trying to figure out what part of that triggered this bug.\n\n\t\tLinus\n"},{"id":"72226","messageId":"alpine.LFD.1.00.0803161434450.2947@xanadu.home","threadId":"12715","inReplyTo":"alpine.LFD.1.00.0803161123260.3020@woody.linux-foundation.org","subject":"Re: \"git pull\" throws away dirty state","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-03-16T18:37:34Z","receivedAt":"2008-03-16T18:37:34Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 16 Mar 2008, Linus Torvalds wrote:\n\n> \n> \n> On Sun, 16 Mar 2008, Linus Torvalds wrote:\n> > \n> > Ok, this is distressing, and I suspect it's another bug of mine due to \n> > unpack-trees changes, but before I delve into it deeper I thought I'd \n> > report it here and see if others see it too, and maybe it's due to \n> > something else..\n> \n> Nope, I bisected it down to\n> \n> \t34110cd4e394e3f92c01a4709689b384c34645d8 is first bad commit\n> \n> \tMake 'unpack_trees()' have a separate source and destination index\n> \n> and I'm trying to figure out what part of that triggered this bug.\n\nWe really should have more tests to cover all those bugs that were \nintroduced and fixed lately.\n\nGiven that Git should work fine in some cases even with a dirty work \ntree by design, I'm a bit surprised that we don't have any test case \ncovering that.\n\n\nNicolas\n"},{"id":"72227","messageId":"alpine.LFD.1.00.0803161136060.3020@woody.linux-foundation.org","threadId":"12715","inReplyTo":"alpine.LFD.1.00.0803161123260.3020@woody.linux-foundation.org","subject":"[PATCH] Don't update unchanged merge entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-16T18:42:50Z","receivedAt":"2008-03-16T18:42:50Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nIn commit 34110cd4e394e3f92c01a4709689b384c34645d8 (\"Make 'unpack_trees()' \nhave a separate source and destination index\") I introduced a really \nstupid bug in that it would always add merged entries with the CE_UPDATE \nflag set. That caused us to always re-write the file, even when it was \nalready up-to-date in the source index.\n\nNot only is that really stupid from a performance angle, but more \nimportantly it's actively wrong: if we have dirty state in the tree when \nwe merge, overwriting it with the result of the merge will incorrectly \noverwrite that dirty state.\n\nThis trivially fixes the problem - simply don't set the CE_UPDATE flag \nwhen the merge result matches the old state.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nOk, that was a really stupid one. \n\nOn Sun, 16 Mar 2008, Linus Torvalds wrote:\n> \n> Nope, I bisected it down to\n> \n> \t34110cd4e394e3f92c01a4709689b384c34645d8 is first bad commit\n> \n> \tMake 'unpack_trees()' have a separate source and destination index\n> \n> and I'm trying to figure out what part of that triggered this bug.\n\n unpack-trees.c |    5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 0cdf198..46d4f6c 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -593,6 +593,8 @@ static int verify_absent(struct cache_entry *ce, const char *action,\n static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\tstruct unpack_trees_options *o)\n {\n+\tint update = CE_UPDATE;\n+\n \tif (old) {\n \t\t/*\n \t\t * See if we can re-use the old CE directly?\n@@ -603,6 +605,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\t */\n \t\tif (same(old, merge)) {\n \t\t\tcopy_cache_entry(merge, old);\n+\t\t\tupdate = 0;\n \t\t} else {\n \t\t\tif (verify_uptodate(old, o))\n \t\t\t\treturn -1;\n@@ -615,7 +618,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\tinvalidate_ce_path(merge, o);\n \t}\n \n-\tadd_entry(o, merge, CE_UPDATE, CE_STAGEMASK);\n+\tadd_entry(o, merge, update, CE_STAGEMASK);\n \treturn 1;\n }\n \n"},{"id":"72233","messageId":"alpine.LNX.1.00.0803161548250.19665@iabervon.org","threadId":"12715","inReplyTo":"alpine.LFD.1.00.0803161136060.3020@woody.linux-foundation.org","subject":"Re: [PATCH] Don't update unchanged merge entries","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-16T20:00:32Z","receivedAt":"2008-03-16T20:00:32Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 16 Mar 2008, Linus Torvalds wrote:\n\n> In commit 34110cd4e394e3f92c01a4709689b384c34645d8 (\"Make 'unpack_trees()' \n> have a separate source and destination index\") I introduced a really \n> stupid bug in that it would always add merged entries with the CE_UPDATE \n> flag set. That caused us to always re-write the file, even when it was \n> already up-to-date in the source index.\n> \n> Not only is that really stupid from a performance angle, but more \n> importantly it's actively wrong: if we have dirty state in the tree when \n> we merge, overwriting it with the result of the merge will incorrectly \n> overwrite that dirty state.\n> \n> This trivially fixes the problem - simply don't set the CE_UPDATE flag \n> when the merge result matches the old state.\n\nWhile you're at it, you should at least fix the comment. I actually think \nit would be better to have update start out 0 and be set to CE_UPDATE \nafter verify_uptodate() and verify_absent(), since those checks are what \nverifies that using CE_UPDATE is okay.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"72235","messageId":"alpine.LFD.1.00.0803161338100.3020@woody.linux-foundation.org","threadId":"12715","inReplyTo":"alpine.LNX.1.00.0803161548250.19665@iabervon.org","subject":"Re: [PATCH] Don't update unchanged merge entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-16T20:40:33Z","receivedAt":"2008-03-16T20:40:33Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 16 Mar 2008, Daniel Barkalow wrote:\n> \n> While you're at it, you should at least fix the comment. I actually think \n> it would be better to have update start out 0 and be set to CE_UPDATE \n> after verify_uptodate() and verify_absent(), since those checks are what \n> verifies that using CE_UPDATE is okay.\n\nWell, I just made it match the old behavior. It used to be that the \ncopy_cache_entry() would clear the CE_UPDATE bit in the target 'merge' \nentry, so I just cleared \"update\" there, the way we used to do it.\n\nSo now we actually *do* match the comment again - the bug was that we \ndidn't match it before due to it all being a bit too subtle.\n\n\t\tLinus\n"},{"id":"72237","messageId":"7vprtuieb4.fsf@gitster.siamese.dyndns.org","threadId":"12715","inReplyTo":"alpine.LFD.1.00.0803161434450.2947@xanadu.home","subject":"Re: \"git pull\" throws away dirty state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-16T20:50:23Z","receivedAt":"2008-03-16T20:50:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> On Sun, 16 Mar 2008, Linus Torvalds wrote:\n>\n>> On Sun, 16 Mar 2008, Linus Torvalds wrote:\n>> > \n>> > Ok, this is distressing, and I suspect it's another bug of mine due to \n>> > unpack-trees changes, but before I delve into it deeper I thought I'd \n>> > report it here and see if others see it too, and maybe it's due to \n>> > something else..\n>> \n>> Nope, I bisected it down to\n>> \n>> \t34110cd4e394e3f92c01a4709689b384c34645d8 is first bad commit\n>> \n>> \tMake 'unpack_trees()' have a separate source and destination index\n>> \n>> and I'm trying to figure out what part of that triggered this bug.\n>\n> We really should have more tests to cover all those bugs that were \n> introduced and fixed lately.\n>\n> Given that Git should work fine in some cases even with a dirty work \n> tree by design, I'm a bit surprised that we don't have any test case \n> covering that.\n\nYeah, I was wondering why these are not covered by\nt/t1000-read-tree-m-3way:\n\n  \"git read-tree -m O A B\"\n\n       O       A       B         result      index requirements\n  -------------------------------------------------------------------\n    12  exists  O!=A    O!=B      take A      must match A, if exists.\n                        A==B\n    ------------------------------------------------------------------\n    13  exists  O!=A    O==B      take A      must match A, if exists.\n    ------------------------------------------------------------------\n    14  exists  O==A    O!=B      take B      if exists, must either (1)\n                                             match A and be up-to-date,\n                                             or (2) match B.\n   ------------------------------------------------------------------\n\nBut this one tests only \"read-tree -m\", not \"read-tree -m -u\".  The tests\nfor \"-m -u\" we have in t1004 is not about being comprehensive like t1000\n(which covers all cases in the case table) but seems to be more about\nrandomly selected cases.\n\nPatches?\n"},{"id":"72238","messageId":"alpine.LNX.1.00.0803161700430.19665@iabervon.org","threadId":"12715","inReplyTo":"alpine.LFD.1.00.0803161338100.3020@woody.linux-foundation.org","subject":"Re: [PATCH] Don't update unchanged merge entries","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-16T21:10:34Z","receivedAt":"2008-03-16T21:10:34Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 16 Mar 2008, Linus Torvalds wrote:\n\n> On Sun, 16 Mar 2008, Daniel Barkalow wrote:\n> > \n> > While you're at it, you should at least fix the comment. I actually think \n> > it would be better to have update start out 0 and be set to CE_UPDATE \n> > after verify_uptodate() and verify_absent(), since those checks are what \n> > verifies that using CE_UPDATE is okay.\n> \n> Well, I just made it match the old behavior. It used to be that the \n> copy_cache_entry() would clear the CE_UPDATE bit in the target 'merge' \n> entry, so I just cleared \"update\" there, the way we used to do it.\n> \n> So now we actually *do* match the comment again - the bug was that we \n> didn't match it before due to it all being a bit too subtle.\n\nWell, the top part of the comment suggests that this is just an \noptimization (don't bother to write out a file that you know is \nunchanged), when it's actually necessary for correctness (since we don't \nknow if the working tree matches the old index).\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"72239","messageId":"7vk5k2id7y.fsf@gitster.siamese.dyndns.org","threadId":"12715","inReplyTo":"7vprtuieb4.fsf@gitster.siamese.dyndns.org","subject":"Re: \"git pull\" throws away dirty state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-16T21:13:53Z","receivedAt":"2008-03-16T21:13:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Nicolas Pitre <nico@cam.org> writes:\n>\n>> We really should have more tests to cover all those bugs that were \n>> introduced and fixed lately.\n\nYeah.\n\n>> Given that Git should work fine in some cases even with a dirty work \n>> tree by design, I'm a bit surprised that we don't have any test case \n>> covering that.\n\nHere is one.  I'll change the \"expect_failure\" to \"expect_success\" and\nsquash it into Linus's patch.\n\n---\n\n t/t1004-read-tree-m-u-wf.sh |   41 +++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 41 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh\nindex d609a55..df58c8d 100755\n--- a/t/t1004-read-tree-m-u-wf.sh\n+++ b/t/t1004-read-tree-m-u-wf.sh\n@@ -116,4 +116,45 @@ test_expect_success 'three-way not complaining on an untracked file' '\n \tgit read-tree -m -u --exclude-per-directory=.gitignore branch-point master side\n '\n \n+test_expect_success '3-way not overwriting local changes (setup)' '\n+\n+\tgit reset --hard &&\n+\tgit checkout -b side-a branch-point &&\n+\techo >>file1 \"new line to be kept in the merge result\" &&\n+\tgit commit -a -m \"side-a changes file1\" &&\n+\tgit checkout -b side-b branch-point &&\n+\techo >>file2 \"new line to be kept in the merge result\" &&\n+\tgit commit -a -m \"side-b changes file2\" &&\n+\tgit checkout side-a\n+\n+'\n+\n+test_expect_failure '3-way not overwriting local changes (our side)' '\n+\n+\t# At this point, file1 from side-a should be kept as side-b\n+\t# did not touch it.\n+\n+\tgit reset --hard &&\n+\n+\techo >>file1 \"local changes\" &&\n+\tgit read-tree -m -u branch-point side-a side-b &&\n+\tgrep \"new line to be kept\" file1 &&\n+\tgrep \"local changes\" file1\n+\n+'\n+\n+test_expect_success '3-way not overwriting local changes (their side)' '\n+\n+\t# At this point, file2 from side-b should be taken as side-a\n+\t# did not touch it.\n+\n+\tgit reset --hard &&\n+\n+\techo >>file2 \"local changes\" &&\n+\ttest_must_fail git read-tree -m -u branch-point side-a side-b &&\n+\t! grep \"new line to be kept\" file2 &&\n+\tgrep \"local changes\" file2\n+\n+'\n+\n test_done\n"},{"id":"72240","messageId":"alpine.LFD.1.00.0803161413250.3020@woody.linux-foundation.org","threadId":"12715","inReplyTo":"alpine.LNX.1.00.0803161700430.19665@iabervon.org","subject":"Re: [PATCH] Don't update unchanged merge entries","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-16T21:15:10Z","receivedAt":"2008-03-16T21:15:10Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 16 Mar 2008, Daniel Barkalow wrote:\n> \n> Well, the top part of the comment suggests that this is just an \n> optimization (don't bother to write out a file that you know is \n> unchanged), when it's actually necessary for correctness (since we don't \n> know if the working tree matches the old index).\n\nAhh, that part. Yeah, maybe we could expand/clarify it. I don't think the \ncomment is wrong per se, but yes, I'm sure it could be improved. \n\n\t\tLinus\n"},{"id":"72243","messageId":"7v1w6aico5.fsf@gitster.siamese.dyndns.org","threadId":"12715","inReplyTo":"alpine.LFD.1.00.0803161413250.3020@woody.linux-foundation.org","subject":"Re: [PATCH] Don't update unchanged merge entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-16T21:25:46Z","receivedAt":"2008-03-16T21:25:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Sun, 16 Mar 2008, Daniel Barkalow wrote:\n>> \n>> Well, the top part of the comment suggests that this is just an \n>> optimization (don't bother to write out a file that you know is \n>> unchanged), when it's actually necessary for correctness (since we don't \n>> know if the working tree matches the old index).\n>\n> Ahh, that part. Yeah, maybe we could expand/clarify it. I don't think the \n> comment is wrong per se, but yes, I'm sure it could be improved. \n\nWill squash this in (together with the test updates I sent out earlier).\n\n unpack-trees.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a72ac03..4b359e0 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -602,8 +602,8 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\t * See if we can re-use the old CE directly?\n \t\t * That way we get the uptodate stat info.\n \t\t *\n-\t\t * This also removes the UPDATE flag on\n-\t\t * a match.\n+\t\t * This also removes the UPDATE flag on a match; otherwise\n+\t\t * we will end up overwriting local changes in the work tree.\n \t\t */\n \t\tif (same(old, merge)) {\n \t\t\tcopy_cache_entry(merge, old);\n"}]}