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

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

On Thu, Feb 18, 2021 at 9:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > `git add` already refrains from updating SKIP_WORKTREE entries, but it
> > silently succeeds when a pathspec only matches these entries. Instead,
> > let's warn the user and display a hint on how to update these entries.
>
> "Silently succeeds" reads as if it succeeds to update, but that is
> not what you meant.

Ok, I will rephrase this section.

> I guess the warning is justified and is desirable because an attempt
> to add an ignored path would result in a similar hint, e.g.
>
>     $ echo '*~' >.gitignore
>     $ git add x~
>     hint: use -f if you really want to...
>     $ git add .
>
> It is curious why the latter does not warn (even when there is
> nothing yet to be added that is not ignored), but that is what we
> have now.

Yeah, this also happens with other directories:

    $ echo 'dir/file' >.gitignore
    $ mkdir dir
    $ touch dir/file
    $ git add dir
    <no warning>

In your previous example, `git add '*~'` and `git add '[xy]~'` also
wouldn't warn about 'x~'.  IIUC, that's because `add` uses
DIR_COLLECT_IGNORED for fill_directory(), and this option "Only
returns ignored files that match pathspec exactly (no wildcards)."

> > Note: refresh_index() was changed to only mark matches with
> > no-SKIP-WORKTREE entries in the `seen` output parameter. This is exactly
> > the behavior we want for `add`, and only `add` calls this function with
> > a non-NULL `seen` pointer. So the change brings no side effect on
> > other callers.
>
> And possible new callers that wants to learn from seen[] output
> would want the same semantics, presumably?

Hmm, TBH I haven't given much thought about new callers that might
want to use seen[]. Perhaps would it be better to implement this
behind a REFRESH_* flag?

> > +test_expect_success 'do not advice about sparse entries when they do not match the pathspec' '
> > +     setup_sparse_entry &&
> > +     test_must_fail git add nonexistent sp 2>stderr &&
> > +     test_i18ngrep "fatal: pathspec .nonexistent. did not match any files" stderr &&
> > +     test_i18ngrep ! "The following pathspecs only matched index entries" stderr
>
> This is because both of the two pathspec elements given do not match
> the sparse entries?

Oops, 'sp' should not be here. But yes, it doesn't display the advice
because the pathspec didn't match 'sparse_entry'.

> It is curious how the command behaves when
> given a pathspec that is broader, e.g. "." (aka "everything under
> the sun").  We could do "add --dry-run" for the test if we do not
> want to set up .gitignore appropriately and do not want to smudge
> the index with stderr, expect, actual etc.

I'll add a test for '.', and perhaps also another test for the case
where the pathspec matches both sparse and dense entries. For the '.'
case, I think we could use a simple .gitignore with '*' and
'!/sparse_entry'.