regression: stash show -p (was [PATCH maint] git-stash.sh: don't default to refs/stash if invalid ref supplied)

From: Brandon Casey <casey@nrlssc.navy.mil>
Date: Oct 1, 2008, 19:53 UTC
Message-ID: <DDpCKmlWUqX7hHbNNF45UGo81L7NuUHMbU3vXSfhHU60An5PUqIvRA@cipher.nrlssc.navy.mil>
In-Reply-To: <twy9P1UnbmZHs2ZDeFBjSzrleLQ225-I7ESNrFkwoXZL-mJCTf4hYw@cipher.nrlssc.navy.mil>

Thanks to the patch referenced in the subject, 'git stash show -p' no longer works if a revision is _not_ supplied.

This is because the argument list passed to show_stash() is not empty, so the default $ref_stash@{0} is not appended.

This brings me back to an unanswered question, "Should rev-parse fall back to its --default argument when an invalid ref is supplied?". The documentation does not imply that it should.

>From Documentation/git-rev-parse.txt:

   --default <arg>::
        If there is no parameter given by the user, use `<arg>`
        instead.

Currently a rev-parse invocation like:

    $ git rev-parse --default HEAD a_non_existent_ref

will fall back to operating on HEAD. Is this correct? Could there be some part of git that depends on this behavior? filter-branch is now the only script which uses the --default option of rev-parse, not sure about the c code.

-brandon

Brandon Casey wrote:

Show 92 quoted lines

> apply_stash() and show_stash() each call rev-parse with
> '--default refs/stash' as an argument. This option causes rev-parse to
> operate on refs/stash if it is not able to successfully operate on any
> element of the command line. This includes failure to supply a "valid"
> revision. This has the effect of causing 'stash apply' and 'stash show'
> to operate as if stash@{0} had been supplied when an invalid revision is
> supplied.
> 
> e.g. 'git stash apply stahs@{1}' would fall back to
>      'git stash apply stash@{0}'
> 
> This patch modifies these two functions so that they avoid using the
> --default option of rev-parse.
> 
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
> 
> 
> This should fix the case I mention above, but it does not fix the
> case where a non-existent reflog entry is specified. In this case
> the last entry will be selected.
> 
> 	$ git stash list
> 	stash@{0}: WIP on master: c050772... small java change
> 	stash@{1}: WIP on master: c050772... small java change
> 	stash@{2}: WIP on master: c050772... small java change
> 	stash@{3}: WIP on master: c050772... small java change
> 	$ git stash apply stash@{10}
> 	warning: Log for 'stash' only has 4 entries.
> 	# On branch master
> 	# Changed but not updated:
> 	... etc.
> 
> stash@{3} was applied.
> 
> Luckily, the dangerous case has no effect.
> 
> 	$ git stash drop stash@{10}
> 	Dropped stash@{10} (b7a2467e58109c92d799d059f508f35853d0bff7)
> 	$ git stash list
> 	stash@{0}: WIP on master: c050772... small java change
> 	stash@{1}: WIP on master: c050772... small java change
> 	stash@{2}: WIP on master: c050772... small java change
> 	stash@{3}: WIP on master: c050772... small java change
> 
> -brandon
> 
> 
>  git-stash.sh |   17 +++++++++++++++--
>  1 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/git-stash.sh b/git-stash.sh
> index d799c76..6bd2572 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -144,7 +144,14 @@ show_stash () {
>  	then
>  		flags=--stat
>  	fi
> -	s=$(git rev-parse --revs-only --no-flags --default $ref_stash "$@")
> +
> +	if test $# = 0
> +	then
> +		set x "$ref_stash@{0}"
> +		shift
> +	fi
> +
> +	s=$(git rev-parse --revs-only --no-flags "$@")
>  
>  	w_commit=$(git rev-parse --verify "$s") &&
>  	b_commit=$(git rev-parse --verify "$s^") &&
> @@ -163,13 +170,19 @@ apply_stash () {
>  		shift
>  	esac
>  
> +	if test $# = 0
> +	then
> +		set x "$ref_stash@{0}"
> +		shift
> +	fi
> +
>  	# current index state
>  	c_tree=$(git write-tree) ||
>  		die 'Cannot apply a stash in the middle of a merge'
>  
>  	# stash records the work tree, and is a merge between the
>  	# base commit (first parent) and the index tree (second parent).
> -	s=$(git rev-parse --revs-only --no-flags --default $ref_stash "$@") &&
> +	s=$(git rev-parse --revs-only --no-flags "$@") &&
>  	w_tree=$(git rev-parse --verify "$s:") &&
>  	b_tree=$(git rev-parse --verify "$s^1:") &&
>  	i_tree=$(git rev-parse --verify "$s^2:") ||