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

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Feb 19, 2021, 17:11 UTC
Message-ID: <CAHd-oW6KVQeGq1aQV+sFD+XJ=cpH3GRWWHasJAZ92Sqiczjq9g@mail.gmail.com>
In-Reply-To: <xmqqo8ggc2u1.fsf@gitster.g>

On Thu, Feb 18, 2021 at 9:34 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 9 quoted lines

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

Show 11 quoted lines

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

Show 8 quoted lines

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

Show 8 quoted lines

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

Show 5 quoted lines

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