[PATCH] Allow stashes to be referenced by index only

[PATCH] Allow stashes to be referenced by index only

[Aaron M Watson]

Instead of referencing "stash@{n}" explicitly, it can simply be
referenced as "n".  Most users only reference stashes by their position
in the stash stask (what I refer to as the "index"). The syntax for the
typical stash (stash@{n}) is slightly annoying and easy to forget, and
sometimes difficult to escape properly in a script. Because of this the
capability to do things with the stash by simply referencing the index
is desirable.

This patch includes the superior implementation provided by Øsse Walle
(thanks for that), with a slight change to fix a broken test in the test
suite. I also merged the test scripts as suggested by Jeff King, and
un-wrapped the documentation as suggested by Junio Hamano.

Signed-off-by: Aaron M Watson <watsona4@gmail.com>
---
 Documentation/git-stash.txt |  3 ++-
 git-stash.sh                | 17 +++++++++++++++--
 t/t3903-stash.sh            | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 92df596..2e9cef0 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -39,7 +39,8 @@ The latest stash you created is stored in `refs/stash`; older
 stashes are found in the reflog of this reference and can be named using
 the usual reflog syntax (e.g. `stash@{0}` is the most recently
 created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`
-is also possible).
+is also possible). Stashes may also be referenced by specifying just the
+stash index (e.g. the integer `n` is equivalent to `stash@{n}`).
 
 OPTIONS
 -------
diff --git a/git-stash.sh b/git-stash.sh
index 826af18..d8d3b8d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -384,9 +384,10 @@ parse_flags_and_rev()
 	i_tree=
 	u_tree=
 
-	REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1
+	REV=$(git rev-parse --no-flags --symbolic --sq "$@" 2> /dev/null)
 
 	FLAGS=
+	ARGV=
 	for opt
 	do
 		case "$opt" in
@@ -404,10 +405,13 @@ parse_flags_and_rev()
 					die "$(eval_gettext "unknown option: \$opt")"
 				FLAGS="${FLAGS}${FLAGS:+ }$opt"
 			;;
+			*)
+				ARGV="${ARGV}${ARGV:+ }'$opt'"
+			;;
 		esac
 	done
 
-	eval set -- $REV
+	eval set -- $ARGV
 
 	case $# in
 		0)
@@ -422,6 +426,15 @@ parse_flags_and_rev()
 		;;
 	esac
 
+	case "$1" in
+		*[!0-9]*)
+			:
+		;;
+		*)
+			set -- "${ref_stash}@{$1}"
+		;;
+	esac
+
 	REV=$(git rev-parse --symbolic --verify --quiet "$1") || {
 		reference="$1"
 		die "$(eval_gettext "\$reference is not a valid reference")"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2142c1f..f82a8c4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -131,6 +131,26 @@ test_expect_success 'drop middle stash' '
 	test 1 = $(git show HEAD:file)
 '
 
+test_expect_success 'drop middle stash by index' '
+	git reset --hard &&
+	echo 8 > file &&
+	git stash &&
+	echo 9 > file &&
+	git stash &&
+	git stash drop 1 &&
+	test 2 = $(git stash list | wc -l) &&
+	git stash apply &&
+	test 9 = $(cat file) &&
+	test 1 = $(git show :file) &&
+	test 1 = $(git show HEAD:file) &&
+	git reset --hard &&
+	git stash drop &&
+	git stash apply &&
+	test 3 = $(cat file) &&
+	test 1 = $(git show :file) &&
+	test 1 = $(git show HEAD:file)
+'
+
 test_expect_success 'stash pop' '
 	git reset --hard &&
 	git stash pop &&
@@ -604,6 +624,21 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
 	git stash drop
 '
 
+test_expect_success 'invalid ref of the form "n", n >= N' '
+	git stash clear &&
+	test_must_fail git stash drop 0 &&
+	echo bar5 > file &&
+	echo bar6 > file2 &&
+	git add file2 &&
+	git stash &&
+	test_must_fail git stash drop 1 &&
+	test_must_fail git stash pop 1 &&
+	test_must_fail git stash apply 1 &&
+	test_must_fail git stash show 1 &&
+	test_must_fail git stash branch tmp 1 &&
+	git stash drop
+'
+
 test_expect_success 'stash branch should not drop the stash if the branch exists' '
 	git stash clear &&
 	echo foo >file &&
-- 
2.7.4

Re: [PATCH] Allow stashes to be referenced by index only

[Jeff King]

On Thu, Sep 08, 2016 at 07:46:37PM -0400, Aaron M Watson wrote:

> Instead of referencing "stash@{n}" explicitly, it can simply be
> referenced as "n".  Most users only reference stashes by their position
> in the stash stask (what I refer to as the "index"). The syntax for the
> typical stash (stash@{n}) is slightly annoying and easy to forget, and
> sometimes difficult to escape properly in a script. Because of this the
> capability to do things with the stash by simply referencing the index
> is desirable.
> 
> This patch includes the superior implementation provided by Øsse Walle
> (thanks for that), with a slight change to fix a broken test in the test
> suite. I also merged the test scripts as suggested by Jeff King, and
> un-wrapped the documentation as suggested by Junio Hamano.

Just cleaning out my list of leftover bits, and it looks like this patch
fell through the cracks.

The last thing before this was generally people agreeing with the
approach that Øsse showed:

  http://public-inbox.org/git/CAFaJEqu-JUcwLjrQBk_huSa3DZfCf8O4eAZ=UgcXHzN=CLgtpw@mail.gmail.com/

  http://public-inbox.org/git/xmqqbmzzoazy.fsf@gitster.mtv.corp.google.com/

and we just needed to roll that up into a patch. Which this _mostly_ is,
but there are a few funny things still:

> diff --git a/git-stash.sh b/git-stash.sh
> index 826af18..d8d3b8d 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -384,9 +384,10 @@ parse_flags_and_rev()
>  	i_tree=
>  	u_tree=
>  
> -	REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1
> +	REV=$(git rev-parse --no-flags --symbolic --sq "$@" 2> /dev/null)

Style: we don't put a space between ">" and the filename.

So here we assign REV, and we no longer exit (to handle something like
"1" by itself, which is good).

>  	FLAGS=
> +	ARGV=
>  	for opt
>  	do
>  		case "$opt" in
> @@ -404,10 +405,13 @@ parse_flags_and_rev()
>  					die "$(eval_gettext "unknown option: \$opt")"
>  				FLAGS="${FLAGS}${FLAGS:+ }$opt"
>  			;;
> +			*)
> +				ARGV="${ARGV}${ARGV:+ }'$opt'"
> +			;;
>  		esac
>  	done
>  
> -	eval set -- $REV
> +	eval set -- $ARGV

But what's going on here? Why did we bother running rev-parse earlier if
we don't actually use the value of REV?

You mentioned tweaking it to fix a broken test, and indeed, just using
$REV here breaks a few tests in t3903.

Offhand, I do not see anything wrong with pulling the non-option values
out in the loop. But in that case I think the assignment of REV can just
go away completely.

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 2142c1f..f82a8c4 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -131,6 +131,26 @@ test_expect_success 'drop middle stash' '
>  	test 1 = $(git show HEAD:file)
>  '
>  
> +test_expect_success 'drop middle stash by index' '
> +	git reset --hard &&
> +	echo 8 > file &&
> +	git stash &&
> +	echo 9 > file &&

Ditto on the ">" style here and elsewhere.

The tests otherwise look good to me.

-Peff

Re: [PATCH] Allow stashes to be referenced by index only

[Jeff King]

On Sun, Oct 23, 2016 at 01:41:25PM -0400, Aaron and Ashley Watson wrote:

> > But what's going on here? Why did we bother running rev-parse earlier if
> > we don't actually use the value of REV?
> >
> > You mentioned tweaking it to fix a broken test, and indeed, just using
> > $REV here breaks a few tests in t3903.
> >
> > Offhand, I do not see anything wrong with pulling the non-option values
> > out in the loop. But in that case I think the assignment of REV can just
> > go away completely.
> >
> 
> The only reason for REV to remain is to preserve the error message seen with
> the previous behavior. Perhaps it would be better to instead move the
> assignment
> of REV to the only place it is still used: the error message when multiple
> arguments were detected.

Ah, thanks, I missed that use. We suppress stderr, so we're literally
just getting the set of revs there. But that should match what we have
in ARGV anyway (after all, $ARGV is where we decided we had too many
revs, and what we'll feed to rev-parse to get the sha1).

So I wonder if:

  Too many revisions specified: $ARGV

would be more appropriate (at which point you can probably just continue
to call it $REV).

> I'm not sure of the next steps in the process of submitting a patch.
> Should I submit a new patch by replying to this email, or is using git
> send-email to create a new mail thread better?

Generally re-rolls of a patch are done as replies to the original. Gmail
is bad about corrupting whitespace, though, so you can't just reply and
paste the patch in there. You can use ask git-send-email to continue the
thread, though:

  mid=1473378397-22453-1-git-send-email-watsona4@gmail.com
  git send-email --in-reply-to=$mid ...other options...

You might also want to cc the people involved in the earlier discussion.
The public-inbox archive gives a customized full send-email command for
each message to make it easy.

  http://public-inbox.org/git/1473378397-22453-1-git-send-email-watsona4@gmail.com/

-Peff

[PATCH] Allow stashes to be referenced by index only

[Aaron M Watson]

Instead of referencing "stash@{n}" explicitly, it can simply be
referenced as "n".  Most users only reference stashes by their position
in the stash stask (what I refer to as the "index"). The syntax for the
typical stash (stash@{n}) is slightly annoying and easy to forget, and
sometimes difficult to escape properly in a script. Because of this the
capability to do things with the stash by simply referencing the index
is desirable.

This patch includes the superior implementation provided by Øsse Walle
(thanks for that), with a slight change to fix a broken test in the test
suite. I also merged the test scripts as suggested by Jeff King, and
un-wrapped the documentation as suggested by Junio Hamano.

Signed-off-by: Aaron M Watson <watsona4@gmail.com>
---
 Documentation/git-stash.txt |  3 ++-
 git-stash.sh                | 15 +++++++++++++--
 t/t3903-stash.sh            | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 92df596..2e9cef0 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -39,7 +39,8 @@ The latest stash you created is stored in `refs/stash`; older
 stashes are found in the reflog of this reference and can be named using
 the usual reflog syntax (e.g. `stash@{0}` is the most recently
 created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`
-is also possible).
+is also possible). Stashes may also be referenced by specifying just the
+stash index (e.g. the integer `n` is equivalent to `stash@{n}`).
 
 OPTIONS
 -------
diff --git a/git-stash.sh b/git-stash.sh
index 826af18..d7072c8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -384,9 +384,8 @@ parse_flags_and_rev()
 	i_tree=
 	u_tree=
 
-	REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1
-
 	FLAGS=
+	REV=
 	for opt
 	do
 		case "$opt" in
@@ -404,6 +403,9 @@ parse_flags_and_rev()
 					die "$(eval_gettext "unknown option: \$opt")"
 				FLAGS="${FLAGS}${FLAGS:+ }$opt"
 			;;
+			*)
+				REV="${REV}${REV:+ }'$opt'"
+			;;
 		esac
 	done
 
@@ -422,6 +424,15 @@ parse_flags_and_rev()
 		;;
 	esac
 
+	case "$1" in
+		*[!0-9]*)
+			:
+		;;
+		*)
+			set -- "${ref_stash}@{$1}"
+		;;
+	esac
+
 	REV=$(git rev-parse --symbolic --verify --quiet "$1") || {
 		reference="$1"
 		die "$(eval_gettext "\$reference is not a valid reference")"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2142c1f..f82a8c4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -131,6 +131,26 @@ test_expect_success 'drop middle stash' '
 	test 1 = $(git show HEAD:file)
 '
 
+test_expect_success 'drop middle stash by index' '
+	git reset --hard &&
+	echo 8 >file &&
+	git stash &&
+	echo 9 >file &&
+	git stash &&
+	git stash drop 1 &&
+	test 2 = $(git stash list | wc -l) &&
+	git stash apply &&
+	test 9 = $(cat file) &&
+	test 1 = $(git show :file) &&
+	test 1 = $(git show HEAD:file) &&
+	git reset --hard &&
+	git stash drop &&
+	git stash apply &&
+	test 3 = $(cat file) &&
+	test 1 = $(git show :file) &&
+	test 1 = $(git show HEAD:file)
+'
+
 test_expect_success 'stash pop' '
 	git reset --hard &&
 	git stash pop &&
@@ -604,7 +624,21 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
 	git stash drop
 '
 
+test_expect_success 'invalid ref of the form "n", n >= N' '
+	git stash clear &&
+	test_must_fail git stash drop 0 &&
+	echo bar5 >file &&
+	echo bar6 >file2 &&
+	git add file2 &&
+	git stash &&
+	test_must_fail git stash drop 1 &&
+	test_must_fail git stash pop 1 &&
+	test_must_fail git stash apply 1 &&
+	test_must_fail git stash show 1 &&
+	test_must_fail git stash branch tmp 1 &&
+	git stash drop
+'
+
 test_expect_success 'stash branch should not drop the stash if the branch exists' '
 	git stash clear &&
 	echo foo >file &&
-- 
2.7.4

Re: [PATCH] Allow stashes to be referenced by index only

[Jeff King]

On Mon, Oct 24, 2016 at 07:40:13PM -0400, Aaron M Watson wrote:

> Instead of referencing "stash@{n}" explicitly, it can simply be
> referenced as "n".  Most users only reference stashes by their position
> in the stash stask (what I refer to as the "index"). The syntax for the
> typical stash (stash@{n}) is slightly annoying and easy to forget, and
> sometimes difficult to escape properly in a script. Because of this the
> capability to do things with the stash by simply referencing the index
> is desirable.
> 
> This patch includes the superior implementation provided by Øsse Walle
> (thanks for that), with a slight change to fix a broken test in the test
> suite. I also merged the test scripts as suggested by Jeff King, and
> un-wrapped the documentation as suggested by Junio Hamano.
> 
> Signed-off-by: Aaron M Watson <watsona4@gmail.com>
> ---

Thanks, this version looks good to me.

Oddly, it does not seem to apply. I get:

  $ git am -3 ~/patch
  Applying: Allow stashes to be referenced by index only
  Using index info to reconstruct a base tree...
  M       git-stash.sh
  error: patch failed: t/t3903-stash.sh:604
  error: t/t3903-stash.sh: patch does not apply
  error: Did you hand edit your patch?
  It does not apply to blobs recorded in its index.
  Patch failed at 0001 Allow stashes to be referenced by index only

The culprit seems to be the final hunk header:

> @@ -604,7 +624,21 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '

This should be "604,6", as there are 6 context lines, and your patch
does not remove any lines.

I suspect the maintainer can fix it up while applying, but for my
curiosity: did you hand-edit it, or is there a potential bug in git's
diff code?

-Peff

Re: [PATCH] Allow stashes to be referenced by index only

[Aaron and Ashley Watson]

On Tue, Oct 25, 2016 at 4:11 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 24, 2016 at 07:40:13PM -0400, Aaron M Watson wrote:
>
>> Instead of referencing "stash@{n}" explicitly, it can simply be
>> referenced as "n".  Most users only reference stashes by their position
>> in the stash stask (what I refer to as the "index"). The syntax for the
>> typical stash (stash@{n}) is slightly annoying and easy to forget, and
>> sometimes difficult to escape properly in a script. Because of this the
>> capability to do things with the stash by simply referencing the index
>> is desirable.
>>
>> This patch includes the superior implementation provided by Øsse Walle
>> (thanks for that), with a slight change to fix a broken test in the test
>> suite. I also merged the test scripts as suggested by Jeff King, and
>> un-wrapped the documentation as suggested by Junio Hamano.
>>
>> Signed-off-by: Aaron M Watson <watsona4@gmail.com>
>> ---
>
> Thanks, this version looks good to me.
>
> Oddly, it does not seem to apply. I get:
>
>   $ git am -3 ~/patch
>   Applying: Allow stashes to be referenced by index only
>   Using index info to reconstruct a base tree...
>   M       git-stash.sh
>   error: patch failed: t/t3903-stash.sh:604
>   error: t/t3903-stash.sh: patch does not apply
>   error: Did you hand edit your patch?
>   It does not apply to blobs recorded in its index.
>   Patch failed at 0001 Allow stashes to be referenced by index only
>
> The culprit seems to be the final hunk header:
>
>> @@ -604,7 +624,21 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
>
> This should be "604,6", as there are 6 context lines, and your patch
> does not remove any lines.
>
> I suspect the maintainer can fix it up while applying, but for my
> curiosity: did you hand-edit it, or is there a potential bug in git's
> diff code?

I did indeed edit the patch by hand (I forgot to remove the spaces
after the > in the test file), but
the bug appears to be in emacs's diff-mode, not in git.

>
> -Peff



-- 
Aaron and Ashley Watson

Re: [PATCH] Allow stashes to be referenced by index only

[Junio C Hamano]

Aaron M Watson <watsona4@gmail.com> writes:

Aaron M Watson <watsona4@gmail.com> writes:

> Instead of referencing "stash@{n}" explicitly, it can simply be
> referenced as "n".
> Most users only reference stashes by their position
> in the stash stask (what I refer to as the "index").

It is unclear if the first sentence is a statement of the fact, an
expression of desire, or something else.  With the current codebase,
it cannot simply be referenced as "n", and you either "wish it were
possible", or "make it possible to do so", or perhaps little bit of
both.

This is why we tend to use imperative mood to give an order to the
codebase to "be like so" to make it clear.

Perhaps

  Instead of referencing "stash@{n}" explicitly, make it possible to
  simply reference as "n".  Most users only reference stashes by their
  position in the stash stask (what I refer to as the "index" here).

or something like that (which is what I tenatively rewritten this to
while queuing).

> @@ -404,6 +403,9 @@ parse_flags_and_rev()
>  					die "$(eval_gettext "unknown option: \$opt")"
>  				FLAGS="${FLAGS}${FLAGS:+ }$opt"
>  			;;
> +			*)
> +				REV="${REV}${REV:+ }'$opt'"
> +			;;
>  		esac
>  	done
>  
> @@ -422,6 +424,15 @@ parse_flags_and_rev()
>  		;;
>  	esac
>  
> +	case "$1" in
> +		*[!0-9]*)
> +			:
> +		;;
> +		*)
> +			set -- "${ref_stash}@{$1}"
> +		;;
> +	esac

I can see that you inherited the brokenness from an existing one in
the earlier hunk, but case arms in these two case statements are
indented one level too deep.  It would be good to fix it in a
follow-up patch (not a reroll of this patch).

Thanks.  Will queue.

Re: [PATCH] Allow stashes to be referenced by index only

[Ramsay Jones]

On 25/10/16 22:41, Junio C Hamano wrote:
> Aaron M Watson <watsona4@gmail.com> writes:
> 
> Aaron M Watson <watsona4@gmail.com> writes:
> 
>> Instead of referencing "stash@{n}" explicitly, it can simply be
>> referenced as "n".
>> Most users only reference stashes by their position
>> in the stash stask (what I refer to as the "index").
> 
> It is unclear if the first sentence is a statement of the fact, an
> expression of desire, or something else.  With the current codebase,
> it cannot simply be referenced as "n", and you either "wish it were
> possible", or "make it possible to do so", or perhaps little bit of
> both.
> 
> This is why we tend to use imperative mood to give an order to the
> codebase to "be like so" to make it clear.
> 
> Perhaps
> 
>   Instead of referencing "stash@{n}" explicitly, make it possible to
>   simply reference as "n".  Most users only reference stashes by their
>   position in the stash stask (what I refer to as the "index" here).

s/stask/stack/

ATB,
Ramsay Jones