Re: [PATCH v2 6/7] add: warn when pathspec only matches SKIP_WORKTREE entries

[Elijah Newren <newren@gmail.com>]

On Thu, Mar 4, 2021 at 7:23 AM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>
> Hi, Elijah
>
> I was going to send v3 a couple days ago, but there is still one issue
> that I'm having a bit of trouble with:
>
> On Wed, Feb 24, 2021 at 3:50 AM Elijah Newren <newren@gmail.com> wrote:
> >
> > Hmm...here's an interesting command sequence:
> >
> > git init lame
> > cd lame
> > mkdir baz
> > touch baz/tracked
> > git add baz/tracked
> > git update-index --skip-worktree baz/tracked
> > rm baz/tracked.  # But leave the empty directory!
> > echo baz >.gitignore
> > git add --ignore-missing --dry-run baz
> >
> >
> > Reports the following:
> > """
> > The following pathspecs only matched index entries outside the current
> > sparse checkout:
> > baz
> > hint: Disable or modify the sparsity rules if you intend to update such entries.
> > hint: Disable this message with "git config advice.updateSparsePath false"
> > The following paths are ignored by one of your .gitignore files:
> > baz
> > hint: Use -f if you really want to add them.
> > hint: Turn this message off by running
> > hint: "git config advice.addIgnoredFile false"
> > """
> >
> > That's probably okay because it does match both, but the "only
> > matched" in the first message followed by saying it matched something
> > else seems a little surprising at first.
>
> After giving this a little more thought, I got to this idea [1]. With
> this patch, git-add doesn't warn about sparse paths that are inside
> ignored dirs (it only warns about the ignored dirs themselves). If
> --force is used, then git-add recurses into the ignored directories, and
> warns about any sparse path inside them. I think this is more consistent
> with the way in which ignored dirs (and their contents) are handled with
> and without --force. And we avoid the double warning.
>
> But there is a performance penalty. The relevant code is:
>
> @@ -2029,19 +2033,23 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
>                        PATHSPEC_ICASE |
>                        PATHSPEC_EXCLUDE);
>
> +       assert(matched_ignored);
> +
>         for (i = 0; i < pathspec->nr; i++) {
>                 const struct pathspec_item *item = &pathspec->items[i];
>                 int len = item->nowildcard_len;
>
> -               if (len == pathlen &&
> -                   !ps_strncmp(item, item->match, path, pathlen))
> -                       return 1;
> -               if (len > pathlen &&
> -                   item->match[pathlen] == '/' &&
> -                   !ps_strncmp(item, item->match, path, pathlen))
> -                       return 1;
> +               if (matched && matched_ignored[i])
> +                       continue;
> +
> +               if ((len == pathlen ||
> +                   (len > pathlen && item->match[pathlen] == '/')) &&
> +                   !ps_strncmp(item, item->match, path, pathlen)) {
> +                       matched = 1;
> +                       matched_ignored[i] = 1;
> +               }
>         }
> -       return 0;
> +       return matched;
>  }
>
> This function is called for each ignored path (not recursing into
> completely ignored dirs, though). Before the patch, the internal loop
> stops at the first pathspec match. But with the patch, the loop must
> continue as we need to know about all pathspecs that had matches with
> the ignored paths...
>
> And there is also one odd case:
> $ mkdir d
> $ touch d/s d/i
> $ git add d/s
> $ git update-index --skip-worktree d/s
> $ echo d/i >.gitinore
> $ git add 'd/[is]'
>
> This outputs:
> The following pathspecs only matched index entries outside the current
> sparse checkout:
> d/[is]
> (and no warning on ignored paths).
>
> I don't think the warning here is 100% correct, as 'd/[is]' *does* match
> other files, only they are ignored. And git-add only warns about an
> ignored path when the pathspec is an exact match with it (no glob
> patterns) or inside it.
>
> So, I don't know... Perhaps should we just rephrase the sparse warning
> to remove the "only" part (and don't try to avoid the double warnings)?
> I'm open to any suggestions on alternative wordings or ideas :)
>
> [1]: https://github.com/matheustavares/git/commit/5d4be321e1f69cb08b0666ffd3f8fa658e9f6953

The only thing that I think was problematic about the double warning
was the contradiction between them due to the use of "only" in the
first; if we remove that, I think printing two warnings is perfectly
fine.  So, I'm in favor of just rephrasing as you suggest.

I think it's kind of lame that git-add won't provide a warning for
ignored files when the match is via glob/pattern; if git-add didn't
end up adding anything, but an ignored file was matched, I think a
warning is appropriate.  However, I don't think your patch series
needs to address that; your series addressing sparse behavior is long
enough, and fixing the handling with ignored files could be done
separately.