Re: gitignore + commit with excludes = bug

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: May 11, 2021, 02:23 UTC
Message-ID: <CAHd-oW5PFTuUQsxTWCQqdSDBskq-+zkCc2OqyY9wJEUqNUSvkw@mail.gmail.com>
In-Reply-To: <CABPp-BHPr0aJZTRsVPK+Ga2oKZJLa75OkcyHLpZH=sD1pfpNZg@mail.gmail.com>

On Mon, May 10, 2021 at 7:34 PM Elijah Newren <newren@gmail.com> wrote:

Show 30 quoted lines

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