[v3] stash: make sure we have a valid index before writing it

Commit Message

Thomas Gummerer Nov. 13, 2019, 3:01 p.m. UTC

On 11/13, Junio C Hamano wrote:
> Thomas Gummerer <[e-mail]> writes:
> 
> >> ...  This may want to become
> >> 
> >> 	git rev-parse --verify :file &&
> >> 
> >> or
> >> 
> >> 	git show :file >actual && echo bar >expect &&
> >> 	test_cmp expect actual &&
> >> 
> >> perhaps?
> >
> > Hmm I just copy-pasted this from somewhere else in this test file.
> > I'll add a preparatory patch getting rid of "$(git command substitution)"
> > as I don't believe Denton got to t3903 yet.
> >
> > There's some more opportunities for modernization of this test file,
> > but I refrained from doing that to not blow up this bug fix series too
> > much.
> 
> It is very much appreciated that you aimed to keep the topic focused
> on the fixing.  What I meant was merely to avoid making things worse
> by adding more of $(git command substitution), not cleaning up the
> existing ones.

I misunderstood then because the other case you had pointed out wasn't
introduced in my patch, but was just in the context. I have already
sent the series with the preparatory cleanup.  I'm happy to just go
without the cleanup though.  Since I'm already sending this email,
I'll just add the patch doing just that below.

--- >8 ---
Subject: [PATCH v3] stash: make sure we have a valid index before writing it

In 'do_apply_stash()' we refresh the index in the end.  Since
34933d0eff ("stash: make sure to write refreshed cache", 2019-09-11),
we also write that refreshed index when --quiet is given to 'git stash
apply'.

However if '--index' is not given to 'git stash apply', we also
discard the index in the else clause just before.  We need to do so
because we use an external 'git update-index --add --stdin', which
leads to an out of date in-core index.

Later we call 'refresh_and_write_cache', which now leads to writing
the discarded index, which means we essentially write an empty index
file.  This is obviously not correct, or the behaviour the user
wanted.  We should not modify the users index without being asked to
do so.

Make sure to re-read the index after discarding the current in-core
index, to avoid dealing with outdated information.  Instead we could
also drop the 'discard_cache()' + 'read_cache()', however that would
make it easy to fall into the same trap as 34933d0eff did, so it's
better to avoid that.

We can also drop the 'refresh_and_write_cache' completely in the quiet
case.  Previously in legacy stash we relied on 'git status' to refresh
the index after calling 'git read-tree' when '--index' was passed to
'git apply'.  However the 'reset_tree()' call that replaced 'git
read-tree' always passes options that are equivalent to '-m', making
the refresh of the index unnecessary.

Reported-by: Grzegorz Rajchman <[e-mail]>
Signed-off-by: Thomas Gummerer <[e-mail]>
---
 builtin/stash.c  | 7 +++----
 t/t3903-stash.sh | 7 ++++++-
 2 files changed, 9 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Nov. 14, 2019, 2:07 a.m. UTC | #1

Thomas Gummerer <[e-mail]> writes:

> Subject: [PATCH v3] stash: make sure we have a valid index before writing it
>
> In 'do_apply_stash()' we refresh the index in the end.  Since
> 34933d0eff ("stash: make sure to write refreshed cache", 2019-09-11),
> we also write that refreshed index when --quiet is given to 'git stash
> apply'.
>
> However if '--index' is not given to 'git stash apply', we also
> discard the index in the else clause just before.  We need to do so
> because we use an external 'git update-index --add --stdin', which
> leads to an out of date in-core index.
>
> Later we call 'refresh_and_write_cache', which now leads to writing
> the discarded index, which means we essentially write an empty index
> file.  This is obviously not correct, or the behaviour the user
> wanted.  We should not modify the users index without being asked to
> do so.
>
> Make sure to re-read the index after discarding the current in-core
> index, to avoid dealing with outdated information.  Instead we could
> also drop the 'discard_cache()' + 'read_cache()', however that would
> make it easy to fall into the same trap as 34933d0eff did, so it's
> better to avoid that.
>
> We can also drop the 'refresh_and_write_cache' completely in the quiet
> case.  Previously in legacy stash we relied on 'git status' to refresh
> the index after calling 'git read-tree' when '--index' was passed to
> 'git apply'.  However the 'reset_tree()' call that replaced 'git
> read-tree' always passes options that are equivalent to '-m', making
> the refresh of the index unnecessary.
>
> Reported-by: Grzegorz Rajchman <[e-mail]>
> Signed-off-by: Thomas Gummerer <[e-mail]>
> ---

Thanks.  This looks good and minimal ;-)

>  builtin/stash.c  | 7 +++----
>  t/t3903-stash.sh | 7 ++++++-
>  2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index ab30d1e920..372fbdb7ac 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -481,13 +481,12 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
>  		if (ret)
>  			return -1;
>  
> +		/* read back the result of update_index() back from the disk */
>  		discard_cache();
> +		read_cache();
>  	}
>  
> -	if (quiet) {
> -		if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
> -			warning("could not refresh index");
> -	} else {
> +	if (!quiet) {
>  		struct child_process cp = CHILD_PROCESS_INIT;
>  
>  		/*
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 392954d6dd..9de1c3616a 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -232,8 +232,11 @@ test_expect_success 'save -q is quiet' '
>  	test_must_be_empty output.out
>  '
>  
> -test_expect_success 'pop -q is quiet' '
> +test_expect_success 'pop -q works and is quiet' '
>  	git stash pop -q >output.out 2>&1 &&
> +	echo bar >expect &&
> +	git show :file >actual &&
> +	test_cmp expect actual &&
>  	test_must_be_empty output.out
>  '
>  
> @@ -242,6 +245,8 @@ test_expect_success 'pop -q --index works and is quiet' '
>  	git add file &&
>  	git stash save --quiet &&
>  	git stash pop -q --index >output.out 2>&1 &&
> +	git diff-files file2 >file2.diff &&
> +	test_must_be_empty file2.diff &&
>  	test foo = "$(git show :file)" &&
>  	test_must_be_empty output.out
>  '

Patch

diff --git a/builtin/stash.c b/builtin/stash.c
index ab30d1e920..372fbdb7ac 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -481,13 +481,12 @@  static int do_apply_stash(const char *prefix, struct stash_info *info,
 		if (ret)
 			return -1;
 
+		/* read back the result of update_index() back from the disk */
 		discard_cache();
+		read_cache();
 	}
 
-	if (quiet) {
-		if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
-			warning("could not refresh index");
-	} else {
+	if (!quiet) {
 		struct child_process cp = CHILD_PROCESS_INIT;
 
 		/*
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 392954d6dd..9de1c3616a 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -232,8 +232,11 @@  test_expect_success 'save -q is quiet' '
 	test_must_be_empty output.out
 '
 
-test_expect_success 'pop -q is quiet' '
+test_expect_success 'pop -q works and is quiet' '
 	git stash pop -q >output.out 2>&1 &&
+	echo bar >expect &&
+	git show :file >actual &&
+	test_cmp expect actual &&
 	test_must_be_empty output.out
 '
 
@@ -242,6 +245,8 @@  test_expect_success 'pop -q --index works and is quiet' '
 	git add file &&
 	git stash save --quiet &&
 	git stash pop -q --index >output.out 2>&1 &&
+	git diff-files file2 >file2.diff &&
+	test_must_be_empty file2.diff &&
 	test foo = "$(git show :file)" &&
 	test_must_be_empty output.out
 '