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

From

Matheus Tavares <matheus.bernardino@usp.br>

Date

Mar 4, 2021, 15:23 UTC

Message-ID

<20210304152315.18498-1-matheus.bernardino@usp.br>

In-Reply-To

<CABPp-BEpnaR1tydJ-vcWAUYnT-TFcfCMMqwbmOz1Dx+nvsHZMQ@mail.gmail.com>

Hi, Elijah

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.

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