add: warn when asked to update SKIP_WORKTREE entries

`git add` already refrains from updating SKIP_WORKTREE entries, but it
silently exits with zero code when it is asked to do so. Instead, let's
warn the user and display a hint on how to update these entries.

Note that we only warn the user whey they give a pathspec item that
matches no eligible path for updating, but it does match one or more
SKIP_WORKTREE entries. A warning was chosen over erroring out right away
to reproduce the same behavior `add` already exhibits with ignored
files. This also allow users to continue their workflow without having
to invoke `add` again with only the eligible paths (as those will have
already been added).

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index acbd0c09aa4fb0b2146ba11f264be55e61e73c77..851b83cf3064f5ddb0f2ba8cc70ae19f2c5e6480 100644 (file)
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -119,4 +119,7 @@ advice.*::
        addEmptyPathspec::
                Advice shown if a user runs the add command without providing
                the pathspec parameter.
+       updateSparsePath::
+               Advice shown when linkgit:git-add[1] is asked to update index
+               entries outside the current sparse checkout.
 --

diff --git a/advice.c b/advice.c
index 164742305fd147c9f31fab6b127a10e1f8e0f636..0b9c89c48ab996da0e7938ac0667a3ecad925efc 100644 (file)
--- a/advice.c
+++ b/advice.c
@@ -2,6 +2,7 @@
 #include "config.h"
 #include "color.h"
 #include "help.h"
+#include "string-list.h"
 
 int advice_fetch_show_forced_updates = 1;
 int advice_push_update_rejected = 1;
@@ -136,6 +137,7 @@ static struct {
        [ADVICE_STATUS_HINTS]                           = { "statusHints", 1 },
        [ADVICE_STATUS_U_OPTION]                        = { "statusUoption", 1 },
        [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
+       [ADVICE_UPDATE_SPARSE_PATH]                     = { "updateSparsePath", 1 },
        [ADVICE_WAITING_FOR_EDITOR]                     = { "waitingForEditor", 1 },
 };
 
@@ -284,6 +286,24 @@ void NORETURN die_conclude_merge(void)
        die(_("Exiting because of unfinished merge."));
 }
 
+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 didn't match any"
+                         " eligible path, but they do match index\n"
+                         "entries outside the current sparse checkout:\n"));
+       for_each_string_list_item(item, pathspec_list)
+               fprintf(stderr, "%s\n", item->string);
+
+       advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,
+                         _("Disable or modify the sparsity rules if you intend"
+                           " to update such entries."));
+}
+
 void detach_advice(const char *new_name)
 {
        const char *fmt =

diff --git a/advice.h b/advice.h
index bc2432980a83d570ea1179ebb5d25c93e94dc238..bd26c385d00dd3af9e9bbd60812697607107b8dd 100644 (file)
--- a/advice.h
+++ b/advice.h
@@ -3,6 +3,8 @@
 
 #include "git-compat-util.h"
 
+struct string_list;
+
 extern int advice_fetch_show_forced_updates;
 extern int advice_push_update_rejected;
 extern int advice_push_non_ff_current;
@@ -71,6 +73,7 @@ extern int advice_add_empty_pathspec;
        ADVICE_STATUS_HINTS,
        ADVICE_STATUS_U_OPTION,
        ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
+       ADVICE_UPDATE_SPARSE_PATH,
        ADVICE_WAITING_FOR_EDITOR,
 };
 
@@ -92,6 +95,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...);
 int error_resolve_conflict(const char *me);
 void NORETURN die_resolve_conflict(const char *me);
 void NORETURN die_conclude_merge(void);
+void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
 
 #endif /* ADVICE_H */

diff --git a/builtin/add.c b/builtin/add.c
index 050cb8af303e43ff46060739f57f73b29b0ef386..6980311aaa6b6ef0fbcbc5b40d7aa41a40a723dd 100644 (file)
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -178,24 +178,43 @@ static char *prune_directory(struct dir_struct *dir, struct pathspec *pathspec,
        }
        dir->nr = dst - dir->entries;
        add_pathspec_matches_against_index(pathspec, &the_index, seen,
-                                          PS_HEED_SKIP_WORKTREE);
+                                          PS_IGNORE_SKIP_WORKTREE);
        return seen;
 }
 
-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_IGNORE_SKIP_WORKTREE |
+                   (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;
        }
+
        free(seen);
+       free(skip_worktree_seen);
+       string_list_clear(&only_match_skip_worktree, 0);
+       return ret;
 }
 
 int run_add_interactive(const char *revision, const char *patch_mode,
@@ -571,16 +590,18 @@ int cmd_add(int argc, const char **argv, const char *prefix)
        }
 
        if (refresh_only) {
-               refresh(verbose, &pathspec);
+               exit_status |= refresh(verbose, &pathspec);
                goto finish;
        }
 
        if (pathspec.nr) {
                int i;
+               char *skip_worktree_seen = NULL;
+               struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
 
                if (!seen)
                        seen = find_pathspecs_matching_against_index(&pathspec,
-                                       &the_index, PS_HEED_SKIP_WORKTREE);
+                                       &the_index, PS_IGNORE_SKIP_WORKTREE);
 
                /*
                 * file_exists() assumes exact match
@@ -594,12 +615,24 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
                for (i = 0; i < pathspec.nr; i++) {
                        const char *path = pathspec.items[i].match;
+
                        if (pathspec.items[i].magic & PATHSPEC_EXCLUDE)
                                continue;
-                       if (!seen[i] && path[0] &&
-                           ((pathspec.items[i].magic &
-                             (PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
-                            !file_exists(path))) {
+                       if (seen[i])
+                               continue;
+
+                       if (matches_skip_worktree(&pathspec, i, &skip_worktree_seen)) {
+                               string_list_append(&only_match_skip_worktree,
+                                                  pathspec.items[i].original);
+                               continue;
+                       }
+
+                       /* Don't complain at 'git add .' on empty repo */
+                       if (!path[0])
+                               continue;
+
+                       if ((pathspec.items[i].magic & (PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
+                           !file_exists(path)) {
                                if (ignore_missing) {
                                        int dtype = DT_UNKNOWN;
                                        if (is_excluded(&dir, &the_index, path, &dtype))
@@ -610,7 +643,16 @@ int cmd_add(int argc, const char **argv, const char *prefix)
                                            pathspec.items[i].original);
                        }
                }
+
+
+               if (only_match_skip_worktree.nr) {
+                       advise_on_updating_sparse_paths(&only_match_skip_worktree);
+                       exit_status = 1;
+               }
+
                free(seen);
+               free(skip_worktree_seen);
+               string_list_clear(&only_match_skip_worktree, 0);
        }
 
        plug_bulk_checkin();

diff --git a/pathspec.c b/pathspec.c
index 6d502e64e583f53fdefd495cb0793e5299a201a3..8247a65ec8f8188c438b13b4c978e29c469b03f8 100644 (file)
--- a/pathspec.c
+++ b/pathspec.c
@@ -62,6 +62,21 @@ char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
        return seen;
 }
 
+char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)
+{
+       struct index_state *istate = the_repository->index;
+       char *seen = xcalloc(pathspec->nr, 1);
+       int i;
+
+       for (i = 0; i < istate->cache_nr; i++) {
+               struct cache_entry *ce = istate->cache[i];
+               if (ce_skip_worktree(ce))
+                   ce_path_match(istate, ce, pathspec, seen);
+       }
+
+       return seen;
+}
+
 /*
  * Magic pathspec
  *

diff --git a/pathspec.h b/pathspec.h
index 0feb8e9f67c19bdb58687d4cf9b31527f855b7d2..5b4c6614bf6711e1bf0c1a4faa0397fea0c80a41 100644 (file)
--- a/pathspec.h
+++ b/pathspec.h
@@ -160,6 +160,14 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
                                            const struct index_state *istate,
                                            enum ps_skip_worktree_action sw_action);
+char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec);
+static inline int matches_skip_worktree(const struct pathspec *pathspec,
+                                       int item, char **seen_ptr)
+{
+       if (!*seen_ptr)
+               *seen_ptr = find_pathspecs_matching_skip_worktree(pathspec);
+       return (*seen_ptr)[item];
+}
 int match_pathspec_attrs(const struct index_state *istate,
                         const char *name, int namelen,
                         const struct pathspec_item *item);

diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
index 00b10ac877d31895ccdf3b2ae74137845cdffde7..2b1fd0d0eef004a5518111936dc8271545a190d7 100755 (executable)
--- a/t/t3705-add-sparse-checkout.sh
+++ b/t/t3705-add-sparse-checkout.sh
@@ -36,10 +36,26 @@ setup_gitignore () {
        EOF
 }
 
+test_expect_success 'setup' "
+       cat >sparse_error_header <<-EOF &&
+       The following pathspecs didn't match any eligible path, but they do match index
+       entries outside the current sparse checkout:
+       EOF
+
+       cat >sparse_hint <<-EOF &&
+       hint: Disable or modify the sparsity rules if you intend to update such entries.
+       hint: Disable this message with \"git config advice.updateSparsePath false\"
+       EOF
+
+       echo sparse_entry | cat sparse_error_header - >sparse_entry_error &&
+       cat sparse_entry_error sparse_hint >error_and_hint
+"
+
 test_expect_success 'git add does not remove sparse entries' '
        setup_sparse_entry &&
        rm sparse_entry &&
-       git add sparse_entry &&
+       test_must_fail git add sparse_entry 2>stderr &&
+       test_cmp error_and_hint stderr &&
        test_sparse_entry_unchanged
 '
 
@@ -47,7 +63,8 @@ test_expect_success 'git add -A does not remove sparse entries' '
        setup_sparse_entry &&
        rm sparse_entry &&
        setup_gitignore &&
-       git add -A &&
+       git add -A 2>stderr &&
+       test_must_be_empty stderr &&
        test_sparse_entry_unchanged
 '
 
@@ -55,7 +72,13 @@ test_expect_success 'git add . does not remove sparse entries' '
        setup_sparse_entry &&
        rm sparse_entry &&
        setup_gitignore &&
-       git add . &&
+       test_must_fail git add . 2>stderr &&
+
+       cat sparse_error_header >expect &&
+       echo . >>expect &&
+       cat sparse_hint >>expect &&
+
+       test_cmp expect stderr &&
        test_sparse_entry_unchanged
 '
 
@@ -64,7 +87,8 @@ do
        test_expect_success "git add${opt:+ $opt} does not update sparse entries" '
                setup_sparse_entry &&
                echo modified >sparse_entry &&
-               git add $opt sparse_entry &&
+               test_must_fail git add $opt sparse_entry 2>stderr &&
+               test_cmp error_and_hint stderr &&
                test_sparse_entry_unchanged
        '
 done
@@ -73,14 +97,16 @@ test_expect_success 'git add --refresh does not update sparse entries' '
        setup_sparse_entry &&
        git ls-files --debug sparse_entry | grep mtime >before &&
        test-tool chmtime -60 sparse_entry &&
-       git add --refresh sparse_entry &&
+       test_must_fail git add --refresh sparse_entry 2>stderr &&
+       test_cmp error_and_hint stderr &&
        git ls-files --debug sparse_entry | grep mtime >after &&
        test_cmp before after
 '
 
 test_expect_success 'git add --chmod does not update sparse entries' '
        setup_sparse_entry &&
-       git add --chmod=+x sparse_entry &&
+       test_must_fail git add --chmod=+x sparse_entry 2>stderr &&
+       test_cmp error_and_hint stderr &&
        test_sparse_entry_unchanged &&
        ! test -x sparse_entry
 '
@@ -89,8 +115,41 @@ test_expect_success 'git add --renormalize does not update sparse entries' '
        test_config core.autocrlf false &&
        setup_sparse_entry "LINEONE\r\nLINETWO\r\n" &&
        echo "sparse_entry text=auto" >.gitattributes &&
-       git add --renormalize sparse_entry &&
+       test_must_fail git add --renormalize sparse_entry 2>stderr &&
+       test_cmp error_and_hint stderr &&
+       test_sparse_entry_unchanged
+'
+
+test_expect_success 'git add --dry-run --ignore-missing warn on sparse path' '
+       setup_sparse_entry &&
+       rm sparse_entry &&
+       test_must_fail git add --dry-run --ignore-missing sparse_entry 2>stderr &&
+       test_cmp error_and_hint stderr &&
        test_sparse_entry_unchanged
 '
 
+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 2>stderr &&
+       grep "fatal: pathspec .nonexistent. did not match any files" stderr &&
+       ! grep -F -f sparse_error_header stderr
+'
+
+test_expect_success 'do not warn when pathspec matches dense entries' '
+       setup_sparse_entry &&
+       echo modified >sparse_entry &&
+       >dense_entry &&
+       git add "*_entry" 2>stderr &&
+       test_must_be_empty stderr &&
+       test_sparse_entry_unchanged &&
+       git ls-files --error-unmatch dense_entry
+'
+
+test_expect_success 'add obeys advice.updateSparsePath' '
+       setup_sparse_entry &&
+       test_must_fail git -c advice.updateSparsePath=false add sparse_entry 2>stderr &&
+       test_cmp sparse_entry_error stderr
+
+'
+
 test_done