gitignore + commit with excludes = bug

gitignore + commit with excludes = bug

[Paul Jackson]

Hello,

I stumbled upon what I believe is a bug in git.
See the following reproduction steps:

mkdir test
cd test
git init
echo 1 > ignored
echo 1 > not-ignored
echo ignored > .gitignore
git add -A -- ':!ignored' || echo 'ERROR!!!'

In these steps, I ignore the "ignored" file twice - first time in
.gitignore, and second time in the "git add" command. I didn't expect
this to be a problem, but I'm getting the following error message:

The following paths are ignored by one of your .gitignore files:
ignored

It looks as if git thinks I wanted to include, not exclude "ignored"
in "git add".

Re: gitignore + commit with excludes = bug

[Philip Oakley]

Hi Paul,

On 01/05/2021 19:37, Paul Jackson wrote:
> Hello,
>
> I stumbled upon what I believe is a bug in git.
> See the following reproduction steps:
>
> mkdir test
> cd test
> git init
> echo 1 > ignored
> echo 1 > not-ignored
> echo ignored > .gitignore
> git add -A -- ':!ignored' || echo 'ERROR!!!'
>
> In these steps, I ignore the "ignored" file twice - first time in
> .gitignore, and second time in the "git add" command. I didn't expect
> this to be a problem, but I'm getting the following error message:
>
> The following paths are ignored by one of your .gitignore files:
> ignored
>
> It looks as if git thinks I wanted to include, not exclude "ignored"
> in "git add".
I was intrigued by this. The man pages can be hard to decipher, and it
may be an 'as designed' feature, but still not intuitive

It took a while to find the relevant parts of the man pages.

The `-A` option of `add` is under
https://git-scm.com/docs/git-add#Documentation/git-add.txt---no-ignore-removal
which has caveats for whether a pathspec is given.

The `exclude` magic pathspec is under
https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-exclude
and again has caveats and a double negative regarding whether the
`exclude` pathspec counts as a path spec.

I _think_ that it is saying that the `exclude` pathspec is ignored for
the purpose of the `-A` (all) condition for git add.
The `git add` then decides that the the set of files to be added is just
the single 'not-ignored' file, having already used the `.gitignore` to
"exclude" the 'ignored' file from that set of files to add.

Now your "exclude" pathspec comes into play and tries remove the named
"exclude" file from the set, and doesn't find it. Or at least that's how
I read it as a plausible explanation. That said, I haven't looked at the
code, so that could be all wrong.

A possible place to start is the
https://gitlab.cecs.anu.edu.au/comp8440/git/commit/ef79b1f8704668a6cdf4278f9255e03ca785bfb4
patch (google..) or
https://github.com/git/git/commit/ef79b1f8704668a6cdf4278f9255e03ca785bfb4

Also locate where the error message is generated. It maybe just a warning.

--
Philip

Re: gitignore + commit with excludes = bug

[Matheus Tavares]

Hi, Paul and Philip

On Tue, May 4, 2021 at 9:55 AM Philip Oakley <philipoakley@iee.email> wrote:
>
> Hi Paul,
>
> On 01/05/2021 19:37, Paul Jackson wrote:
> > Hello,
> >
> > I stumbled upon what I believe is a bug in git.
> > See the following reproduction steps:
> >
> > mkdir test
> > cd test
> > git init
> > echo 1 > ignored
> > echo 1 > not-ignored
> > echo ignored > .gitignore
> > git add -A -- ':!ignored' || echo 'ERROR!!!'
> >
> > In these steps, I ignore the "ignored" file twice - first time in
> > .gitignore, and second time in the "git add" command. I didn't expect
> > this to be a problem, but I'm getting the following error message:
> >
> > The following paths are ignored by one of your .gitignore files:
> > ignored
> >
> > It looks as if git thinks I wanted to include, not exclude "ignored"
> > in "git add".
> I was intrigued by this. The man pages can be hard to decipher, and it
> may be an 'as designed' feature, but still not intuitive
>
> It took a while to find the relevant parts of the man pages.
>
> The `-A` option of `add` is under
> https://git-scm.com/docs/git-add#Documentation/git-add.txt---no-ignore-removal
> which has caveats for whether a pathspec is given.
>
> The `exclude` magic pathspec is under
> https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-exclude
> and again has caveats and a double negative regarding whether the
> `exclude` pathspec counts as a path spec.
>
> I _think_ that it is saying that the `exclude` pathspec is ignored for
> the purpose of the `-A` (all) condition for git add.

Hmm, I think the issue is not really related to `-A`. In fact, if we
reproduce Paul's original example without `-A`, we still get the
warning.

The problem seems to be at `dir.c:exclude_matches_pathspec()`, which
creates the list of ignored files that is later used by `git add` to
presented the "The following paths are ignored..." warning.

This function ignores the `exclude` magic, so a path 'x' incorrectly
matches both ':x' and ':!x'. And thus, we end up warning the user about
'x' being ignored even when the user had ran `git add ':!x'`.

I think something like this, might solve the problem:

diff --git a/dir.c b/dir.c
index 3474e67e8f..165ca6a543 100644
--- a/dir.c
+++ b/dir.c
@@ -2042,6 +2042,25 @@ 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 0;
+		if (len > pathlen &&
+		    item->match[pathlen] == '/' &&
+		    !ps_strncmp(item, item->match, path, pathlen))
+			return 0;
+	}
+
+	for (i = 0; i < pathspec->nr; i++) {
+		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;
---

I had to split the original loop into two and handle the `exclude`
pathspecs first because we cannot let the original loop return early
when one of the `non-exclude` pathspecs matches the path. Otherwise, we
would still incorrectly warn the user on executions like
`git add ignored ':!ignored'`.

(We might also want to extract the matching part to its own function to
avoid repeating this code on the two loops.)

Re: gitignore + commit with excludes = bug

[Paul Jackson]

Thanks, I hope it gets merged at some point.

On Tue, May 4, 2021 at 5:07 PM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>
> Hi, Paul and Philip
>
> On Tue, May 4, 2021 at 9:55 AM Philip Oakley <philipoakley@iee.email> wrote:
> >
> > Hi Paul,
> >
> > On 01/05/2021 19:37, Paul Jackson wrote:
> > > Hello,
> > >
> > > I stumbled upon what I believe is a bug in git.
> > > See the following reproduction steps:
> > >
> > > mkdir test
> > > cd test
> > > git init
> > > echo 1 > ignored
> > > echo 1 > not-ignored
> > > echo ignored > .gitignore
> > > git add -A -- ':!ignored' || echo 'ERROR!!!'
> > >
> > > In these steps, I ignore the "ignored" file twice - first time in
> > > .gitignore, and second time in the "git add" command. I didn't expect
> > > this to be a problem, but I'm getting the following error message:
> > >
> > > The following paths are ignored by one of your .gitignore files:
> > > ignored
> > >
> > > It looks as if git thinks I wanted to include, not exclude "ignored"
> > > in "git add".
> > I was intrigued by this. The man pages can be hard to decipher, and it
> > may be an 'as designed' feature, but still not intuitive
> >
> > It took a while to find the relevant parts of the man pages.
> >
> > The `-A` option of `add` is under
> > https://git-scm.com/docs/git-add#Documentation/git-add.txt---no-ignore-removal
> > which has caveats for whether a pathspec is given.
> >
> > The `exclude` magic pathspec is under
> > https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-exclude
> > and again has caveats and a double negative regarding whether the
> > `exclude` pathspec counts as a path spec.
> >
> > I _think_ that it is saying that the `exclude` pathspec is ignored for
> > the purpose of the `-A` (all) condition for git add.
>
> Hmm, I think the issue is not really related to `-A`. In fact, if we
> reproduce Paul's original example without `-A`, we still get the
> warning.
>
> The problem seems to be at `dir.c:exclude_matches_pathspec()`, which
> creates the list of ignored files that is later used by `git add` to
> presented the "The following paths are ignored..." warning.
>
> This function ignores the `exclude` magic, so a path 'x' incorrectly
> matches both ':x' and ':!x'. And thus, we end up warning the user about
> 'x' being ignored even when the user had ran `git add ':!x'`.
>
> I think something like this, might solve the problem:
>
> diff --git a/dir.c b/dir.c
> index 3474e67e8f..165ca6a543 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2042,6 +2042,25 @@ 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 0;
> +               if (len > pathlen &&
> +                   item->match[pathlen] == '/' &&
> +                   !ps_strncmp(item, item->match, path, pathlen))
> +                       return 0;
> +       }
> +
> +       for (i = 0; i < pathspec->nr; i++) {
> +               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;
> ---
>
> I had to split the original loop into two and handle the `exclude`
> pathspecs first because we cannot let the original loop return early
> when one of the `non-exclude` pathspecs matches the path. Otherwise, we
> would still incorrectly warn the user on executions like
> `git add ignored ':!ignored'`.
>
> (We might also want to extract the matching part to its own function to
> avoid repeating this code on the two loops.)

Re: gitignore + commit with excludes = bug

[Philip Oakley]

On 09/05/2021 07:21, Paul Jackson wrote:
> Thanks, I hope it gets merged at some point.

Are you able to pull it together as a formal patch, with a suitable
commit message, so that it can be reviewed and merged?

Philip

>
> On Tue, May 4, 2021 at 5:07 PM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
>> Hi, Paul and Philip
>>
>> On Tue, May 4, 2021 at 9:55 AM Philip Oakley <philipoakley@iee.email> wrote:
>>> Hi Paul,
>>>
>>> On 01/05/2021 19:37, Paul Jackson wrote:
>>>> Hello,
>>>>
>>>> I stumbled upon what I believe is a bug in git.
>>>> See the following reproduction steps:
>>>>
>>>> mkdir test
>>>> cd test
>>>> git init
>>>> echo 1 > ignored
>>>> echo 1 > not-ignored
>>>> echo ignored > .gitignore
>>>> git add -A -- ':!ignored' || echo 'ERROR!!!'
>>>>
>>>> In these steps, I ignore the "ignored" file twice - first time in
>>>> .gitignore, and second time in the "git add" command. I didn't expect
>>>> this to be a problem, but I'm getting the following error message:
>>>>
>>>> The following paths are ignored by one of your .gitignore files:
>>>> ignored
>>>>
>>>> It looks as if git thinks I wanted to include, not exclude "ignored"
>>>> in "git add".
>>> I was intrigued by this. The man pages can be hard to decipher, and it
>>> may be an 'as designed' feature, but still not intuitive
>>>
>>> It took a while to find the relevant parts of the man pages.
>>>
>>> The `-A` option of `add` is under
>>> https://git-scm.com/docs/git-add#Documentation/git-add.txt---no-ignore-removal
>>> which has caveats for whether a pathspec is given.
>>>
>>> The `exclude` magic pathspec is under
>>> https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-exclude
>>> and again has caveats and a double negative regarding whether the
>>> `exclude` pathspec counts as a path spec.
>>>
>>> I _think_ that it is saying that the `exclude` pathspec is ignored for
>>> the purpose of the `-A` (all) condition for git add.
>> Hmm, I think the issue is not really related to `-A`. In fact, if we
>> reproduce Paul's original example without `-A`, we still get the
>> warning.
>>
>> The problem seems to be at `dir.c:exclude_matches_pathspec()`, which
>> creates the list of ignored files that is later used by `git add` to
>> presented the "The following paths are ignored..." warning.
>>
>> This function ignores the `exclude` magic, so a path 'x' incorrectly
>> matches both ':x' and ':!x'. And thus, we end up warning the user about
>> 'x' being ignored even when the user had ran `git add ':!x'`.
>>
>> I think something like this, might solve the problem:
>>
>> diff --git a/dir.c b/dir.c
>> index 3474e67e8f..165ca6a543 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2042,6 +2042,25 @@ 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 0;
>> +               if (len > pathlen &&
>> +                   item->match[pathlen] == '/' &&
>> +                   !ps_strncmp(item, item->match, path, pathlen))
>> +                       return 0;
>> +       }
>> +
>> +       for (i = 0; i < pathspec->nr; i++) {
>> +               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;
>> ---
>>
>> I had to split the original loop into two and handle the `exclude`
>> pathspecs first because we cannot let the original loop return early
>> when one of the `non-exclude` pathspecs matches the path. Otherwise, we
>> would still incorrectly warn the user on executions like
>> `git add ignored ':!ignored'`.
>>
>> (We might also want to extract the matching part to its own function to
>> avoid repeating this code on the two loops.)

Re: gitignore + commit with excludes = bug

[Elijah Newren]

On Tue, May 4, 2021 at 7:10 AM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>
> Hi, Paul and Philip
>
> On Tue, May 4, 2021 at 9:55 AM Philip Oakley <philipoakley@iee.email> wrote:
> >
> > Hi Paul,
> >
> > On 01/05/2021 19:37, Paul Jackson wrote:
> > > Hello,
> > >
> > > I stumbled upon what I believe is a bug in git.
> > > See the following reproduction steps:
> > >
> > > mkdir test
> > > cd test
> > > git init
> > > echo 1 > ignored
> > > echo 1 > not-ignored
> > > echo ignored > .gitignore
> > > git add -A -- ':!ignored' || echo 'ERROR!!!'
> > >
> > > In these steps, I ignore the "ignored" file twice - first time in
> > > .gitignore, and second time in the "git add" command. I didn't expect
> > > this to be a problem, but I'm getting the following error message:
> > >
> > > The following paths are ignored by one of your .gitignore files:
> > > ignored
> > >
> > > It looks as if git thinks I wanted to include, not exclude "ignored"
> > > in "git add".
> > I was intrigued by this. The man pages can be hard to decipher, and it
> > may be an 'as designed' feature, but still not intuitive
> >
> > It took a while to find the relevant parts of the man pages.
> >
> > The `-A` option of `add` is under
> > https://git-scm.com/docs/git-add#Documentation/git-add.txt---no-ignore-removal
> > which has caveats for whether a pathspec is given.
> >
> > The `exclude` magic pathspec is under
> > https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-exclude
> > and again has caveats and a double negative regarding whether the
> > `exclude` pathspec counts as a path spec.
> >
> > I _think_ that it is saying that the `exclude` pathspec is ignored for
> > the purpose of the `-A` (all) condition for git add.
>
> Hmm, I think the issue is not really related to `-A`. In fact, if we
> reproduce Paul's original example without `-A`, we still get the
> warning.
>
> The problem seems to be at `dir.c:exclude_matches_pathspec()`, which
> creates the list of ignored files that is later used by `git add` to
> presented the "The following paths are ignored..." warning.
>
> This function ignores the `exclude` magic, so a path 'x' incorrectly
> matches both ':x' and ':!x'. And thus, we end up warning the user about
> 'x' being ignored even when the user had ran `git add ':!x'`.
>
> I think something like this, might solve the problem:
>
> diff --git a/dir.c b/dir.c
> index 3474e67e8f..165ca6a543 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2042,6 +2042,25 @@ 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 0;
> +               if (len > pathlen &&
> +                   item->match[pathlen] == '/' &&
> +                   !ps_strncmp(item, item->match, path, pathlen))
> +                       return 0;
> +       }
> +
> +       for (i = 0; i < pathspec->nr; i++) {
> +               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;
> ---

Thanks for tracking this down.  Your analysis and patch look correct
to me, but perhaps we could simplify the code a bit.  Instead of
looping twice, perhaps insert the following code above the if-checks:

+               /*
+                * We can have a bunch of exclusion rules:
+                *    .gitignore
+                *    *.log
+                *    !settings.log
+                * and we're trying to see if the path matches one of these,
+                * but note that the last one is a "negated exclusion rule",
+                * for the excludes to match this pathspec, it needs to not
+                * match the negated exclusion.
+                */
+               int retval = (item->magic & PATHSPEC_EXCLUDE) ? 0 : 1;

and then change all the "return 1" statements to "return retval".

Thoughts?

>
> I had to split the original loop into two and handle the `exclude`
> pathspecs first because we cannot let the original loop return early
> when one of the `non-exclude` pathspecs matches the path. Otherwise, we
> would still incorrectly warn the user on executions like
> `git add ignored ':!ignored'`.
>
> (We might also want to extract the matching part to its own function to
> avoid repeating this code on the two loops.)

Re: gitignore + commit with excludes = bug

[Matheus Tavares Bernardino]

On Mon, May 10, 2021 at 4:27 PM Elijah Newren <newren@gmail.com> wrote:
>
> Thanks for tracking this down.  Your analysis and patch look correct
> to me, but perhaps we could simplify the code a bit.  Instead of
> looping twice, perhaps insert the following code above the if-checks:
>
> +               /*
> +                * We can have a bunch of exclusion rules:
> +                *    .gitignore
> +                *    *.log
> +                *    !settings.log
> +                * and we're trying to see if the path matches one of these,
> +                * but note that the last one is a "negated exclusion rule",
> +                * for the excludes to match this pathspec, it needs to not
> +                * match the negated exclusion.
> +                */
> +               int retval = (item->magic & PATHSPEC_EXCLUDE) ? 0 : 1;
>
> and then change all the "return 1" statements to "return retval".

Hmm, but then wouldn't something like `git add ignored :^ignored` also
trigger the warning, as we see 'ignored' first, and return 1 before
seeing ':^ignored'?

Re: gitignore + commit with excludes = bug

[Elijah Newren]

On Mon, May 10, 2021 at 12:37 PM Matheus Tavares Bernardino
<matheus.bernardino@usp.br> wrote:
>
> On Mon, May 10, 2021 at 4:27 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > Thanks for tracking this down.  Your analysis and patch look correct
> > to me, but perhaps we could simplify the code a bit.  Instead of
> > looping twice, perhaps insert the following code above the if-checks:
> >
> > +               /*
> > +                * We can have a bunch of exclusion rules:
> > +                *    .gitignore
> > +                *    *.log
> > +                *    !settings.log
> > +                * and we're trying to see if the path matches one of these,
> > +                * but note that the last one is a "negated exclusion rule",
> > +                * for the excludes to match this pathspec, it needs to not
> > +                * match the negated exclusion.
> > +                */
> > +               int retval = (item->magic & PATHSPEC_EXCLUDE) ? 0 : 1;
> >
> > and then change all the "return 1" statements to "return retval".
>
> Hmm, but then wouldn't something like `git add ignored :^ignored` also
> trigger the warning, as we see 'ignored' first, and return 1 before
> seeing ':^ignored'?

Oh, right, whoops.  Do you want to add this testcase, and a commit
message for this (and maybe a comment explaining the double loop)?

Re: gitignore + commit with excludes = bug

[Matheus Tavares Bernardino]

On Mon, May 10, 2021 at 7:34 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Mon, May 10, 2021 at 12:37 PM Matheus Tavares Bernardino
> <matheus.bernardino@usp.br> wrote:
> >
> > On Mon, May 10, 2021 at 4:27 PM Elijah Newren <newren@gmail.com> wrote:
> > >
> > > Thanks for tracking this down.  Your analysis and patch look correct
> > > to me, but perhaps we could simplify the code a bit.  Instead of
> > > looping twice, perhaps insert the following code above the if-checks:
> > >
> > > +               /*
> > > +                * We can have a bunch of exclusion rules:
> > > +                *    .gitignore
> > > +                *    *.log
> > > +                *    !settings.log
> > > +                * and we're trying to see if the path matches one of these,
> > > +                * but note that the last one is a "negated exclusion rule",
> > > +                * for the excludes to match this pathspec, it needs to not
> > > +                * match the negated exclusion.
> > > +                */
> > > +               int retval = (item->magic & PATHSPEC_EXCLUDE) ? 0 : 1;
> > >
> > > and then change all the "return 1" statements to "return retval".
> >
> > Hmm, but then wouldn't something like `git add ignored :^ignored` also
> > trigger the warning, as we see 'ignored' first, and return 1 before
> > seeing ':^ignored'?
>
> Oh, right, whoops.  Do you want to add this testcase, and a commit
> message for this (and maybe a comment explaining the double loop)?

Sure, I can do that :)

Another thought that came to my mind is that this solution doesn't
cover more complex cases with other magic patterns and/or wildcards.
For example, we would still get the warning about trying to add
'ignored' when running `git add ignored ':^*ignored'`.

But that's because we are not fully matching the pathspecs to display
the ignored files warning, only using the simplified logic from
exclude_matches_pathspec(). We could perhaps use match_pathspec(), but
then I'm not sure how we would handle something like `git add
dir/file`, where `dir` is ignored, without having to recurse into the
ignored dir...