`git stash <command> <n>` stopped working in 2.22.0

Mike Hommey· Jun 14, 2019, 07:42 UTC · lore

Hi,

`git stash <command> <n>` where n is a number used to work until 2.21.*. It doesn't work in 2.22.0.

Bisection points to:

dc7bd382b1063303f4f45d243bff371899285acb is the first bad commit
commit dc7bd382b1063303f4f45d243bff371899285acb
Author: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Date:   Mon Feb 25 23:16:20 2019 +0000

    stash: convert show to builtin

which I guess makes sense :)

Mike

Thomas Gummerer· Jun 15, 2019, 11:26 UTC · re: Mike Hommey · lore

[PATCH] stash: fix show referencing stash index

On 06/14, Mike Hommey wrote:

Show 15 quoted lines

> Hi,
> 
> `git stash <command> <n>` where n is a number used to work until 2.21.*.
> It doesn't work in 2.22.0.
> 
> Bisection points to:
> 
> dc7bd382b1063303f4f45d243bff371899285acb is the first bad commit
> commit dc7bd382b1063303f4f45d243bff371899285acb
> Author: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
> Date:   Mon Feb 25 23:16:20 2019 +0000
> 
>     stash: convert show to builtin
> 
> which I guess makes sense :)

Yup, this is definitely a bug. I think it only affected 'git stash show' however, and not other stash subcommands. If not, could you point me to where else you saw this bug?

Below is a patch that should fix it.

--- >8 ---
Subject: [PATCH] stash: fix show referencing stash index

In the conversion of 'stash show' to C in dc7bd382b1 ("stash: convert show to builtin", 2019-02-25), 'git stash show <n>', where n is the index of a stash got broken, if n is not a file or a valid revision by itself.

'stash show' accepts any flag 'git diff' accepts for changing the output format. Internally we use 'setup_revisions()' to parse these command line flags. Currently we pass the whole argv through to 'setup_revisions()', which includes the stash index.

As the stash index is not a valid revision or a file in the working tree in most cases however, this 'setup_revisions()' call (and thus the whole command) ends up failing if we use this form of 'git stash show'.

Instead of passing the whole argv to 'setup_revisions()', only pass the flags (and the command name) through, while excluding the stash reference. The stash reference is parsed (and validated) in 'get_stash_info()' already.

This separate parsing also means that we currently do produce the correct output if the command succeeds.

Reported-by: Mike Hommey <mh@glandium.org>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/stash.c  |  9 +++++----
 t/t3903-stash.sh | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 2a8e6d09b4..fde6397caa 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -713,11 +713,11 @@ static int git_stash_config(const char *var, const char *value, void *cb)
 static int show_stash(int argc, const char **argv, const char *prefix)
 {
 	int i;
-	int opts = 0;
 	int ret = 0;
 	struct stash_info info;
 	struct rev_info rev;
 	struct argv_array stash_args = ARGV_ARRAY_INIT;
+	struct argv_array revision_args = ARGV_ARRAY_INIT;
 	struct option options[] = {
 		OPT_END()
 	};
@@ -726,11 +726,12 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 	git_config(git_diff_ui_config, NULL);
 	init_revisions(&rev, prefix);
 
+	argv_array_push(&revision_args, argv[0]);
 	for (i = 1; i < argc; i++) {
 		if (argv[i][0] != '-')
 			argv_array_push(&stash_args, argv[i]);
 		else
-			opts++;
+			argv_array_push(&revision_args, argv[i]);
 	}
 
 	ret = get_stash_info(&info, stash_args.argc, stash_args.argv);
@@ -742,7 +743,7 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 	 * The config settings are applied only if there are not passed
 	 * any options.
 	 */
-	if (!opts) {
+	if (revision_args.argc == 1) {
 		git_config(git_stash_config, NULL);
 		if (show_stat)
 			rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT;
@@ -756,7 +757,7 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	argc = setup_revisions(argc, argv, &rev, NULL);
+	argc = setup_revisions(revision_args.argc, revision_args.argv, &rev, NULL);
 	if (argc > 1) {
 		free_stash_info(&info);
 		usage_with_options(git_stash_show_usage, options);
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ea30d5f6a0..3973cbda0e 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -708,6 +708,24 @@ test_expect_success 'invalid ref of the form "n", n >= N' '
 	git stash drop
 '
 
+test_expect_success 'valid ref of the form "n", n >= N' '
+	git stash clear &&
+	echo bar5 >file &&
+	echo bar6 >file2 &&
+	git add file2 &&
+	git stash &&
+	git stash show 0 &&
+	git stash branch tmp 0 &&
+	git checkout master &&
+	git stash &&
+	git stash apply 0 &&
+	git reset --hard &&
+	git stash pop 0 &&
+	git stash &&
+	git stash drop 0 &&
+	test_must_fail git stash drop
+'
+
 test_expect_success 'branch: do not drop the stash if the branch exists' '
 	git stash clear &&
 	echo foo >file &&

-- 
2.22.0.rc2

Andrei Rybak· Jun 15, 2019, 13:02 UTC · re: Thomas Gummerer · lore

Re: [PATCH] stash: fix show referencing stash index

On 6/15/19 1:26 PM, Thomas Gummerer wrote:

Show 9 quoted lines

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index ea30d5f6a0..3973cbda0e 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -708,6 +708,24 @@ test_expect_success 'invalid ref of the form "n", n >= N' '
>  	git stash drop
>  '
>  
> +test_expect_success 'valid ref of the form "n", n >= N' '

If ref is valid, 'n < N' was probably meant here.

Thomas Gummerer· Jun 17, 2019, 06:03 UTC · re: Andrei Rybak · lore

Re: [PATCH] stash: fix show referencing stash index

On 06/15, Andrei Rybak wrote:

Show 12 quoted lines

> On 6/15/19 1:26 PM, Thomas Gummerer wrote:
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index ea30d5f6a0..3973cbda0e 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -708,6 +708,24 @@ test_expect_success 'invalid ref of the form "n", n >= N' '
> >  	git stash drop
> >  '
> >  
> > +test_expect_success 'valid ref of the form "n", n >= N' '
> 
> If ref is valid, 'n < N' was probably meant here.

Yes, indeed.  Thanks!

Mike Hommey· Jun 24, 2019, 20:29 UTC · re: Thomas Gummerer · lore

Re: [PATCH] stash: fix show referencing stash index

On Sat, Jun 15, 2019 at 12:26:18PM +0100, Thomas Gummerer wrote:

Show 20 quoted lines

> On 06/14, Mike Hommey wrote:
> > Hi,
> > 
> > `git stash <command> <n>` where n is a number used to work until 2.21.*.
> > It doesn't work in 2.22.0.
> > 
> > Bisection points to:
> > 
> > dc7bd382b1063303f4f45d243bff371899285acb is the first bad commit
> > commit dc7bd382b1063303f4f45d243bff371899285acb
> > Author: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
> > Date:   Mon Feb 25 23:16:20 2019 +0000
> > 
> >     stash: convert show to builtin
> > 
> > which I guess makes sense :)
> 
> Yup, this is definitely a bug.  I think it only affected 'git stash
> show' however, and not other stash subcommands.  If not, could you
> point me to where else you saw this bug?

I confirmed pop, apply, branch, and drop are not affected.

Mike