{"thread":{"id":"12701","subject":"recent 'unpack_trees()'-related changes break 'git stash'","startedAt":"2008-03-15T01:41:33Z","lastAt":"2008-03-30T21:14:38Z","messageCount":8,"participants":["SZEDER Gábor","Linus Torvalds","Junio C Hamano","Szeder Gábor","しらいしななこ"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"72155","messageId":"20080315014133.GB32265@neumann","threadId":"12701","inReplyTo":null,"subject":"recent 'unpack_trees()'-related changes break 'git stash'","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2008-03-15T01:41:33Z","receivedAt":"2008-03-15T01:41:33Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nt3903-stash.sh _sometimes_ fails at the 'drop middle stash' testcase.\nAfter playing around with it this evening I was able to narrow it\ndown, and turned out that it has nothing to do with 'git stash drop',\nbut something is broken behind 'stash'.\n\nUnfortunately, I can't reproduce the bug reliably.  Here is a\ntestcase, that sometimes fails:\n\n\ntest_description='Test git-stash'\n. ./test-lib.sh\ntest_expect_success 'try to catch some rare occurring stash bug' '\n\techo 1 > file &&\n\tgit add file &&\n\ttest_tick &&\n\tgit commit -m initial &&\n\techo 2 > file &&\n\ttest_tick &&\n\tgit stash &&\n\ttest 2 = $(git stash show stash@{0} | wc -l) &&\n\techo \"after first show test\"\n\techo 3 > file &&\n\tgit stash &&\n\ttest 2 = $(git stash show stash@{0} | wc -l) &&\n\techo \"after second show test\"\n'\ntest_done\n\n\nand here is a loop to run the above testcase until it fails (take\ncare, it deletes ./trash at the beginning!):\n\n\nret=0\ni=0\nwhile test $ret = 0 ; do\n\trm -rf ./trash\n\t./mystashtest.sh --verbose\n\tret=$?\n\ti=$((++i))\ndone\necho \"test failed at ${i}. run\"\n\n\nBoth should go into t/ directory.\n\nThe testcase usually fails during the first 25 run, but sometimes it\nruns more than 100 times before failing.  The test fails because the\nsecond 'git stash' sometimes does something wrong:  there is no\ndifference between stash@{0} and the clean working tree.  There is no\nerror message from 'git stash' upon failure.  During all the test runs\nI never saw a failure occuring at the first 'git stash'.\n\nI ran bisect using these scripts, and it turned out that the bug was\nintroduced by 34110cd4 (Make 'unpack_trees()' have a separate source\nand destination index, 2008-03-06).\n\nI have tried whether it has already been fixed in next or pu, but\nthose branches are affected, too.\n\n\nBest,\nGábor\n"},{"id":"72159","messageId":"alpine.LFD.1.00.0803142023490.3557@woody.linux-foundation.org","threadId":"12701","inReplyTo":"20080315014133.GB32265@neumann","subject":"Fix recent 'unpack_trees()'-related changes breaking 'git stash'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-15T04:20:41Z","receivedAt":"2008-03-15T04:20:41Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 15 Mar 2008, SZEDER G?bor wrote:\n>\n> The testcase usually fails during the first 25 run, but sometimes it\n> runs more than 100 times before failing.\n\nDamn, this series has had more subtle issues than I ever expected.\n\n'git stash' creates its saved working tree object with:\n\n        # state of the working tree\n        w_tree=$( (\n                rm -f \"$TMP-index\" &&\n                cp -p ${GIT_INDEX_FILE-\"$GIT_DIR/index\"} \"$TMP-index\" &&\n                GIT_INDEX_FILE=\"$TMP-index\" &&\n                export GIT_INDEX_FILE &&\n                git read-tree -m $i_tree &&\n                git add -u &&\n                git write-tree &&\n                rm -f \"$TMP-index\"\n        ) ) ||\n                die \"Cannot save the current worktree state\"\n\nwhich creates a new index file with the updates, and writes the tree from \nthat.\n\nWe have this logic where we compare the timestamp of the index with the \ntimestamp of the files and we then write them out \"smudged\" if they are \nthe same, and it basically depends on the fact that the date on the index \nfile is compared with the date encoded in the stat information itself.\n\nAnd what is going on is:\n\n - we create a new index file with that \"cp\". We are careful to preserve \n   the timestamps by using \"-p\", so this one should be all ok.\n\n - then we *update* that index by resetting it to the tree with git \n   read-tree, but now we do *not* preserve the timestamp on this new copy \n   any more, even though we copy over all the timestamps on the files that \n   are indexed from the stat information!\n\nNow, we always had that problem when re-writing the index, but we had this \nclever workaround in the writing part: if the source had racily clean \nentries, then when we wrote those out (and thus can't depend on the index \nfiel timestamp showing that they are racily clean any more!), we would \nsmudge them when writing. \n\nIOW, we handle this issue by having write_index() do this:\n\n\tfor (i = 0; i < entries; i++) {  \n\t\t...\n\t\tif (is_racy_timestamp(istate, ce))\n\t\t\tce_smudge_racily_clean_entry(ce);\n\t\t..\n\nwhen writing out entries. And that all took care of it, because now when \nwe wrote the new index, we'd change the timestamp on the index, yes, but \nwe'd smudge the entries we wrote out, so now the resulting index would \nstill show that file as not-up-to-date any more.\n\nBut with commit 34110cd4e394e3f92c01a4709689b384c34645d8 (\"Make \n'unpack_trees()' have a separate source and destination index\"), this \nlogic no longer triggers, because we now write out the \"result\" index, and \nthat one never got its timestamp updated from the source index, so it had \nlost all that \"is_racy_timestamp()\" information!\n\nThis trivial patch fixes it. It looks trivial, and it's a simple fix, but \nboy did it take me way too much thinking and explaining to myself to \nexplain why there was a problem in the first place!\n\nThe trivial fix is to just copy the index timestamp from the source index \ninto the result index. But we only do this if we *have* a source index, of \ncourse, and if we will even bother to use the result.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n unpack-trees.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 91649f3..77d52db 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -336,6 +336,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tstate.refresh_cache = 1;\n \n \tmemset(&o->result, 0, sizeof(o->result));\n+\tif (o->src_index && o->dst_index)\n+\t\to->result.timestamp = o->src_index->timestamp;\n \to->merge_size = len;\n \n \tif (!dfc)\n"},{"id":"72161","messageId":"alpine.LFD.1.00.0803142133160.3557@woody.linux-foundation.org","threadId":"12701","inReplyTo":"alpine.LFD.1.00.0803142023490.3557@woody.linux-foundation.org","subject":"Re: Fix recent 'unpack_trees()'-related changes breaking 'git stash'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-15T04:40:02Z","receivedAt":"2008-03-15T04:40:02Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 14 Mar 2008, Linus Torvalds wrote:\n> \n> The trivial fix is to just copy the index timestamp from the source index \n> into the result index. But we only do this if we *have* a source index, of \n> course, and if we will even bother to use the result.\n\nActually, that second part of the test is just unnecessarily clever, and \nit's just asking for trouble.\n\nEven if we never use the \"result\" for anything in the end, it's probably a \ngood idea to have its timestamp match the source timestamp just in case \nsomebody wants to do the \"is_racy_timestamp()\" on the result while it's \nbeing generated (and before it is thrown away).\n\nIn particular, it would not be necessarily wrong to use ie_match_stat() on \nthe result index in a callback.\n\nSo it might be better to make that thing be just\n\n>  \tmemset(&o->result, 0, sizeof(o->result));\n> +\tif (o->src_index)\n> +\t\to->result.timestamp = o->src_index->timestamp;\n>  \to->merge_size = len;\n\ninstead of checking both src_index *and* dst_index. The source index is \nall that matters anyway. Even if 'o->result' isn't used in the end, who \ncares? We can still give it the right timestamp.\n\nAnd no, this really isn't likely to matter, but let's pick the simpler \nversion if it doesn't matter.\n\n\t\tLinus\n"},{"id":"72163","messageId":"7v1w6cpox6.fsf@gitster.siamese.dyndns.org","threadId":"12701","inReplyTo":"alpine.LFD.1.00.0803142023490.3557@woody.linux-foundation.org","subject":"Re: Fix recent 'unpack_trees()'-related changes breaking 'git stash'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-15T04:54:45Z","receivedAt":"2008-03-15T04:54:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Damn, this series has had more subtle issues than I ever expected.\n>\n> 'git stash' creates its saved working tree object with:\n>\n>         # state of the working tree\n>         w_tree=$( (\n>                 rm -f \"$TMP-index\" &&\n>                 cp -p ${GIT_INDEX_FILE-\"$GIT_DIR/index\"} \"$TMP-index\" &&\n>                 GIT_INDEX_FILE=\"$TMP-index\" &&\n>                 export GIT_INDEX_FILE &&\n>                 git read-tree -m $i_tree &&\n>                 git add -u &&\n>                 git write-tree &&\n>                 rm -f \"$TMP-index\"\n>         ) ) ||\n>                 die \"Cannot save the current worktree state\"\n>\n> which creates a new index file with the updates, and writes the tree from \n> that.\n\nIt would be slightly simpler to write the above sequence like this:\n\n\tw_tree=$( (\n\t\trm -f \"$TMP-index\" &&\n                git read-tree --index-output=\"$TMP-index\" -m $i_tree &&\n                GIT_INDEX_FILE=\"$TMP-index\" &&\n                export GIT_INDEX_FILE &&\n                git add -u &&\n                git write-tree &&\n                rm -f \"$TMP-index\"\n\t) )\n\nI think your fix would apply equally well if we rewrite stash to work like\nthis.\n"},{"id":"72177","messageId":"20080315113154.GA10921@elysium.homelinux.org","threadId":"12701","inReplyTo":"alpine.LFD.1.00.0803142133160.3557@woody.linux-foundation.org","subject":"Re: Fix recent 'unpack_trees()'-related changes breaking 'git stash'","fromName":"Szeder Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2008-03-15T11:31:54Z","receivedAt":"2008-03-15T11:31:54Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Mar 14, 2008 at 09:40:02PM -0700, Linus Torvalds wrote:\n> So it might be better to make that thing be just\n> \n> >  \tmemset(&o->result, 0, sizeof(o->result));\n> > +\tif (o->src_index)\n> > +\t\to->result.timestamp = o->src_index->timestamp;\n> >  \to->merge_size = len;\n> \n> instead of checking both src_index *and* dst_index. The source index is \n> all that matters anyway. Even if 'o->result' isn't used in the end, who \n> cares? We can still give it the right timestamp.\n> \n> And no, this really isn't likely to matter, but let's pick the simpler \n> version if it doesn't matter.\nI have applied the above patch and run both my stripped-down testcase\nand t3903-stash.sh a couple thousand times without a single failure.\n\n\nBest,\nGábor\n"},{"id":"72178","messageId":"20080315113607.GB10921@elysium.homelinux.org","threadId":"12701","inReplyTo":"7v1w6cpox6.fsf@gitster.siamese.dyndns.org","subject":"Re: Fix recent 'unpack_trees()'-related changes breaking 'git stash'","fromName":"Szeder Gábor","fromEmail":"szeder@fzi.de","sentAt":"2008-03-15T11:36:07Z","receivedAt":"2008-03-15T11:36:07Z","isPatch":false,"sender":{"key":"szeder@fzi.de","avatar":null},"body":"On Fri, Mar 14, 2008 at 09:54:45PM -0700, Junio C Hamano wrote:\n> It would be slightly simpler to write the above sequence like this:\n> \n> \tw_tree=$( (\n> \t\trm -f \"$TMP-index\" &&\n>                 git read-tree --index-output=\"$TMP-index\" -m $i_tree &&\n>                 GIT_INDEX_FILE=\"$TMP-index\" &&\n>                 export GIT_INDEX_FILE &&\n>                 git add -u &&\n>                 git write-tree &&\n>                 rm -f \"$TMP-index\"\n> \t) )\n> \n> I think your fix would apply equally well if we rewrite stash to work like\n> this.\nYes, with the above changes but without Linus' patch the bug is still\npresent.\n\n\nBest,\nGábor\n"},{"id":"72187","messageId":"alpine.LFD.1.00.0803150934100.3557@woody.linux-foundation.org","threadId":"12701","inReplyTo":"7v1w6cpox6.fsf@gitster.siamese.dyndns.org","subject":"Re: Fix recent 'unpack_trees()'-related changes breaking 'git stash'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-15T16:51:21Z","receivedAt":"2008-03-15T16:51:21Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 14 Mar 2008, Junio C Hamano wrote:\n> \n> It would be slightly simpler to write the above sequence like this:\n> \n> \tw_tree=$( (\n> \t\trm -f \"$TMP-index\" &&\n>                 git read-tree --index-output=\"$TMP-index\" -m $i_tree &&\n\nAck. That's an independent cleanup.\n\nIn fact, I would almost prefer to try to stop using GIT_INDEX_FILE \nentirely, and add it as a top-level git flag, so you can then make the \nrest be:\n\n\tgit --index-file \"$TMP-index\" add -u &&\n\tgit --index-file \"$TMP-index\" write-tree &&\n\trm -f \"$TMP-index\"\n\ninstead of doing that\n\n>                 GIT_INDEX_FILE=\"$TMP-index\" &&\n>                 export GIT_INDEX_FILE &&\n>                 git add -u &&\n>                 git write-tree &&\n>                 rm -f \"$TMP-index\"\n\nthing.\n\n\nSomething like the appended, in other words.\n\nOh, and that whole git.c argument parsing should be made to use the proper \narg parser, too. But I'm too damn lazy and not comfy enough with \n'parse_options()' usage. Somebody who is should take a look..\n\n\t\tLinus\n\n---\n git-stash.sh |    9 +++------\n git.c        |   15 +++++++++++++++\n 2 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex c2b6820..95b65dc 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -63,12 +63,9 @@ create_stash () {\n \t# state of the working tree\n \tw_tree=$( (\n \t\trm -f \"$TMP-index\" &&\n-\t\tcp -p ${GIT_INDEX_FILE-\"$GIT_DIR/index\"} \"$TMP-index\" &&\n-\t\tGIT_INDEX_FILE=\"$TMP-index\" &&\n-\t\texport GIT_INDEX_FILE &&\n-\t\tgit read-tree -m $i_tree &&\n-\t\tgit add -u &&\n-\t\tgit write-tree &&\n+\t\tgit read-tree --index-output=\"$TMP-index\" -m $i_tree &&\n+\t\tgit --index-file \"$TMP-index\" add -u &&\n+\t\tgit --index-file \"$TMP-index\" write-tree &&\n \t\trm -f \"$TMP-index\"\n \t) ) ||\n \t\tdie \"Cannot save the current worktree state\"\ndiff --git a/git.c b/git.c\nindex 13de801..a615df9 100644\n--- a/git.c\n+++ b/git.c\n@@ -55,6 +55,21 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n+\t\t} else if (!strcmp(cmd, \"--index-file\")) {\n+\t\t\tif (*argc < 2) {\n+\t\t\t\tfprintf(stderr, \"No directory given for --index-file.\\n\" );\n+\t\t\t\tusage(git_usage_string);\n+\t\t\t}\n+\t\t\tsetenv(INDEX_ENVIRONMENT, (*argv)[1], 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n+\t\t\t(*argv)++;\n+\t\t\t(*argc)--;\n+\t\t\thandled++;\n+\t\t} else if (!prefixcmp(cmd, \"--index-file=\")) {\n+\t\t\tsetenv(INDEX_ENVIRONMENT, cmd + 13, 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--work-tree\")) {\n \t\t\tif (*argc < 2) {\n \t\t\t\tfprintf(stderr, \"No directory given for --work-tree.\\n\" );\n"},{"id":"73367","messageId":"200803302115.m2ULFGLP021077@mi0.bluebottle.com","threadId":"12701","inReplyTo":"alpine.LFD.1.00.0803150934100.3557@woody.linux-foundation.org","subject":"[PATCH] git-stash: use git-read-tree --index-output option","fromName":"しらいしななこ","fromEmail":"nanako3@bluebottle.com","sentAt":"2008-03-30T21:14:38Z","receivedAt":"2008-03-30T21:14:38Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Instead of copying the original index with \"cp -p\" to preserve timestamp, use \"--index-output\" option of git-read-tree program.\n\nSigned-off-by: Nanako Shiraishi <nanako3@bluebottle.com>\n---\n\n git-stash.sh |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex c2b6820..ca49c5e 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -63,10 +63,9 @@ create_stash () {\n \t# state of the working tree\n \tw_tree=$( (\n \t\trm -f \"$TMP-index\" &&\n-\t\tcp -p ${GIT_INDEX_FILE-\"$GIT_DIR/index\"} \"$TMP-index\" &&\n+\t\tgit read-tree --index-output=\"$TMP-index\" -m $i_tree &&\n \t\tGIT_INDEX_FILE=\"$TMP-index\" &&\n \t\texport GIT_INDEX_FILE &&\n-\t\tgit read-tree -m $i_tree &&\n \t\tgit add -u &&\n \t\tgit write-tree &&\n \t\trm -f \"$TMP-index\"\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n\n----------------------------------------------------------------------\nFinally - A spam blocker that actually works.\nhttp://www.bluebottle.com/tag/4\n"}]}