diff mbox series

stash: fix show referencing stash index

Message ID 20190615112618.GC11340@hank.intra.tgummerer.com (mailing list archive)
State New, archived
Headers show
Series stash: fix show referencing stash index | expand

Commit Message

Thomas Gummerer June 15, 2019, 11:26 a.m. UTC 
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?

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(-)

Comments

Andrei Rybak June 15, 2019, 1:02 p.m. UTC | #1 
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.
Thomas Gummerer June 17, 2019, 6:03 a.m. UTC | #2 
On 06/15, Andrei Rybak wrote:
> 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 June 24, 2019, 8:29 p.m. UTC | #3 
On Sat, Jun 15, 2019 at 12:26:18PM +0100, Thomas Gummerer wrote:
> 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
diff mbox series

Patch

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 &&