git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* gitignore + commit with excludes = bug
@ 2021-05-01 18:37 Paul Jackson
  2021-05-04 12:55 ` Philip Oakley
  0 siblings, 1 reply; 9+ messages in thread
From: Paul Jackson @ 2021-05-01 18:37 UTC (permalink / raw)
  To: git

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".

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: gitignore + commit with excludes = bug
  2021-05-01 18:37 gitignore + commit with excludes = bug Paul Jackson
@ 2021-05-04 12:55 ` Philip Oakley
  2021-05-04 14:06   ` Matheus Tavares
  0 siblings, 1 reply; 9+ messages in thread
From: Philip Oakley @ 2021-05-04 12:55 UTC (permalink / raw)
  To: Paul Jackson, git

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





^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: gitignore + commit with excludes = bug
  2021-05-04 12:55 ` Philip Oakley
@ 2021-05-04 14:06   ` Matheus Tavares
  2021-05-09  6:21     ` Paul Jackson
  2021-05-10 19:27     ` Elijah Newren
  0 siblings, 2 replies; 9+ messages in thread
From: Matheus Tavares @ 2021-05-04 14:06 UTC (permalink / raw)
  To: philipoakley; +Cc: git, mailnew4ster

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.)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: gitignore + commit with excludes = bug
  2021-05-04 14:06   ` Matheus Tavares
@ 2021-05-09  6:21     ` Paul Jackson
  2021-05-10 16:20       ` Philip Oakley
  2021-05-10 19:27     ` Elijah Newren
  1 sibling, 1 reply; 9+ messages in thread
From: Paul Jackson @ 2021-05-09  6:21 UTC (permalink / raw)
  To: Matheus Tavares; +Cc: philipoakley, git

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.)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: gitignore + commit with excludes = bug
  2021-05-09  6:21     ` Paul Jackson
@ 2021-05-10 16:20       ` Philip Oakley
  0 siblings, 0 replies; 9+ messages in thread
From: Philip Oakley @ 2021-05-10 16:20 UTC (permalink / raw)
  To: Paul Jackson, Matheus Tavares; +Cc: git



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.)


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: gitignore + commit with excludes = bug
  2021-05-04 14:06   ` Matheus Tavares
  2021-05-09  6:21     ` Paul Jackson
@ 2021-05-10 19:27     ` Elijah Newren
  2021-05-10 19:37       ` Matheus Tavares Bernardino
  1 sibling, 1 reply; 9+ messages in thread
From: Elijah Newren @ 2021-05-10 19:27 UTC (permalink / raw)
  To: Matheus Tavares; +Cc: Philip Oakley, Git Mailing List, mailnew4ster

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.)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: gitignore + commit with excludes = bug
  2021-05-10 19:27     ` Elijah Newren
@ 2021-05-10 19:37       ` Matheus Tavares Bernardino
  2021-05-10 22:34         ` Elijah Newren
  0 siblings, 1 reply; 9+ messages in thread
From: Matheus Tavares Bernardino @ 2021-05-10 19:37 UTC (permalink / raw)
  To: Elijah Newren; +Cc: Philip Oakley, Git Mailing List, mailnew4ster

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'?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: gitignore + commit with excludes = bug
  2021-05-10 19:37       ` Matheus Tavares Bernardino
@ 2021-05-10 22:34         ` Elijah Newren
  2021-05-11  2:23           ` Matheus Tavares Bernardino
  0 siblings, 1 reply; 9+ messages in thread
From: Elijah Newren @ 2021-05-10 22:34 UTC (permalink / raw)
  To: Matheus Tavares Bernardino; +Cc: Philip Oakley, Git Mailing List, mailnew4ster

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)?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: gitignore + commit with excludes = bug
  2021-05-10 22:34         ` Elijah Newren
@ 2021-05-11  2:23           ` Matheus Tavares Bernardino
  0 siblings, 0 replies; 9+ messages in thread
From: Matheus Tavares Bernardino @ 2021-05-11  2:23 UTC (permalink / raw)
  To: Elijah Newren; +Cc: Philip Oakley, Git Mailing List, mailnew4ster

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...

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-05-11  2:23 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-01 18:37 gitignore + commit with excludes = bug Paul Jackson
2021-05-04 12:55 ` Philip Oakley
2021-05-04 14:06   ` Matheus Tavares
2021-05-09  6:21     ` Paul Jackson
2021-05-10 16:20       ` Philip Oakley
2021-05-10 19:27     ` Elijah Newren
2021-05-10 19:37       ` Matheus Tavares Bernardino
2021-05-10 22:34         ` Elijah Newren
2021-05-11  2:23           ` Matheus Tavares Bernardino

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git