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

From: Jeff King <peff@peff.net>
Date: Oct 23, 2016, 09:02 UTC
Message-ID: <20161023090207.sg52l4mijqtfvip6@sigill.intra.peff.net>
In-Reply-To: <1473378397-22453-1-git-send-email-watsona4@gmail.com>

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

Show 12 quoted lines

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

Show 10 quoted lines

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

Show 17 quoted lines

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

Show 13 quoted lines

> 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