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

On Wed, Feb 24, 2021 at 3:50 AM Elijah Newren [off-list ref] wrote:

On Tue, Feb 23, 2021 at 8:05 PM Matheus Tavares
[off-list ref] wrote:

quoted

+void advise_on_updating_sparse_paths(struct string_list *pathspec_list)
+{
+       struct string_list_item *item;
+
+       if (!pathspec_list->nr)
+               return;
+
+       fprintf(stderr, _("The following pathspecs only matched index entries outside the current\n"
+                         "sparse checkout:\n"));
+       for_each_string_list_item(item, pathspec_list)
+               fprintf(stderr, "%s\n", item->string);

Was the use of fprintf(stderr, ...) because of the fact that you want
to do multiple print statements?  I'm just curious if that was the
reason for avoiding the warning() function, or if there was another
consideration at play as well.

Yes, that was one of the reasons. The other was to use the same style as
the ignored files message, which doesn't print the "warning:" prefix.
But I don't have any strong preference here, I'd be OK with using
warning() too.

quoted

-static void refresh(int verbose, const struct pathspec *pathspec)
+static int refresh(int verbose, const struct pathspec *pathspec)
 {
        char *seen;
-       int i;
+       int i, ret = 0;
+       char *skip_worktree_seen = NULL;
+       struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
+       int flags = REFRESH_DONT_MARK_SPARSE_MATCHES |
+                   (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);

        seen = xcalloc(pathspec->nr, 1);
-       refresh_index(&the_index, verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET,
-                     pathspec, seen, _("Unstaged changes after refreshing the index:"));
+       refresh_index(&the_index, flags, pathspec, seen,
+                     _("Unstaged changes after refreshing the index:"));
        for (i = 0; i < pathspec->nr; i++) {
-               if (!seen[i])
-                       die(_("pathspec '%s' did not match any files"),
-                           pathspec->items[i].original);
+               if (!seen[i]) {
+                       if (matches_skip_worktree(pathspec, i, &skip_worktree_seen)) {
+                               string_list_append(&only_match_skip_worktree,
+                                                  pathspec->items[i].original);
+                       } else {
+                               die(_("pathspec '%s' did not match any files"),
+                                   pathspec->items[i].original);
+                       }
+               }
+       }
+
+       if (only_match_skip_worktree.nr) {
+               advise_on_updating_sparse_paths(&only_match_skip_worktree);
+               ret = 1;
        }

On first reading, I missed that the code die()s if there are any
non-SKIP_WORKTREE entries matched, and that is the reason you know
that only SKIP_WORKTREE entries could have been matched for this last
if-statement.

Hmm, I may be misinterpreting your explanation, but I think the
reasoning is slightly different. The code die()s if there are _no_
matches either among sparse or dense entries. The reason why we know
that only sparse entries matched the pathspecs in this last if-statement
is because the `only_match_skip_worktree` list is only appended when a
pathspec is not marked on seen[] (dense entries only), but it is marked
on skip_worktree_seen[] (sparse entries only).

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 interesting. You can also trigger this behavior with a plain add
(i.e. without "--ignore-missing --dry-run").

Since we only get the list of ignored paths from fill_directory(), we
can't really tell whether a specific pathspec item had matches among
ignored files or not. If we had this information, we could conditionally
skip the sparse warning.

I.e. something like this (WARNING: hacky and just briefly tested):

diff --git a/builtin/add.c b/builtin/add.c
index fde6462850..90614e7e76 100644
--- a/builtin/add.c
+++ b/builtin/add.c

@@ -597,3 +597,3 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		int i;
-		char *skip_worktree_seen = NULL;
+		char *skip_worktree_seen = NULL, *ignored_seen = NULL;
 		struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;

@@ -621,3 +621,14 @@ int cmd_add(int argc, const char **argv, const char *prefix)

-			if (matches_skip_worktree(&pathspec, i, &skip_worktree_seen)) {
+			if (dir.ignored_nr) {
+				int j, prefix_len = common_prefix_len(&pathspec);
+				ignored_seen = xcalloc(pathspec.nr, 1);
+				for (j = 0; j < dir.ignored_nr; j++) {
+					dir_path_match(&the_index, dir.ignored[j],
+						       &pathspec, prefix_len,
+						       ignored_seen);
+				}
+			}
+
+			if (ignored_seen && !ignored_seen[i] &&
+			    matches_skip_worktree(&pathspec, i, &skip_worktree_seen)) {
 				string_list_append(&only_match_skip_worktree,

diff --git a/dir.c b/dir.c
index d153a63bbd..a19bc7aa0b 100644
--- a/dir.c
+++ b/dir.c

@@ -136,3 +136,3 @@ static int fnmatch_icase_mem(const char *pattern, int patternlen,

-static size_t common_prefix_len(const struct pathspec *pathspec)
+size_t common_prefix_len(const struct pathspec *pathspec)
 {

diff --git a/dir.h b/dir.h
index facfae4740..aa2d4aa71b 100644
--- a/dir.h
+++ b/dir.h

@@ -355,2 +355,3 @@ int simple_length(const char *match);
 int no_wildcard(const char *string);
+size_t common_prefix_len(const struct pathspec *pathspec);
 char *common_prefix(const struct pathspec *pathspec);

Now `git add baz` would only produce:

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"