git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v4 6/7] add: warn when asked to update SKIP_WORKTREE entries

From
Matheus Tavares <matheus.bernardino@usp.br>
Date
Apr 8, 2021, 20:41 UTC
Message-ID
<da9cd3e9f6c10e624e46774e8b19f3caaf1ef60f.1617914011.git.matheus.bernardino@usp.br>
In-Reply-To
<cover.1617914011.git.matheus.bernardino@usp.br>

`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>
---
 Documentation/config/advice.txt |  3 ++
 advice.c                        | 20 +++++++++
 advice.h                        |  4 ++
 builtin/add.c                   | 70 ++++++++++++++++++++++++-------
 pathspec.c                      | 15 +++++++
 pathspec.h                      |  8 ++++
 t/t3705-add-sparse-checkout.sh  | 73 +++++++++++++++++++++++++++++----
 7 files changed, 172 insertions(+), 21 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index acbd0c09aa..851b83cf30 100644
--- 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 164742305f..0b9c89c48a 100644
--- 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 bc2432980a..bd26c385d0 100644
--- 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 050cb8af30..6980311aaa 100644
--- 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 8b05575110..ab67ff7c5c 100644
--- 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 0feb8e9f67..5b4c6614bf 100644
--- 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 00b10ac877..2b1fd0d0ee 100755
--- 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

-- 
2.30.1
Previous: Matheus TavaresNext: Matheus Tavares
Message 27 of 32 in “add/rm: honor sparse checkout and warn on sparse paths”
  1. 0/7 add/rm: honor sparse checkout and warn on sparse pathsMatheus Tavares, Mar 12, 2021
  2. 2/7 t3705: add tests for `git add` in sparse checkoutsMatheus Tavares, Mar 12, 2021
  3. Derrick StoleeMar 23, 2021
  4. 1/7 add: include magic part of pathspec on --refresh errorMatheus Tavares, Mar 12, 2021
  5. 4/7 pathspec: allow to ignore SKIP_WORKTREE entries on index matchingMatheus Tavares, Mar 12, 2021
  6. 5/7 refresh_index(): add REFRESH_DONT_MARK_SPARSE_MATCHES flagMatheus Tavares, Mar 12, 2021
  7. Junio C HamanoMar 18, 2021
  8. Junio C HamanoMar 19, 2021
  9. Matheus Tavares BernardinoMar 19, 2021
  10. Junio C HamanoMar 19, 2021
  11. Matheus Tavares BernardinoMar 30, 2021
  12. Elijah NewrenMar 31, 2021
  13. 3/7 add: make --chmod and --renormalize honor sparse checkoutsMatheus Tavares, Mar 12, 2021
  14. 6/7 add: warn when asked to update SKIP_WORKTREE entriesMatheus Tavares, Mar 12, 2021
  15. 7/7 rm: honor sparse checkout patternsMatheus Tavares, Mar 12, 2021
  16. Ævar Arnfjörð BjarmasonMar 21, 2021
  17. Matheus Tavares BernardinoMar 22, 2021
  18. Derrick StoleeMar 23, 2021
  19. Elijah NewrenMar 13, 2021
  20. 0/7 add/rm: honor sparse checkout and warn on sparse pathsMatheus Tavares, Apr 8, 2021
  21. 1/7 add: include magic part of pathspec on --refresh errorMatheus Tavares, Apr 8, 2021
  22. 2/7 t3705: add tests for `git add` in sparse checkoutsMatheus Tavares, Apr 8, 2021
  23. Derrick StoleeApr 14, 2021
  24. 3/7 add: make --chmod and --renormalize honor sparse checkoutsMatheus Tavares, Apr 8, 2021
  25. 4/7 pathspec: allow to ignore SKIP_WORKTREE entries on index matchingMatheus Tavares, Apr 8, 2021
  26. 5/7 refresh_index(): add flag to ignore SKIP_WORKTREE entriesMatheus Tavares, Apr 8, 2021
  27. 6/7 add: warn when asked to update SKIP_WORKTREE entriesMatheus Tavares, Apr 8, 2021
  28. 7/7 rm: honor sparse checkout patternsMatheus Tavares, Apr 8, 2021
  29. Elijah NewrenApr 14, 2021
  30. Matheus Tavares BernardinoApr 14, 2021
  31. Junio C HamanoApr 16, 2021
  32. Elijah NewrenApr 16, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.