{"thread":{"id":"55605","subject":"gitignore + commit with excludes = bug","startedAt":"2021-05-01T18:38:07Z","lastAt":"2021-05-11T02:23:32Z","messageCount":9,"participants":["Paul Jackson","Philip Oakley","Matheus Tavares","Elijah Newren","Matheus Tavares Bernardino"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"423420","messageId":"CAGKR6vKwojz8_bs9oRasQPCamZJ-4mrmBKBZQCJKhX=8w+SzDg@mail.gmail.com","threadId":"55605","inReplyTo":null,"subject":"gitignore + commit with excludes = bug","fromName":"Paul Jackson","fromEmail":"mailnew4ster@gmail.com","sentAt":"2021-05-01T18:37:52Z","receivedAt":"2021-05-01T18:38:07Z","isPatch":false,"sender":{"key":"mailnew4ster@gmail.com","avatar":null},"body":"Hello,\n\nI stumbled upon what I believe is a bug in git.\nSee the following reproduction steps:\n\nmkdir test\ncd test\ngit init\necho 1 > ignored\necho 1 > not-ignored\necho ignored > .gitignore\ngit add -A -- ':!ignored' || echo 'ERROR!!!'\n\nIn these steps, I ignore the \"ignored\" file twice - first time in\n.gitignore, and second time in the \"git add\" command. I didn't expect\nthis to be a problem, but I'm getting the following error message:\n\nThe following paths are ignored by one of your .gitignore files:\nignored\n\nIt looks as if git thinks I wanted to include, not exclude \"ignored\"\nin \"git add\".\n"},{"id":"423575","messageId":"e4c3d482-03b4-141c-e47b-cb3e175da3ab@iee.email","threadId":"55605","inReplyTo":"CAGKR6vKwojz8_bs9oRasQPCamZJ-4mrmBKBZQCJKhX=8w+SzDg@mail.gmail.com","subject":"Re: gitignore + commit with excludes = bug","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-05-04T12:55:26Z","receivedAt":"2021-05-04T12:55:29Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi Paul,\n\nOn 01/05/2021 19:37, Paul Jackson wrote:\n> Hello,\n>\n> I stumbled upon what I believe is a bug in git.\n> See the following reproduction steps:\n>\n> mkdir test\n> cd test\n> git init\n> echo 1 > ignored\n> echo 1 > not-ignored\n> echo ignored > .gitignore\n> git add -A -- ':!ignored' || echo 'ERROR!!!'\n>\n> In these steps, I ignore the \"ignored\" file twice - first time in\n> .gitignore, and second time in the \"git add\" command. I didn't expect\n> this to be a problem, but I'm getting the following error message:\n>\n> The following paths are ignored by one of your .gitignore files:\n> ignored\n>\n> It looks as if git thinks I wanted to include, not exclude \"ignored\"\n> in \"git add\".\nI was intrigued by this. The man pages can be hard to decipher, and it\nmay be an 'as designed' feature, but still not intuitive\n\nIt took a while to find the relevant parts of the man pages.\n\nThe `-A` option of `add` is under\nhttps://git-scm.com/docs/git-add#Documentation/git-add.txt---no-ignore-removal\nwhich has caveats for whether a pathspec is given.\n\nThe `exclude` magic pathspec is under\nhttps://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-exclude\nand again has caveats and a double negative regarding whether the\n`exclude` pathspec counts as a path spec.\n\nI _think_ that it is saying that the `exclude` pathspec is ignored for\nthe purpose of the `-A` (all) condition for git add.\nThe `git add` then decides that the the set of files to be added is just\nthe single 'not-ignored' file, having already used the `.gitignore` to\n\"exclude\" the 'ignored' file from that set of files to add.\n\nNow your \"exclude\" pathspec comes into play and tries remove the named\n\"exclude\" file from the set, and doesn't find it. Or at least that's how\nI read it as a plausible explanation. That said, I haven't looked at the\ncode, so that could be all wrong.\n\nA possible place to start is the\nhttps://gitlab.cecs.anu.edu.au/comp8440/git/commit/ef79b1f8704668a6cdf4278f9255e03ca785bfb4\npatch (google..) or\nhttps://github.com/git/git/commit/ef79b1f8704668a6cdf4278f9255e03ca785bfb4\n\nAlso locate where the error message is generated. It maybe just a warning.\n\n--\nPhilip\n\n\n\n\n"},{"id":"423576","messageId":"789bcc9cd074cb94f706b9d432e29b52bf6a33c3.1620134775.git.matheus.bernardino@usp.br","threadId":"55605","inReplyTo":"e4c3d482-03b4-141c-e47b-cb3e175da3ab@iee.email","subject":"Re: gitignore + commit with excludes = bug","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-04T14:06:53Z","receivedAt":"2021-05-04T14:07:03Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Paul and Philip\n\nOn Tue, May 4, 2021 at 9:55 AM Philip Oakley <philipoakley@iee.email> wrote:\n>\n> Hi Paul,\n>\n> On 01/05/2021 19:37, Paul Jackson wrote:\n> > Hello,\n> >\n> > I stumbled upon what I believe is a bug in git.\n> > See the following reproduction steps:\n> >\n> > mkdir test\n> > cd test\n> > git init\n> > echo 1 > ignored\n> > echo 1 > not-ignored\n> > echo ignored > .gitignore\n> > git add -A -- ':!ignored' || echo 'ERROR!!!'\n> >\n> > In these steps, I ignore the \"ignored\" file twice - first time in\n> > .gitignore, and second time in the \"git add\" command. I didn't expect\n> > this to be a problem, but I'm getting the following error message:\n> >\n> > The following paths are ignored by one of your .gitignore files:\n> > ignored\n> >\n> > It looks as if git thinks I wanted to include, not exclude \"ignored\"\n> > in \"git add\".\n> I was intrigued by this. The man pages can be hard to decipher, and it\n> may be an 'as designed' feature, but still not intuitive\n>\n> It took a while to find the relevant parts of the man pages.\n>\n> The `-A` option of `add` is under\n> https://git-scm.com/docs/git-add#Documentation/git-add.txt---no-ignore-removal\n> which has caveats for whether a pathspec is given.\n>\n> The `exclude` magic pathspec is under\n> https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-exclude\n> and again has caveats and a double negative regarding whether the\n> `exclude` pathspec counts as a path spec.\n>\n> I _think_ that it is saying that the `exclude` pathspec is ignored for\n> the purpose of the `-A` (all) condition for git add.\n\nHmm, I think the issue is not really related to `-A`. In fact, if we\nreproduce Paul's original example without `-A`, we still get the\nwarning.\n\nThe problem seems to be at `dir.c:exclude_matches_pathspec()`, which\ncreates the list of ignored files that is later used by `git add` to\npresented the \"The following paths are ignored...\" warning.\n\nThis function ignores the `exclude` magic, so a path 'x' incorrectly\nmatches both ':x' and ':!x'. And thus, we end up warning the user about\n'x' being ignored even when the user had ran `git add ':!x'`.\n\nI think something like this, might solve the problem:\n\ndiff --git a/dir.c b/dir.c\nindex 3474e67e8f..165ca6a543 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2042,6 +2042,25 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\n \t\tint len = item->nowildcard_len;\n \n+\t\tif (!(item->magic & PATHSPEC_EXCLUDE))\n+\t\t\tcontinue;\n+\n+\t\tif (len == pathlen &&\n+\t\t    !ps_strncmp(item, item->match, path, pathlen))\n+\t\t\treturn 0;\n+\t\tif (len > pathlen &&\n+\t\t    item->match[pathlen] == '/' &&\n+\t\t    !ps_strncmp(item, item->match, path, pathlen))\n+\t\t\treturn 0;\n+\t}\n+\n+\tfor (i = 0; i < pathspec->nr; i++) {\n+\t\tconst struct pathspec_item *item = &pathspec->items[i];\n+\t\tint len = item->nowildcard_len;\n+\n+\t\tif (item->magic & PATHSPEC_EXCLUDE)\n+\t\t\tcontinue;\n+\n \t\tif (len == pathlen &&\n \t\t    !ps_strncmp(item, item->match, path, pathlen))\n \t\t\treturn 1;\n---\n\nI had to split the original loop into two and handle the `exclude`\npathspecs first because we cannot let the original loop return early\nwhen one of the `non-exclude` pathspecs matches the path. Otherwise, we\nwould still incorrectly warn the user on executions like\n`git add ignored ':!ignored'`.\n\n(We might also want to extract the matching part to its own function to\navoid repeating this code on the two loops.)\n"},{"id":"423953","messageId":"CAGKR6vLqXpdSr_boKR_V5SMqkACt9BSBa2UA6rW46GvcymjXtg@mail.gmail.com","threadId":"55605","inReplyTo":"789bcc9cd074cb94f706b9d432e29b52bf6a33c3.1620134775.git.matheus.bernardino@usp.br","subject":"Re: gitignore + commit with excludes = bug","fromName":"Paul Jackson","fromEmail":"mailnew4ster@gmail.com","sentAt":"2021-05-09T06:21:28Z","receivedAt":"2021-05-09T06:21:42Z","isPatch":false,"sender":{"key":"mailnew4ster@gmail.com","avatar":null},"body":"Thanks, I hope it gets merged at some point.\n\nOn Tue, May 4, 2021 at 5:07 PM Matheus Tavares\n<matheus.bernardino@usp.br> wrote:\n>\n> Hi, Paul and Philip\n>\n> On Tue, May 4, 2021 at 9:55 AM Philip Oakley <philipoakley@iee.email> wrote:\n> >\n> > Hi Paul,\n> >\n> > On 01/05/2021 19:37, Paul Jackson wrote:\n> > > Hello,\n> > >\n> > > I stumbled upon what I believe is a bug in git.\n> > > See the following reproduction steps:\n> > >\n> > > mkdir test\n> > > cd test\n> > > git init\n> > > echo 1 > ignored\n> > > echo 1 > not-ignored\n> > > echo ignored > .gitignore\n> > > git add -A -- ':!ignored' || echo 'ERROR!!!'\n> > >\n> > > In these steps, I ignore the \"ignored\" file twice - first time in\n> > > .gitignore, and second time in the \"git add\" command. I didn't expect\n> > > this to be a problem, but I'm getting the following error message:\n> > >\n> > > The following paths are ignored by one of your .gitignore files:\n> > > ignored\n> > >\n> > > It looks as if git thinks I wanted to include, not exclude \"ignored\"\n> > > in \"git add\".\n> > I was intrigued by this. The man pages can be hard to decipher, and it\n> > may be an 'as designed' feature, but still not intuitive\n> >\n> > It took a while to find the relevant parts of the man pages.\n> >\n> > The `-A` option of `add` is under\n> > https://git-scm.com/docs/git-add#Documentation/git-add.txt---no-ignore-removal\n> > which has caveats for whether a pathspec is given.\n> >\n> > The `exclude` magic pathspec is under\n> > https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-exclude\n> > and again has caveats and a double negative regarding whether the\n> > `exclude` pathspec counts as a path spec.\n> >\n> > I _think_ that it is saying that the `exclude` pathspec is ignored for\n> > the purpose of the `-A` (all) condition for git add.\n>\n> Hmm, I think the issue is not really related to `-A`. In fact, if we\n> reproduce Paul's original example without `-A`, we still get the\n> warning.\n>\n> The problem seems to be at `dir.c:exclude_matches_pathspec()`, which\n> creates the list of ignored files that is later used by `git add` to\n> presented the \"The following paths are ignored...\" warning.\n>\n> This function ignores the `exclude` magic, so a path 'x' incorrectly\n> matches both ':x' and ':!x'. And thus, we end up warning the user about\n> 'x' being ignored even when the user had ran `git add ':!x'`.\n>\n> I think something like this, might solve the problem:\n>\n> diff --git a/dir.c b/dir.c\n> index 3474e67e8f..165ca6a543 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2042,6 +2042,25 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n>                 const struct pathspec_item *item = &pathspec->items[i];\n>                 int len = item->nowildcard_len;\n>\n> +               if (!(item->magic & PATHSPEC_EXCLUDE))\n> +                       continue;\n> +\n> +               if (len == pathlen &&\n> +                   !ps_strncmp(item, item->match, path, pathlen))\n> +                       return 0;\n> +               if (len > pathlen &&\n> +                   item->match[pathlen] == '/' &&\n> +                   !ps_strncmp(item, item->match, path, pathlen))\n> +                       return 0;\n> +       }\n> +\n> +       for (i = 0; i < pathspec->nr; i++) {\n> +               const struct pathspec_item *item = &pathspec->items[i];\n> +               int len = item->nowildcard_len;\n> +\n> +               if (item->magic & PATHSPEC_EXCLUDE)\n> +                       continue;\n> +\n>                 if (len == pathlen &&\n>                     !ps_strncmp(item, item->match, path, pathlen))\n>                         return 1;\n> ---\n>\n> I had to split the original loop into two and handle the `exclude`\n> pathspecs first because we cannot let the original loop return early\n> when one of the `non-exclude` pathspecs matches the path. Otherwise, we\n> would still incorrectly warn the user on executions like\n> `git add ignored ':!ignored'`.\n>\n> (We might also want to extract the matching part to its own function to\n> avoid repeating this code on the two loops.)\n"},{"id":"424056","messageId":"cb6c20cf-6461-3ef6-7a2e-cb98b7b05b1f@iee.email","threadId":"55605","inReplyTo":"CAGKR6vLqXpdSr_boKR_V5SMqkACt9BSBa2UA6rW46GvcymjXtg@mail.gmail.com","subject":"Re: gitignore + commit with excludes = bug","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-05-10T16:20:19Z","receivedAt":"2021-05-10T16:20:23Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"\n\nOn 09/05/2021 07:21, Paul Jackson wrote:\n> Thanks, I hope it gets merged at some point.\n\nAre you able to pull it together as a formal patch, with a suitable\ncommit message, so that it can be reviewed and merged?\n\nPhilip\n\n>\n> On Tue, May 4, 2021 at 5:07 PM Matheus Tavares\n> <matheus.bernardino@usp.br> wrote:\n>> Hi, Paul and Philip\n>>\n>> On Tue, May 4, 2021 at 9:55 AM Philip Oakley <philipoakley@iee.email> wrote:\n>>> Hi Paul,\n>>>\n>>> On 01/05/2021 19:37, Paul Jackson wrote:\n>>>> Hello,\n>>>>\n>>>> I stumbled upon what I believe is a bug in git.\n>>>> See the following reproduction steps:\n>>>>\n>>>> mkdir test\n>>>> cd test\n>>>> git init\n>>>> echo 1 > ignored\n>>>> echo 1 > not-ignored\n>>>> echo ignored > .gitignore\n>>>> git add -A -- ':!ignored' || echo 'ERROR!!!'\n>>>>\n>>>> In these steps, I ignore the \"ignored\" file twice - first time in\n>>>> .gitignore, and second time in the \"git add\" command. I didn't expect\n>>>> this to be a problem, but I'm getting the following error message:\n>>>>\n>>>> The following paths are ignored by one of your .gitignore files:\n>>>> ignored\n>>>>\n>>>> It looks as if git thinks I wanted to include, not exclude \"ignored\"\n>>>> in \"git add\".\n>>> I was intrigued by this. The man pages can be hard to decipher, and it\n>>> may be an 'as designed' feature, but still not intuitive\n>>>\n>>> It took a while to find the relevant parts of the man pages.\n>>>\n>>> The `-A` option of `add` is under\n>>> https://git-scm.com/docs/git-add#Documentation/git-add.txt---no-ignore-removal\n>>> which has caveats for whether a pathspec is given.\n>>>\n>>> The `exclude` magic pathspec is under\n>>> https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-exclude\n>>> and again has caveats and a double negative regarding whether the\n>>> `exclude` pathspec counts as a path spec.\n>>>\n>>> I _think_ that it is saying that the `exclude` pathspec is ignored for\n>>> the purpose of the `-A` (all) condition for git add.\n>> Hmm, I think the issue is not really related to `-A`. In fact, if we\n>> reproduce Paul's original example without `-A`, we still get the\n>> warning.\n>>\n>> The problem seems to be at `dir.c:exclude_matches_pathspec()`, which\n>> creates the list of ignored files that is later used by `git add` to\n>> presented the \"The following paths are ignored...\" warning.\n>>\n>> This function ignores the `exclude` magic, so a path 'x' incorrectly\n>> matches both ':x' and ':!x'. And thus, we end up warning the user about\n>> 'x' being ignored even when the user had ran `git add ':!x'`.\n>>\n>> I think something like this, might solve the problem:\n>>\n>> diff --git a/dir.c b/dir.c\n>> index 3474e67e8f..165ca6a543 100644\n>> --- a/dir.c\n>> +++ b/dir.c\n>> @@ -2042,6 +2042,25 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n>>                 const struct pathspec_item *item = &pathspec->items[i];\n>>                 int len = item->nowildcard_len;\n>>\n>> +               if (!(item->magic & PATHSPEC_EXCLUDE))\n>> +                       continue;\n>> +\n>> +               if (len == pathlen &&\n>> +                   !ps_strncmp(item, item->match, path, pathlen))\n>> +                       return 0;\n>> +               if (len > pathlen &&\n>> +                   item->match[pathlen] == '/' &&\n>> +                   !ps_strncmp(item, item->match, path, pathlen))\n>> +                       return 0;\n>> +       }\n>> +\n>> +       for (i = 0; i < pathspec->nr; i++) {\n>> +               const struct pathspec_item *item = &pathspec->items[i];\n>> +               int len = item->nowildcard_len;\n>> +\n>> +               if (item->magic & PATHSPEC_EXCLUDE)\n>> +                       continue;\n>> +\n>>                 if (len == pathlen &&\n>>                     !ps_strncmp(item, item->match, path, pathlen))\n>>                         return 1;\n>> ---\n>>\n>> I had to split the original loop into two and handle the `exclude`\n>> pathspecs first because we cannot let the original loop return early\n>> when one of the `non-exclude` pathspecs matches the path. Otherwise, we\n>> would still incorrectly warn the user on executions like\n>> `git add ignored ':!ignored'`.\n>>\n>> (We might also want to extract the matching part to its own function to\n>> avoid repeating this code on the two loops.)\n\n"},{"id":"424071","messageId":"CABPp-BFpeoENLADH-e6dCc0PyEVQyHueY6PV8AZYbD+a3iceXA@mail.gmail.com","threadId":"55605","inReplyTo":"789bcc9cd074cb94f706b9d432e29b52bf6a33c3.1620134775.git.matheus.bernardino@usp.br","subject":"Re: gitignore + commit with excludes = bug","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-05-10T19:27:28Z","receivedAt":"2021-05-10T19:27:43Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 4, 2021 at 7:10 AM Matheus Tavares\n<matheus.bernardino@usp.br> wrote:\n>\n> Hi, Paul and Philip\n>\n> On Tue, May 4, 2021 at 9:55 AM Philip Oakley <philipoakley@iee.email> wrote:\n> >\n> > Hi Paul,\n> >\n> > On 01/05/2021 19:37, Paul Jackson wrote:\n> > > Hello,\n> > >\n> > > I stumbled upon what I believe is a bug in git.\n> > > See the following reproduction steps:\n> > >\n> > > mkdir test\n> > > cd test\n> > > git init\n> > > echo 1 > ignored\n> > > echo 1 > not-ignored\n> > > echo ignored > .gitignore\n> > > git add -A -- ':!ignored' || echo 'ERROR!!!'\n> > >\n> > > In these steps, I ignore the \"ignored\" file twice - first time in\n> > > .gitignore, and second time in the \"git add\" command. I didn't expect\n> > > this to be a problem, but I'm getting the following error message:\n> > >\n> > > The following paths are ignored by one of your .gitignore files:\n> > > ignored\n> > >\n> > > It looks as if git thinks I wanted to include, not exclude \"ignored\"\n> > > in \"git add\".\n> > I was intrigued by this. The man pages can be hard to decipher, and it\n> > may be an 'as designed' feature, but still not intuitive\n> >\n> > It took a while to find the relevant parts of the man pages.\n> >\n> > The `-A` option of `add` is under\n> > https://git-scm.com/docs/git-add#Documentation/git-add.txt---no-ignore-removal\n> > which has caveats for whether a pathspec is given.\n> >\n> > The `exclude` magic pathspec is under\n> > https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-exclude\n> > and again has caveats and a double negative regarding whether the\n> > `exclude` pathspec counts as a path spec.\n> >\n> > I _think_ that it is saying that the `exclude` pathspec is ignored for\n> > the purpose of the `-A` (all) condition for git add.\n>\n> Hmm, I think the issue is not really related to `-A`. In fact, if we\n> reproduce Paul's original example without `-A`, we still get the\n> warning.\n>\n> The problem seems to be at `dir.c:exclude_matches_pathspec()`, which\n> creates the list of ignored files that is later used by `git add` to\n> presented the \"The following paths are ignored...\" warning.\n>\n> This function ignores the `exclude` magic, so a path 'x' incorrectly\n> matches both ':x' and ':!x'. And thus, we end up warning the user about\n> 'x' being ignored even when the user had ran `git add ':!x'`.\n>\n> I think something like this, might solve the problem:\n>\n> diff --git a/dir.c b/dir.c\n> index 3474e67e8f..165ca6a543 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2042,6 +2042,25 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n>                 const struct pathspec_item *item = &pathspec->items[i];\n>                 int len = item->nowildcard_len;\n>\n> +               if (!(item->magic & PATHSPEC_EXCLUDE))\n> +                       continue;\n> +\n> +               if (len == pathlen &&\n> +                   !ps_strncmp(item, item->match, path, pathlen))\n> +                       return 0;\n> +               if (len > pathlen &&\n> +                   item->match[pathlen] == '/' &&\n> +                   !ps_strncmp(item, item->match, path, pathlen))\n> +                       return 0;\n> +       }\n> +\n> +       for (i = 0; i < pathspec->nr; i++) {\n> +               const struct pathspec_item *item = &pathspec->items[i];\n> +               int len = item->nowildcard_len;\n> +\n> +               if (item->magic & PATHSPEC_EXCLUDE)\n> +                       continue;\n> +\n>                 if (len == pathlen &&\n>                     !ps_strncmp(item, item->match, path, pathlen))\n>                         return 1;\n> ---\n\nThanks for tracking this down.  Your analysis and patch look correct\nto me, but perhaps we could simplify the code a bit.  Instead of\nlooping twice, perhaps insert the following code above the if-checks:\n\n+               /*\n+                * We can have a bunch of exclusion rules:\n+                *    .gitignore\n+                *    *.log\n+                *    !settings.log\n+                * and we're trying to see if the path matches one of these,\n+                * but note that the last one is a \"negated exclusion rule\",\n+                * for the excludes to match this pathspec, it needs to not\n+                * match the negated exclusion.\n+                */\n+               int retval = (item->magic & PATHSPEC_EXCLUDE) ? 0 : 1;\n\nand then change all the \"return 1\" statements to \"return retval\".\n\nThoughts?\n\n>\n> I had to split the original loop into two and handle the `exclude`\n> pathspecs first because we cannot let the original loop return early\n> when one of the `non-exclude` pathspecs matches the path. Otherwise, we\n> would still incorrectly warn the user on executions like\n> `git add ignored ':!ignored'`.\n>\n> (We might also want to extract the matching part to its own function to\n> avoid repeating this code on the two loops.)\n"},{"id":"424073","messageId":"CAHd-oW7veyN+qetv-Mf-qPa+bcah=JCz6mSRAzunVgaFRxdbnA@mail.gmail.com","threadId":"55605","inReplyTo":"CABPp-BFpeoENLADH-e6dCc0PyEVQyHueY6PV8AZYbD+a3iceXA@mail.gmail.com","subject":"Re: gitignore + commit with excludes = bug","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-10T19:37:39Z","receivedAt":"2021-05-10T19:37:54Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, May 10, 2021 at 4:27 PM Elijah Newren <newren@gmail.com> wrote:\n>\n> Thanks for tracking this down.  Your analysis and patch look correct\n> to me, but perhaps we could simplify the code a bit.  Instead of\n> looping twice, perhaps insert the following code above the if-checks:\n>\n> +               /*\n> +                * We can have a bunch of exclusion rules:\n> +                *    .gitignore\n> +                *    *.log\n> +                *    !settings.log\n> +                * and we're trying to see if the path matches one of these,\n> +                * but note that the last one is a \"negated exclusion rule\",\n> +                * for the excludes to match this pathspec, it needs to not\n> +                * match the negated exclusion.\n> +                */\n> +               int retval = (item->magic & PATHSPEC_EXCLUDE) ? 0 : 1;\n>\n> and then change all the \"return 1\" statements to \"return retval\".\n\nHmm, but then wouldn't something like `git add ignored :^ignored` also\ntrigger the warning, as we see 'ignored' first, and return 1 before\nseeing ':^ignored'?\n"},{"id":"424082","messageId":"CABPp-BHPr0aJZTRsVPK+Ga2oKZJLa75OkcyHLpZH=sD1pfpNZg@mail.gmail.com","threadId":"55605","inReplyTo":"CAHd-oW7veyN+qetv-Mf-qPa+bcah=JCz6mSRAzunVgaFRxdbnA@mail.gmail.com","subject":"Re: gitignore + commit with excludes = bug","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-05-10T22:34:20Z","receivedAt":"2021-05-10T22:34:35Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, May 10, 2021 at 12:37 PM Matheus Tavares Bernardino\n<matheus.bernardino@usp.br> wrote:\n>\n> On Mon, May 10, 2021 at 4:27 PM Elijah Newren <newren@gmail.com> wrote:\n> >\n> > Thanks for tracking this down.  Your analysis and patch look correct\n> > to me, but perhaps we could simplify the code a bit.  Instead of\n> > looping twice, perhaps insert the following code above the if-checks:\n> >\n> > +               /*\n> > +                * We can have a bunch of exclusion rules:\n> > +                *    .gitignore\n> > +                *    *.log\n> > +                *    !settings.log\n> > +                * and we're trying to see if the path matches one of these,\n> > +                * but note that the last one is a \"negated exclusion rule\",\n> > +                * for the excludes to match this pathspec, it needs to not\n> > +                * match the negated exclusion.\n> > +                */\n> > +               int retval = (item->magic & PATHSPEC_EXCLUDE) ? 0 : 1;\n> >\n> > and then change all the \"return 1\" statements to \"return retval\".\n>\n> Hmm, but then wouldn't something like `git add ignored :^ignored` also\n> trigger the warning, as we see 'ignored' first, and return 1 before\n> seeing ':^ignored'?\n\nOh, right, whoops.  Do you want to add this testcase, and a commit\nmessage for this (and maybe a comment explaining the double loop)?\n"},{"id":"424090","messageId":"CAHd-oW5PFTuUQsxTWCQqdSDBskq-+zkCc2OqyY9wJEUqNUSvkw@mail.gmail.com","threadId":"55605","inReplyTo":"CABPp-BHPr0aJZTRsVPK+Ga2oKZJLa75OkcyHLpZH=sD1pfpNZg@mail.gmail.com","subject":"Re: gitignore + commit with excludes = bug","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-11T02:23:17Z","receivedAt":"2021-05-11T02:23:32Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, May 10, 2021 at 7:34 PM Elijah Newren <newren@gmail.com> wrote:\n>\n> On Mon, May 10, 2021 at 12:37 PM Matheus Tavares Bernardino\n> <matheus.bernardino@usp.br> wrote:\n> >\n> > On Mon, May 10, 2021 at 4:27 PM Elijah Newren <newren@gmail.com> wrote:\n> > >\n> > > Thanks for tracking this down.  Your analysis and patch look correct\n> > > to me, but perhaps we could simplify the code a bit.  Instead of\n> > > looping twice, perhaps insert the following code above the if-checks:\n> > >\n> > > +               /*\n> > > +                * We can have a bunch of exclusion rules:\n> > > +                *    .gitignore\n> > > +                *    *.log\n> > > +                *    !settings.log\n> > > +                * and we're trying to see if the path matches one of these,\n> > > +                * but note that the last one is a \"negated exclusion rule\",\n> > > +                * for the excludes to match this pathspec, it needs to not\n> > > +                * match the negated exclusion.\n> > > +                */\n> > > +               int retval = (item->magic & PATHSPEC_EXCLUDE) ? 0 : 1;\n> > >\n> > > and then change all the \"return 1\" statements to \"return retval\".\n> >\n> > Hmm, but then wouldn't something like `git add ignored :^ignored` also\n> > trigger the warning, as we see 'ignored' first, and return 1 before\n> > seeing ':^ignored'?\n>\n> Oh, right, whoops.  Do you want to add this testcase, and a commit\n> message for this (and maybe a comment explaining the double loop)?\n\nSure, I can do that :)\n\nAnother thought that came to my mind is that this solution doesn't\ncover more complex cases with other magic patterns and/or wildcards.\nFor example, we would still get the warning about trying to add\n'ignored' when running `git add ignored ':^*ignored'`.\n\nBut that's because we are not fully matching the pathspecs to display\nthe ignored files warning, only using the simplified logic from\nexclude_matches_pathspec(). We could perhaps use match_pathspec(), but\nthen I'm not sure how we would handle something like `git add\ndir/file`, where `dir` is ignored, without having to recurse into the\nignored dir...\n"}]}