* [PATCH] stash: disable literal treatment when passing top pathspec
@ 2022-04-08 3:12 Kyle Meyer
2022-04-08 6:33 ` Bagas Sanjaya
2022-04-08 18:46 ` Junio C Hamano
0 siblings, 2 replies; 7+ messages in thread
From: Kyle Meyer @ 2022-04-08 3:12 UTC (permalink / raw)
To: git; +Cc: Thomas Gummerer, joostkremers
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.
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
--
2.34.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] stash: disable literal treatment when passing top pathspec
2022-04-08 3:12 [PATCH] stash: disable literal treatment when passing top pathspec Kyle Meyer
@ 2022-04-08 6:33 ` Bagas Sanjaya
2022-04-08 8:46 ` Ævar Arnfjörð Bjarmason
2022-04-08 18:46 ` Junio C Hamano
1 sibling, 1 reply; 7+ messages in thread
From: Bagas Sanjaya @ 2022-04-08 6:33 UTC (permalink / raw)
To: Kyle Meyer, git; +Cc: Thomas Gummerer, joostkremers
On 08/04/22 10.12, Kyle Meyer wrote:
> +test_expect_success 'stash -u works with --literal-pathspecs' '
> + >untracked &&
> + git --literal-pathspecs stash -u &&
> + test_path_is_missing untracked
> +'
Why not "touch untracked" instead?
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] stash: disable literal treatment when passing top pathspec
2022-04-08 6:33 ` Bagas Sanjaya
@ 2022-04-08 8:46 ` Ævar Arnfjörð Bjarmason
2022-04-08 18:47 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-04-08 8:46 UTC (permalink / raw)
To: Bagas Sanjaya; +Cc: Kyle Meyer, git, Thomas Gummerer, joostkremers
On Fri, Apr 08 2022, Bagas Sanjaya wrote:
> On 08/04/22 10.12, Kyle Meyer wrote:
>> +test_expect_success 'stash -u works with --literal-pathspecs' '
>> + >untracked &&
>> + git --literal-pathspecs stash -u &&
>> + test_path_is_missing untracked
>> +'
>
> Why not "touch untracked" instead?
The ">" form is correct here. We use "touch" when updating the timestamp
to something in particular is important, but here we're just creating an
empty file.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] stash: disable literal treatment when passing top pathspec
2022-04-08 3:12 [PATCH] stash: disable literal treatment when passing top pathspec Kyle Meyer
2022-04-08 6:33 ` Bagas Sanjaya
@ 2022-04-08 18:46 ` Junio C Hamano
2022-04-09 4:10 ` Kyle Meyer
1 sibling, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2022-04-08 18:46 UTC (permalink / raw)
To: Kyle Meyer; +Cc: git, Thomas Gummerer, joostkremers
Kyle Meyer <kyle@kyleam.com> writes:
> 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?
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] stash: disable literal treatment when passing top pathspec
2022-04-08 8:46 ` Ævar Arnfjörð Bjarmason
@ 2022-04-08 18:47 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2022-04-08 18:47 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Bagas Sanjaya, Kyle Meyer, git, Thomas Gummerer, joostkremers
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> On Fri, Apr 08 2022, Bagas Sanjaya wrote:
>
>> On 08/04/22 10.12, Kyle Meyer wrote:
>>> +test_expect_success 'stash -u works with --literal-pathspecs' '
>>> + >untracked &&
>>> + git --literal-pathspecs stash -u &&
>>> + test_path_is_missing untracked
>>> +'
>>
>> Why not "touch untracked" instead?
>
> The ">" form is correct here. We use "touch" when updating the timestamp
> to something in particular is important, but here we're just creating an
> empty file.
Maybe it is better to have it in CodingGuidelines or t/README?
Thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] stash: disable literal treatment when passing top pathspec
2022-04-08 18:46 ` Junio C Hamano
@ 2022-04-09 4:10 ` Kyle Meyer
2022-04-11 17:55 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Kyle Meyer @ 2022-04-09 4:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Thomas Gummerer, joostkremers
Junio C Hamano writes:
> Kyle Meyer <kyle@kyleam.com> writes:
[...]
>> * 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?
For the --keep-index/checkout case, yes, it looks like the command
starts from the top-level. Passing "." as the pathspec to checkout
works fine, as far as I can tell.
However, for --include-untracked/clean case, the subprocess directory is
set to startup_info->original_cwd since 0fce211ccc (stash: do not
attempt to remove startup_info->original_cwd, 2021-12-09).
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] stash: disable literal treatment when passing top pathspec
2022-04-09 4:10 ` Kyle Meyer
@ 2022-04-11 17:55 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2022-04-11 17:55 UTC (permalink / raw)
To: Elijah Newren; +Cc: git, Thomas Gummerer, joostkremers, Kyle Meyer
Kyle Meyer <kyle@kyleam.com> writes:
> However, for --include-untracked/clean case, the subprocess directory is
> set to startup_info->original_cwd since 0fce211ccc (stash: do not
> attempt to remove startup_info->original_cwd, 2021-12-09).
Interesting. I find the logic there a bit convoluted. IIUC, it
goes like this:
- we do not want to lose the directory our process was originally
in, which is recorded in startup_info->original_cmd.
- we have gone up to the root of the working tree, and running
"clean" from there is what we want---even if we started "git
stash" from a subdirectory, we want to make the entire working
tree clean, not just inside our subdirectory.
- but we came up with a hack that allows us to skip removing the
directory the Git process started at. To take advantage of the
mechanism, we'd need to start from that original_cmd.
- but then "clean" run from that subdirectory normally cleans only
that subdirectory, which is not what we want to do. To work it
around, we'd need to pass :/ pathspec to say that we are cleaning
from the top.
It makes me suspect that "we protect current directory" is a too
specialized way that didn't really consider the possibility that we
sometimes spawn a subcommand. Even "we protect this directory" may
not be sufficient and we may need a "we protect these directories",
I suspect. When the user originally starts "git foo" in one
directory, which may have to run "git bar" in another directory, and
"git bar" would want to protect the directory it starts in and also
where "git foo" started from, no? It almost makes me suspect that
we'd want some "git" wide option that allows us to pass a list of
paths not to rmdir, whose default value is ["."], or something.
Elijah, thoughts?
But as a short-term fix, I think "--no-literal-pathspecs" is fine for
this code path.
Thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-04-11 17:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-08 3:12 [PATCH] stash: disable literal treatment when passing top pathspec Kyle Meyer
2022-04-08 6:33 ` Bagas Sanjaya
2022-04-08 8:46 ` Ævar Arnfjörð Bjarmason
2022-04-08 18:47 ` Junio C Hamano
2022-04-08 18:46 ` Junio C Hamano
2022-04-09 4:10 ` Kyle Meyer
2022-04-11 17:55 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.