recent 'unpack_trees()'-related changes break 'git stash'

recent 'unpack_trees()'-related changes break 'git stash'

[SZEDER Gábor]

Hi,

t3903-stash.sh _sometimes_ fails at the 'drop middle stash' testcase.
After playing around with it this evening I was able to narrow it
down, and turned out that it has nothing to do with 'git stash drop',
but something is broken behind 'stash'.

Unfortunately, I can't reproduce the bug reliably.  Here is a
testcase, that sometimes fails:


test_description='Test git-stash'
. ./test-lib.sh
test_expect_success 'try to catch some rare occurring stash bug' '
	echo 1 > file &&
	git add file &&
	test_tick &&
	git commit -m initial &&
	echo 2 > file &&
	test_tick &&
	git stash &&
	test 2 = $(git stash show stash@{0} | wc -l) &&
	echo "after first show test"
	echo 3 > file &&
	git stash &&
	test 2 = $(git stash show stash@{0} | wc -l) &&
	echo "after second show test"
'
test_done


and here is a loop to run the above testcase until it fails (take
care, it deletes ./trash at the beginning!):


ret=0
i=0
while test $ret = 0 ; do
	rm -rf ./trash
	./mystashtest.sh --verbose
	ret=$?
	i=$((++i))
done
echo "test failed at ${i}. run"


Both should go into t/ directory.

The testcase usually fails during the first 25 run, but sometimes it
runs more than 100 times before failing.  The test fails because the
second 'git stash' sometimes does something wrong:  there is no
difference between stash@{0} and the clean working tree.  There is no
error message from 'git stash' upon failure.  During all the test runs
I never saw a failure occuring at the first 'git stash'.

I ran bisect using these scripts, and it turned out that the bug was
introduced by 34110cd4 (Make 'unpack_trees()' have a separate source
and destination index, 2008-03-06).

I have tried whether it has already been fixed in next or pu, but
those branches are affected, too.


Best,
Gábor

Fix recent 'unpack_trees()'-related changes breaking 'git stash'

[Linus Torvalds]

On Sat, 15 Mar 2008, SZEDER G?bor wrote:
>
> The testcase usually fails during the first 25 run, but sometimes it
> runs more than 100 times before failing.

Damn, this series has had more subtle issues than I ever expected.

'git stash' creates its saved working tree object with:

        # state of the working tree
        w_tree=$( (
                rm -f "$TMP-index" &&
                cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
                GIT_INDEX_FILE="$TMP-index" &&
                export GIT_INDEX_FILE &&
                git read-tree -m $i_tree &&
                git add -u &&
                git write-tree &&
                rm -f "$TMP-index"
        ) ) ||
                die "Cannot save the current worktree state"

which creates a new index file with the updates, and writes the tree from 
that.

We have this logic where we compare the timestamp of the index with the 
timestamp of the files and we then write them out "smudged" if they are 
the same, and it basically depends on the fact that the date on the index 
file is compared with the date encoded in the stat information itself.

And what is going on is:

 - we create a new index file with that "cp". We are careful to preserve 
   the timestamps by using "-p", so this one should be all ok.

 - then we *update* that index by resetting it to the tree with git 
   read-tree, but now we do *not* preserve the timestamp on this new copy 
   any more, even though we copy over all the timestamps on the files that 
   are indexed from the stat information!

Now, we always had that problem when re-writing the index, but we had this 
clever workaround in the writing part: if the source had racily clean 
entries, then when we wrote those out (and thus can't depend on the index 
fiel timestamp showing that they are racily clean any more!), we would 
smudge them when writing. 

IOW, we handle this issue by having write_index() do this:

	for (i = 0; i < entries; i++) {  
		...
		if (is_racy_timestamp(istate, ce))
			ce_smudge_racily_clean_entry(ce);
		..

when writing out entries. And that all took care of it, because now when 
we wrote the new index, we'd change the timestamp on the index, yes, but 
we'd smudge the entries we wrote out, so now the resulting index would 
still show that file as not-up-to-date any more.

But with commit 34110cd4e394e3f92c01a4709689b384c34645d8 ("Make 
'unpack_trees()' have a separate source and destination index"), this 
logic no longer triggers, because we now write out the "result" index, and 
that one never got its timestamp updated from the source index, so it had 
lost all that "is_racy_timestamp()" information!

This trivial patch fixes it. It looks trivial, and it's a simple fix, but 
boy did it take me way too much thinking and explaining to myself to 
explain why there was a problem in the first place!

The trivial fix is to just copy the index timestamp from the source index 
into the result index. But we only do this if we *have* a source index, of 
course, and if we will even bother to use the result.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 unpack-trees.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 91649f3..77d52db 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -336,6 +336,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	state.refresh_cache = 1;
 
 	memset(&o->result, 0, sizeof(o->result));
+	if (o->src_index && o->dst_index)
+		o->result.timestamp = o->src_index->timestamp;
 	o->merge_size = len;
 
 	if (!dfc)

Re: Fix recent 'unpack_trees()'-related changes breaking 'git stash'

[Linus Torvalds]

On Fri, 14 Mar 2008, Linus Torvalds wrote:
> 
> The trivial fix is to just copy the index timestamp from the source index 
> into the result index. But we only do this if we *have* a source index, of 
> course, and if we will even bother to use the result.

Actually, that second part of the test is just unnecessarily clever, and 
it's just asking for trouble.

Even if we never use the "result" for anything in the end, it's probably a 
good idea to have its timestamp match the source timestamp just in case 
somebody wants to do the "is_racy_timestamp()" on the result while it's 
being generated (and before it is thrown away).

In particular, it would not be necessarily wrong to use ie_match_stat() on 
the result index in a callback.

So it might be better to make that thing be just

>  	memset(&o->result, 0, sizeof(o->result));
> +	if (o->src_index)
> +		o->result.timestamp = o->src_index->timestamp;
>  	o->merge_size = len;

instead of checking both src_index *and* dst_index. The source index is 
all that matters anyway. Even if 'o->result' isn't used in the end, who 
cares? We can still give it the right timestamp.

And no, this really isn't likely to matter, but let's pick the simpler 
version if it doesn't matter.

		Linus

Re: Fix recent 'unpack_trees()'-related changes breaking 'git stash'

[Junio C Hamano]

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

> Damn, this series has had more subtle issues than I ever expected.
>
> 'git stash' creates its saved working tree object with:
>
>         # state of the working tree
>         w_tree=$( (
>                 rm -f "$TMP-index" &&
>                 cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
>                 GIT_INDEX_FILE="$TMP-index" &&
>                 export GIT_INDEX_FILE &&
>                 git read-tree -m $i_tree &&
>                 git add -u &&
>                 git write-tree &&
>                 rm -f "$TMP-index"
>         ) ) ||
>                 die "Cannot save the current worktree state"
>
> which creates a new index file with the updates, and writes the tree from 
> that.

It would be slightly simpler to write the above sequence like this:

	w_tree=$( (
		rm -f "$TMP-index" &&
                git read-tree --index-output="$TMP-index" -m $i_tree &&
                GIT_INDEX_FILE="$TMP-index" &&
                export GIT_INDEX_FILE &&
                git add -u &&
                git write-tree &&
                rm -f "$TMP-index"
	) )

I think your fix would apply equally well if we rewrite stash to work like
this.

Re: Fix recent 'unpack_trees()'-related changes breaking 'git stash'

[Szeder Gábor]

On Fri, Mar 14, 2008 at 09:40:02PM -0700, Linus Torvalds wrote:
> So it might be better to make that thing be just
> 
> >  	memset(&o->result, 0, sizeof(o->result));
> > +	if (o->src_index)
> > +		o->result.timestamp = o->src_index->timestamp;
> >  	o->merge_size = len;
> 
> instead of checking both src_index *and* dst_index. The source index is 
> all that matters anyway. Even if 'o->result' isn't used in the end, who 
> cares? We can still give it the right timestamp.
> 
> And no, this really isn't likely to matter, but let's pick the simpler 
> version if it doesn't matter.
I have applied the above patch and run both my stripped-down testcase
and t3903-stash.sh a couple thousand times without a single failure.


Best,
Gábor

Re: Fix recent 'unpack_trees()'-related changes breaking 'git stash'

[Szeder Gábor]

On Fri, Mar 14, 2008 at 09:54:45PM -0700, Junio C Hamano wrote:
> It would be slightly simpler to write the above sequence like this:
> 
> 	w_tree=$( (
> 		rm -f "$TMP-index" &&
>                 git read-tree --index-output="$TMP-index" -m $i_tree &&
>                 GIT_INDEX_FILE="$TMP-index" &&
>                 export GIT_INDEX_FILE &&
>                 git add -u &&
>                 git write-tree &&
>                 rm -f "$TMP-index"
> 	) )
> 
> I think your fix would apply equally well if we rewrite stash to work like
> this.
Yes, with the above changes but without Linus' patch the bug is still
present.


Best,
Gábor

Re: Fix recent 'unpack_trees()'-related changes breaking 'git stash'

[Linus Torvalds]

On Fri, 14 Mar 2008, Junio C Hamano wrote:
> 
> It would be slightly simpler to write the above sequence like this:
> 
> 	w_tree=$( (
> 		rm -f "$TMP-index" &&
>                 git read-tree --index-output="$TMP-index" -m $i_tree &&

Ack. That's an independent cleanup.

In fact, I would almost prefer to try to stop using GIT_INDEX_FILE 
entirely, and add it as a top-level git flag, so you can then make the 
rest be:

	git --index-file "$TMP-index" add -u &&
	git --index-file "$TMP-index" write-tree &&
	rm -f "$TMP-index"

instead of doing that

>                 GIT_INDEX_FILE="$TMP-index" &&
>                 export GIT_INDEX_FILE &&
>                 git add -u &&
>                 git write-tree &&
>                 rm -f "$TMP-index"

thing.


Something like the appended, in other words.

Oh, and that whole git.c argument parsing should be made to use the proper 
arg parser, too. But I'm too damn lazy and not comfy enough with 
'parse_options()' usage. Somebody who is should take a look..

		Linus

---
 git-stash.sh |    9 +++------
 git.c        |   15 +++++++++++++++
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index c2b6820..95b65dc 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -63,12 +63,9 @@ create_stash () {
 	# state of the working tree
 	w_tree=$( (
 		rm -f "$TMP-index" &&
-		cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
-		GIT_INDEX_FILE="$TMP-index" &&
-		export GIT_INDEX_FILE &&
-		git read-tree -m $i_tree &&
-		git add -u &&
-		git write-tree &&
+		git read-tree --index-output="$TMP-index" -m $i_tree &&
+		git --index-file "$TMP-index" add -u &&
+		git --index-file "$TMP-index" write-tree &&
 		rm -f "$TMP-index"
 	) ) ||
 		die "Cannot save the current worktree state"
diff --git a/git.c b/git.c
index 13de801..a615df9 100644
--- a/git.c
+++ b/git.c
@@ -55,6 +55,21 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 			setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);
 			if (envchanged)
 				*envchanged = 1;
+		} else if (!strcmp(cmd, "--index-file")) {
+			if (*argc < 2) {
+				fprintf(stderr, "No directory given for --index-file.\n" );
+				usage(git_usage_string);
+			}
+			setenv(INDEX_ENVIRONMENT, (*argv)[1], 1);
+			if (envchanged)
+				*envchanged = 1;
+			(*argv)++;
+			(*argc)--;
+			handled++;
+		} else if (!prefixcmp(cmd, "--index-file=")) {
+			setenv(INDEX_ENVIRONMENT, cmd + 13, 1);
+			if (envchanged)
+				*envchanged = 1;
 		} else if (!strcmp(cmd, "--work-tree")) {
 			if (*argc < 2) {
 				fprintf(stderr, "No directory given for --work-tree.\n" );

[PATCH] git-stash: use git-read-tree --index-output option

[しらいしななこ]

Instead of copying the original index with "cp -p" to preserve timestamp, use "--index-output" option of git-read-tree program.

Signed-off-by: Nanako Shiraishi <nanako3@bluebottle.com>
---

 git-stash.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index c2b6820..ca49c5e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -63,10 +63,9 @@ create_stash () {
 	# state of the working tree
 	w_tree=$( (
 		rm -f "$TMP-index" &&
-		cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
+		git read-tree --index-output="$TMP-index" -m $i_tree &&
 		GIT_INDEX_FILE="$TMP-index" &&
 		export GIT_INDEX_FILE &&
-		git read-tree -m $i_tree &&
 		git add -u &&
 		git write-tree &&
 		rm -f "$TMP-index"

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

----------------------------------------------------------------------
Finally - A spam blocker that actually works.
http://www.bluebottle.com/tag/4