[PATCH] fix git add :!x exiting with error when x is in .gitignore

[PATCH] fix git add :!x exiting with error when x is in .gitignore

[Remy D. Farley]

`git add :!x .`, which is also executed as part of `git stash :!x`,
seems to treat pathspec with and without exclude magic the same, exiting
with error when "x" exists and is in gitignore.

Git-add manpage doesn't specify that exclude pathspecs should be treated
anyhow differently from normal ones, which seems like a bug. Two
inconsistencies I noticed: `git add :!ignored .` succeeds when "ignored"
file doesn't exist, and `git add :!ignored/x .` succeeds even when
"ignored/x" file exists.

This commit makes makes `git add :!x` not error on x being excluded path.


| $ sh repro.sh
| [...]
| + echo x >.gitignore
| + echo x >x
| + git stash --include-untracked -- ':!x'
| Saved working directory and index state WIP on main: c8a842d Init
| The following paths are ignored by one of your .gitignore files:
| x
| hint: Use -f if you really want to add them.
| hint: Disable this message with "git config set advice.addIgnoredFile false"
| + echo exited with code 1
| exited with code 1


| # repro.sh
| rm -rf repro; mkdir repro; cd repro
| trap 'echo exited with code $?' EXIT
| set -euo pipefail -o xtrace
|
| git init
| git commit -m Init --allow-empty
|
| # Commenting out either of the following lines makes git add/stash below succeed
| echo x >.gitignore
| echo x >x
|
| # Git add . is executed as part of git stash, as can be seen using strace -ffeexecve:
| git add -- ":!x" . # fails
| # git stash --include-untracked -- ":!x" # fails

---
I'm not sure who else to cc, last commit touching this code is 2ec87741
from 10 year ago, being a mere refactoring. I think this bug was simply
overlooked when introducing PATHSPEC_EXCLUDE.

Thanks to Tian Yuchen for looking at my earlier submission (and noticing
an awkwardly stupid bug there).
---
 dir.c                              |  3 +++
 t/t2204-add-ignored.sh             | 14 ++++++++++++++
 t/t3905-stash-include-untracked.sh | 23 +++++++++++++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/dir.c b/dir.c
index b00821f294..ed6b99e337 100644
--- a/dir.c
+++ b/dir.c
@@ -2280,6 +2280,9 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
 		const struct pathspec_item *item = &pathspec->items[i];
 		int len = item->nowildcard_len;
 
+		if (item->magic & PATHSPEC_EXCLUDE)
+			continue;
+
 		if (len == pathlen &&
 		    !ps_strncmp(item, item->match, path, pathlen))
 			return 1;
diff --git a/t/t2204-add-ignored.sh b/t/t2204-add-ignored.sh
index 31eb233df5..76c53fbfde 100755
--- a/t/t2204-add-ignored.sh
+++ b/t/t2204-add-ignored.sh
@@ -47,6 +47,20 @@ do
 	test_expect_success "complaints for ignored $i with unignored file output" '
 		test_grep -e "Use -f if" err
 	'
+
+	test_expect_success "no complaints for unignored file with ignored :!$i" '
+		rm -f .git/index &&
+		git add file ":!$i" &&
+		git ls-files file "$i" >out &&
+		test -s out
+	'
+
+	test_expect_success "complaints for ignored $i with ignored :!ign" '
+		rm -f .git/index &&
+		test_must_fail git add "$i" :!ign 2>err &&
+		git ls-files "$i" ign >out &&
+		test_must_be_empty out
+	'
 done
 
 for i in sub sub/*
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index 7704709054..028ff3efc0 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -206,6 +206,29 @@ test_expect_success 'stash push --include-untracked with pathspec' '
 	test_path_is_file foo
 '
 
+test_expect_success 'stash push --include-untracked with :!pathspec' '
+	>foo &&
+	>bar &&
+	git stash push --include-untracked -- :!bar &&
+	test_path_is_file bar &&
+	test_path_is_missing foo &&
+	git stash pop &&
+	test_path_is_file bar &&
+	test_path_is_file foo
+'
+
+test_expect_success 'stash push --include-untracked with :!pathspec in .gitignore' '
+	echo ignored > .gitignore &&
+	>foo &&
+	>ignored &&
+	git stash push --include-untracked -- :!ignored &&
+	test_path_is_file ignored &&
+	test_path_is_missing foo &&
+	git stash pop &&
+	test_path_is_file ignored &&
+	test_path_is_file foo
+'
+
 test_expect_success 'stash push with $IFS character' '
 	>"foo bar" &&
 	>foo &&
-- 
2.51.2

Re: [PATCH] fix git add :!x exiting with error when x is in .gitignore

[Junio C Hamano]

"Remy D. Farley" <one-d-wide@protonmail.com> writes:

> diff --git a/dir.c b/dir.c
> index b00821f294..ed6b99e337 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2280,6 +2280,9 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
>  		const struct pathspec_item *item = &pathspec->items[i];
>  		int len = item->nowildcard_len;
>  
> +		if (item->magic & PATHSPEC_EXCLUDE)
> +			continue;
> +
>  		if (len == pathlen &&
>  		    !ps_strncmp(item, item->match, path, pathlen))
>  			return 1;

A question that immediately comes to mind is if it is appropriate
for a negated pathspec element to recuse itself like this from the
decision process and let other pathspec elements decide the fate of
the path, or if a negated pathspec element should take a more active
role of saying "no" (no, not by immediately returning 0, but this
loop may have to become a two step process if we wanted to implement
e.g., for the function to yield "yes", it has to match at least one
positive pathspec element and zero negated one, or something like
that).

What should

	git add "$x" ":!$y"

do when a path <matches, does not match> $X and <matches, does not
match> $Y?  We have four combinations to consider in such a case.
The code in the patch says it should behave identically to

	git add "$x"

and negated ":!$y" should not make any difference.  Is that what we
want?

Thanks.

Re: [PATCH] fix git add :!x exiting with error when x is in .gitignore

[Tian Yuchen]

On 2/5/26 00:48, Junio C Hamano wrote:
> "Remy D. Farley" <one-d-wide@protonmail.com> writes:

> A question that immediately comes to mind is if it is appropriate
> for a negated pathspec element to recuse itself like this from the
> decision process and let other pathspec elements decide the fate of
> the path, or if a negated pathspec element should take a more active
> role of saying "no" (no, not by immediately returning 0, but this
> loop may have to become a two step process if we wanted to implement
> e.g., for the function to yield "yes", it has to match at least one
> positive pathspec element and zero negated one, or something like
> that).

You are right. To illustrate, if we run:

git add ignored_file ":!ignored_file"

Then following things might happen with the patch:
-> For the first item,
	- Does it match 'exclude'? No.
	- Does it match 'path'? Yes.
	- Return 1.
-> For the second item,
	- Is never reached
-> Git complain,
	'The following paths are ignored: ignored_file.'

In other word, it's not the expected silent no-op (returning 0).

As you suggested, The loop needs to verify that the path matches at 
least one positive item AND matches none of the negative items. A 
possible way to acheive it is:
(Notice that we no longer return 1 in the half way)


 >bool matched_positive = false;
 >
 >for (item in pathspec) {
 >	if (item matches patch) {
 >		if (item is exclude) {
 >			return 0;
 >		} else {
 >			matched_positive = true;
 >		}
 >	}
 >}
 >
 >return matched_positive ? 1 : 0;

By the way, I think extreme cases like 'git add x :!x' should be added 
into the test scripts.

Regards,

Yuchen

Re: [PATCH] fix git add :!x exiting with error when x is in .gitignore

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

> As you suggested, The loop needs to verify that the path matches at 
> least one positive item AND matches none of the negative items. A 
> possible way to acheive it is:
> (Notice that we no longer return 1 in the half way)
>
>  >bool matched_positive = false;
>  >
>  >for (item in pathspec) {
>  >	if (item matches patch) {
>  >		if (item is exclude) {
>  >			return 0;
>  >		} else {
>  >			matched_positive = true;
>  >		}
>  >	}
>  >}
>  >
>  >return matched_positive ? 1 : 0;

One caveat.  The case without any positive pathspec entries needs
special consideration.  I suspect, but can be totally wrong as I
didn't think things through thoroughly, that

    git add "!$y"

would want to behave as if an implicit "everything matches" was
given, i.e.,

    git add "!$y" .

while a pathspec with one or more positive entries would not need
and want such an implicit "everything" treatment.

> By the way, I think extreme cases like 'git add x :!x' should be added 
> into the test scripts.

True.

[PATCH] fix git add :!x exiting with error when x is in .gitignore

[Remy D. Farley]

Junio C Hamano <gitster@pobox.com> wrote:
> A question that immediately comes to mind is if it is appropriate
> for a negated pathspec element to recuse itself like this from the
> decision process and let other pathspec elements decide the fate of
> the path, or if a negated pathspec element should take a more active
> role of saying "no" (no, not by immediately returning 0, but this
> loop may have to become a two step process if we wanted to implement
> e.g., for the function to yield "yes", it has to match at least one
> positive pathspec element and zero negated one, or something like
> that).
>
> What should
>
>     git add "$x" ":!$y"
>
> do when a path <matches, does not match> $X and <matches, does not
> match> $Y? We have four combinations to consider in such a case.
> The code in the patch says it should behave identically to
>
>     git add "$x"
>
> and negated ":!$y" should not make any difference. Is that what we
> want?


I indeed failed to consider cases where pathspecs could interfere with each
other, sorry.

It does seems like we don't want to just blanketly ignore negated pathspecs,
at least for the sake of consistency:

    git add -n :!a a/ignored/c # is ok (even on mainline, nothing is added)

    git add -n :!a/b a/b # ok (same)

Probably the same should hold after this patch if "a" or "a/b" were excluded.
I'll try to think this through.


Tian Yuchen <a3205153416@gmail.com> wrote:
> By the way, I think extreme cases like 'git add x :!x' should be added
> into the test scripts.


I think this was already covered, though with a simplistic (wrong) approach.

>  for i in ign dir/ign dir/sub dir/sub/*ign sub/file sub sub/*
>  do
>  	[...]
> +	test_expect_success "complaints for ignored $i with ignored :!ign" '
> +		rm -f .git/index &&
> +		test_must_fail git add "$i" :!ign 2>err &&
> +		git ls-files "$i" ign >out &&
> +		test_must_be_empty out
> +	'
>  done


Junio C Hamano <gitster@pobox.com> wrote:
> One caveat.  The case without any positive pathspec entries needs
> special consideration.  I suspect, but can be totally wrong as I
> didn't think things through thoroughly, that
> 
>     git add "!$y"
> 
> would want to behave as if an implicit "everything matches" was
> given, i.e.,
> 
>     git add "!$y" .
>
> while a pathspec with one or more positive entries would not need
> and want such an implicit "everything" treatment.


This case is actually already handled by the pathspec itself.


From pathspec.c:
> void parse_pathspec(struct pathspec *pathspec,
> 		    unsigned magic_mask, unsigned flags,
> 		    const char *prefix, const char **argv)
> {
> 	[...]
> 	/*
> 	 * If everything is an exclude pattern, add one positive pattern
> 	 * that matches everything. We allocated an extra one for this.
> 	 */
> 	if (nr_exclude == n) {
> 		int plen = (!(flags & PATHSPEC_PREFER_CWD)) ? 0 : prefixlen;
> 		init_pathspec_item(item + n, 0, prefix, plen, ".");
> 		pathspec->nr++;
> 	}

Re: [PATCH] fix git add :!x exiting with error when x is in .gitignore

[Junio C Hamano]

"Remy D. Farley" <one-d-wide@protonmail.com> writes:

> This case is actually already handled by the pathspec itself.
>
>
> From pathspec.c:
>> void parse_pathspec(struct pathspec *pathspec,
>> 		    unsigned magic_mask, unsigned flags,
>> 		    const char *prefix, const char **argv)
>> {
>> 	[...]
>> 	/*
>> 	 * If everything is an exclude pattern, add one positive pattern
>> 	 * that matches everything. We allocated an extra one for this.
>> 	 */
>> 	if (nr_exclude == n) {
>> 		int plen = (!(flags & PATHSPEC_PREFER_CWD)) ? 0 : prefixlen;
>> 		init_pathspec_item(item + n, 0, prefix, plen, ".");
>> 		pathspec->nr++;
>> 	}

Yes, that is from Linus 9 years ago plus a bit of my work, in
859b7f1d (pathspec: don't error out on all-exclusionary pathspec
patterns, 2017-02-07) and b02fdbc8 (pathspec: correct an empty
string used as a pathspec element, 2022-05-29).  No wonder it
sounded familiar ;-).

Thanks.