On Thu, Feb 18, 2021 at 9:34 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Matheus Tavares <matheus.bernardino@xxxxxx> 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'.
