Re: [PATCH] stash: disable literal treatment when passing top pathspec

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 8, 2022, 18:46 UTC
Message-ID: <xmqqa6cvmmzn.fsf@gitster.g>
In-Reply-To: <20220408031228.782547-1-kyle@kyleam.com>

Kyle Meyer <kyle@kyleam.com> writes:

Show 13 quoted lines

> do_push_stash() passes ":/" as the pathspec to two subprocess calls.
> When pathspecs are interpreted literally for the main process, these
> subprocess calls do not behave as intended:
>
>  * the 'git clean' call, triggered by --include-untracked, does not
>    remove untracked files from the working tree
>
>  * the 'git checkout' call, triggered by --keep-index, fails with a
>    message about ":/" not matching any known files, and the main
>    command exits with a non-zero status
>
> Fix both of these spots by passing --no-literal-pathspecs to the
> subprocess commands.

Yuck (to the original problem, not to the proposed solution).

I wonder if stopping to use ":/" (or using "." instead, if we need to give _some_ pathspec) is a better approach. Don't we move to the top of the working tree by the time cmd_stash() is called and whatever subprocess we spawn via run_command() interface will start at the top anyway, no?

Show 63 quoted lines

> Signed-off-by: Kyle Meyer <kyle@kyleam.com>
> ---
>  builtin/stash.c                    | 5 ++++-
>  t/t3903-stash.sh                   | 5 +++++
>  t/t3905-stash-include-untracked.sh | 5 +++++
>  3 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 0c7b6a9588..afc8400c5d 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1529,7 +1529,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>  					     GIT_WORK_TREE_ENVIRONMENT,
>  					     the_repository->worktree);
>  			}
> -			strvec_pushl(&cp.args, "clean", "--force",
> +			strvec_pushl(&cp.args, "--no-literal-pathspecs",
> +				     "clean", "--force",
>  				     "--quiet", "-d", ":/", NULL);
>  			if (include_untracked == INCLUDE_ALL_FILES)
>  				strvec_push(&cp.args, "-x");
> @@ -1592,6 +1593,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>  			struct child_process cp = CHILD_PROCESS_INIT;
>  
>  			cp.git_cmd = 1;
> +			if (!ps->nr)
> +				strvec_push(&cp.args, "--no-literal-pathspecs");
>  			strvec_pushl(&cp.args, "checkout", "--no-overlay",
>  				     oid_to_hex(&info.i_tree), "--", NULL);
>  			if (!ps->nr)
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 4abbc8fcca..f85c3a06cb 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1383,6 +1383,11 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
>  	test_path_is_missing to-remove
>  '
>  
> +test_expect_success 'stash --keep-index succeeds with --literal-pathspecs' '
> +	echo modified >file &&
> +	git --literal-pathspecs stash --keep-index
> +'
> +
>  test_expect_success 'stash apply should succeed with unmodified file' '
>  	echo base >file &&
>  	git add file &&
> diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
> index 5390eec4e3..2f216274b2 100755
> --- a/t/t3905-stash-include-untracked.sh
> +++ b/t/t3905-stash-include-untracked.sh
> @@ -427,5 +427,10 @@ test_expect_success 'stash -u ignores sub-repository' '
>  	git init sub-repo &&
>  	git stash -u
>  '
> +test_expect_success 'stash -u works with --literal-pathspecs' '
> +	>untracked &&
> +	git --literal-pathspecs stash -u &&
> +	test_path_is_missing untracked
> +'
>  
>  test_done
>
> base-commit: bf23fe5c37d62f37267d31d4afa1a1444f70cdac