Re: [PATCH] git-stash: fix pushing stash with pathspec from subdir

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 13, 2017, 15:33 UTC
Message-ID: <xmqqr2yn9a2h.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <6200f6bf1a9ac81d02391b898d203d650c6af962.1497353866.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 10 quoted lines

> The `git stash push` command recently gained the ability to get a
> pathspec as its argument to only stash matching files. Calling this
> command from a subdirectory does not work, though, as one of the first
> things we do is changing to the top level directory without keeping
> track of the prefix from which the command is being run.
>
> Fix the shortcoming by storing the prefix previous to the call to
> `cd_to_toplevel` and then subsequently using `git rev-parse --prefix` to
> correctly resolve the pathspec. Add a test to catch future breakage of
> this usecase.

It sounds more like a simple bug than "shortcoming" ;-), and the right approach to fix it is to add the original prefix before the pathspecs before using them, which is exactly what your patch does. Looks good.

I suspect that "rev-parse --prefix" needs a bit of tweak to make it truly usable for pathspecs with magic, but that is a totally separate issue.

Thanks.

Show 54 quoted lines

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  git-stash.sh     |  3 +++
>  t/t3903-stash.sh | 16 ++++++++++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 2fb651b2b..e7b85932d 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -19,6 +19,7 @@ OPTIONS_SPEC=
>  START_DIR=$(pwd)
>  . git-sh-setup
>  require_work_tree
> +prefix=$(git rev-parse --show-prefix) || exit 1
>  cd_to_toplevel
>  
>  TMP="$GIT_DIR/.git-stash.$$"
> @@ -273,6 +274,8 @@ push_stash () {
>  		shift
>  	done
>  
> +	eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"
> +
>  	if test -n "$patch_mode" && test -n "$untracked"
>  	then
>  		die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 3b4bed5c9..4046817d7 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -812,6 +812,22 @@ test_expect_success 'stash -- <pathspec> stashes and restores the file' '
>  	test_path_is_file bar
>  '
>  
> +test_expect_success 'stash -- <pathspec> stashes in subdirectory' '
> +	mkdir sub &&
> +	>foo &&
> +	>bar &&
> +	git add foo bar &&
> +	(
> +		cd sub &&
> +		git stash push -- ../foo
> +	) &&
> +	test_path_is_file bar &&
> +	test_path_is_missing foo &&
> +	git stash pop &&
> +	test_path_is_file foo &&
> +	test_path_is_file bar
> +'
> +
>  test_expect_success 'stash with multiple pathspec arguments' '
>  	>foo &&
>  	>bar &&