| Message ID | cover.1615588108.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| Headers | show |
| Series | add/rm: honor sparse checkout and warn on sparse paths | expand |
On Fri, Mar 12, 2021 at 2:48 PM Matheus Tavares <matheus.bernardino@usp.br> wrote: > > Make `rm` honor sparse checkouts, and make both `rm` and `add` warn > when they are asked to update sparse entries. > > Main changes since v2: > > Patch 2: > - Removed unnecessary `rm` in t3705's auxiliary function and added > comment about why `update-index --force-remove` is used there. > > Patch 4: > - Replaced the boolean function parameter with a more descriptive enum. > This way, readers don't have to go to the function definition to > understand what the caller wants. > > Patches 5 and 6: > - Reworded both commit messages and the advice.updateSparsePath docs to > avoid saying "pathspecs that *only* match sparse entries", as the > situations to which they refer might also include matches among > ignored paths. > - Rephrased the warning message about sparse paths so that it doesn't > contradict the message on ignored paths when they are displayed > together. > > Patch 7: > - Adjusted the tests to check for the new sparse warning message. v2 already looked pretty good to me; even if I still had lots of little comments on that series. This version addresses all my remaining concerns with v2. So this gets my: Reviewed-by: Elijah Newren <newren@gmail.com> Thanks for all your work on this!
Make `rm` honor sparse checkouts, and make both `rm` and `add` warn when they are asked to update sparse entries. Main changes since v2: Patch 2: - Removed unnecessary `rm` in t3705's auxiliary function and added comment about why `update-index --force-remove` is used there. Patch 4: - Replaced the boolean function parameter with a more descriptive enum. This way, readers don't have to go to the function definition to understand what the caller wants. Patches 5 and 6: - Reworded both commit messages and the advice.updateSparsePath docs to avoid saying "pathspecs that *only* match sparse entries", as the situations to which they refer might also include matches among ignored paths. - Rephrased the warning message about sparse paths so that it doesn't contradict the message on ignored paths when they are displayed together. Patch 7: - Adjusted the tests to check for the new sparse warning message. Matheus Tavares (7): add: include magic part of pathspec on --refresh error t3705: add tests for `git add` in sparse checkouts add: make --chmod and --renormalize honor sparse checkouts pathspec: allow to ignore SKIP_WORKTREE entries on index matching refresh_index(): add REFRESH_DONT_MARK_SPARSE_MATCHES flag add: warn when asked to update SKIP_WORKTREE entries rm: honor sparse checkout patterns Documentation/config/advice.txt | 4 + Documentation/git-rm.txt | 4 +- advice.c | 20 ++++ advice.h | 4 + builtin/add.c | 77 ++++++++++++--- builtin/check-ignore.c | 3 +- builtin/rm.c | 35 ++++--- cache.h | 15 +-- pathspec.c | 25 ++++- pathspec.h | 18 +++- read-cache.c | 5 +- t/t3602-rm-sparse-checkout.sh | 78 ++++++++++++++++ t/t3700-add.sh | 6 ++ t/t3705-add-sparse-checkout.sh | 155 +++++++++++++++++++++++++++++++ t/t7011-skip-worktree-reading.sh | 5 - t/t7012-skip-worktree-writing.sh | 19 ---- 16 files changed, 409 insertions(+), 64 deletions(-) create mode 100755 t/t3602-rm-sparse-checkout.sh create mode 100755 t/t3705-add-sparse-checkout.sh Range-diff against v2: 1: 46147a8637 = 1: 05b66a8c2b add: include magic part of pathspec on --refresh error 2: 2de55420fa ! 2: d93c3f5146 t3705: add tests for `git add` in sparse checkouts @@ t/t3705-add-sparse-checkout.sh (new) + +# Optionally take a printf format string to write to the sparse_entry file +setup_sparse_entry () { -+ rm -f sparse_entry && ++ # 'sparse_entry' might already be in the index with the skip-worktree ++ # bit set. Remove it so that the subsequent git add can update it. + git update-index --force-remove sparse_entry && -+ + if test $# -eq 1 + then + printf "$1" >sparse_entry 3: 604480ade9 = 3: c90d5e3c23 add: make --chmod and --renormalize honor sparse checkouts 4: 1494e65316 ! 4: 69d51579b3 pathspec: allow to ignore SKIP_WORKTREE entries on index matching @@ Metadata ## Commit message ## pathspec: allow to ignore SKIP_WORKTREE entries on index matching - Add the 'ignore_skip_worktree' boolean parameter to both - add_pathspec_matches_against_index() and - find_pathspecs_matching_against_index(). When true, these functions will - not try to match the given pathspec with SKIP_WORKTREE entries. This - will be used in a future patch to make `git add` display a hint - when the pathspec matches only sparse paths. + Add a new enum parameter to `add_pathspec_matches_against_index()` and + `find_pathspecs_matching_against_index()`, allowing callers to specify + whether these function should attempt to match SKIP_WORKTREE entries or + not. This will be used in a future patch to make `git add` display a + hint when it is asked to update SKIP_WORKTREE entries. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> @@ builtin/add.c: static char *prune_directory(struct dir_struct *dir, struct paths } dir->nr = dst - dir->entries; - add_pathspec_matches_against_index(pathspec, &the_index, seen); -+ add_pathspec_matches_against_index(pathspec, &the_index, seen, 0); ++ add_pathspec_matches_against_index(pathspec, &the_index, seen, ++ PS_HEED_SKIP_WORKTREE); return seen; } @@ builtin/add.c: int cmd_add(int argc, const char **argv, const char *prefix) if (!seen) - seen = find_pathspecs_matching_against_index(&pathspec, &the_index); -+ seen = find_pathspecs_matching_against_index(&pathspec, &the_index, 0); ++ seen = find_pathspecs_matching_against_index(&pathspec, ++ &the_index, PS_HEED_SKIP_WORKTREE); /* * file_exists() assumes exact match @@ builtin/check-ignore.c: static int check_ignore(struct dir_struct *dir, * 'git status', 'git add' etc. */ - seen = find_pathspecs_matching_against_index(&pathspec, &the_index); -+ seen = find_pathspecs_matching_against_index(&pathspec, &the_index, 0); ++ seen = find_pathspecs_matching_against_index(&pathspec, &the_index, ++ PS_HEED_SKIP_WORKTREE); for (i = 0; i < pathspec.nr; i++) { full_path = pathspec.items[i].match; pattern = NULL; @@ pathspec.c void add_pathspec_matches_against_index(const struct pathspec *pathspec, const struct index_state *istate, - char *seen) -+ char *seen, int ignore_skip_worktree) ++ char *seen, ++ enum ps_skip_worktree_action sw_action) { int num_unmatched = 0, i; @@ pathspec.c: void add_pathspec_matches_against_index(const struct pathspec *paths return; for (i = 0; i < istate->cache_nr; i++) { const struct cache_entry *ce = istate->cache[i]; -+ if (ignore_skip_worktree && ce_skip_worktree(ce)) ++ if (sw_action == PS_IGNORE_SKIP_WORKTREE && ce_skip_worktree(ce)) + continue; ce_path_match(istate, ce, pathspec, seen); } @@ pathspec.c: void add_pathspec_matches_against_index(const struct pathspec *paths char *find_pathspecs_matching_against_index(const struct pathspec *pathspec, - const struct index_state *istate) + const struct index_state *istate, -+ int ignore_skip_worktree) ++ enum ps_skip_worktree_action sw_action) { char *seen = xcalloc(pathspec->nr, 1); - add_pathspec_matches_against_index(pathspec, istate, seen); -+ add_pathspec_matches_against_index(pathspec, istate, seen, -+ ignore_skip_worktree); ++ add_pathspec_matches_against_index(pathspec, istate, seen, sw_action); return seen; } ## pathspec.h ## @@ pathspec.h: static inline int ps_strcmp(const struct pathspec_item *item, + return strcmp(s1, s2); + } ++enum ps_skip_worktree_action { ++ PS_HEED_SKIP_WORKTREE = 0, ++ PS_IGNORE_SKIP_WORKTREE = 1 ++}; void add_pathspec_matches_against_index(const struct pathspec *pathspec, const struct index_state *istate, - char *seen); -+ char *seen, int ignore_skip_worktree); ++ char *seen, ++ enum ps_skip_worktree_action sw_action); char *find_pathspecs_matching_against_index(const struct pathspec *pathspec, - const struct index_state *istate); + const struct index_state *istate, -+ int ignore_skip_worktree); ++ enum ps_skip_worktree_action sw_action); int match_pathspec_attrs(const struct index_state *istate, const char *name, int namelen, const struct pathspec_item *item); 5: 584811e5c0 ! 5: 34a61a0d03 refresh_index(): add REFRESH_DONT_MARK_SPARSE_MATCHES flag @@ Commit message refresh_index() optionally takes a seen[] array to mark the pathspec items that had matches in the index. This is used by `git add --refresh` - to find out if there were any pathspec without matches, and display an + to find out if there was any pathspec without matches, and display an error accordingly. In the following patch, `git add` will also learn to warn about - pathspecs that only match sparse entries (which are not updated). But - for that, we will need a seen[] array marked exclusively with matches - from dense entries. To avoid having to call ce_path_match() again for - these entries after refresh_index() returns, add a flag that implements - this restriction inside the function itself. + pathspecs that match no eligible path for updating, but do match sparse + entries. For that, we will need a seen[] array marked exclusively with + matches from dense entries. To avoid having to call ce_path_match() + again for these entries after refresh_index() returns, add a flag that + implements this restriction inside the function itself. Note that refresh_index() does not update sparse entries, regardless of passing the flag or not. The flag only controls whether matches with @@ read-cache.c: int refresh_index(struct index_state *istate, unsigned int flags, int in_porcelain = (flags & REFRESH_IN_PORCELAIN); unsigned int options = (CE_MATCH_REFRESH | @@ read-cache.c: int refresh_index(struct index_state *istate, unsigned int flags, - int cache_errno = 0; - int changed = 0; int filtered = 0; + int t2_did_lstat = 0; + int t2_did_scan = 0; + char *cur_seen; ce = istate->cache[i]; 6: 15f399f4fa ! 6: 66d5c71182 add: warn when pathspec only matches SKIP_WORKTREE entries @@ Metadata Author: Matheus Tavares <matheus.bernardino@usp.br> ## Commit message ## - add: warn when pathspec only matches SKIP_WORKTREE entries + 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 a pathspec only matches these - entries. Instead, let's warn the user and display a hint on how to - update these entries. + 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 the warning is only shown if the pathspec matches no untracked - paths in the working tree and only matches index entries with the - SKIP_WORKTREE bit set. A warning message 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 matching pathspecs, as the - matched files will have already been added. + 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: advice.*:: Advice shown if a user runs the add command without providing the pathspec parameter. + updateSparsePath:: -+ Advice shown if the pathspec given to linkgit:git-add[1] only -+ matches index entries outside the current sparse-checkout. ++ Advice shown when linkgit:git-add[1] is asked to update index ++ entries outside the current sparse checkout. -- ## advice.c ## @@ advice.c: void NORETURN die_conclude_merge(void) + if (!pathspec_list->nr) + return; + -+ fprintf(stderr, _("The following pathspecs only matched index entries outside the current\n" -+ "sparse checkout:\n")); ++ 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.")); -+ ++ _("Disable or modify the sparsity rules if you intend" ++ " to update such entries.")); +} + void detach_advice(const char *new_name) @@ advice.h: void advise_if_enabled(enum advice_type type, const char *advice, ...) ## builtin/add.c ## @@ builtin/add.c: static char *prune_directory(struct dir_struct *dir, struct pathspec *pathspec, - *dst++ = entry; } dir->nr = dst - dir->entries; -- add_pathspec_matches_against_index(pathspec, &the_index, seen, 0); -+ add_pathspec_matches_against_index(pathspec, &the_index, seen, 1); + add_pathspec_matches_against_index(pathspec, &the_index, seen, +- PS_HEED_SKIP_WORKTREE); ++ PS_IGNORE_SKIP_WORKTREE); return seen; } @@ builtin/add.c: int cmd_add(int argc, const char **argv, const char *prefix) + struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP; if (!seen) -- seen = find_pathspecs_matching_against_index(&pathspec, &the_index, 0); -+ seen = find_pathspecs_matching_against_index(&pathspec, &the_index, 1); + seen = find_pathspecs_matching_against_index(&pathspec, +- &the_index, PS_HEED_SKIP_WORKTREE); ++ &the_index, PS_IGNORE_SKIP_WORKTREE); /* * file_exists() assumes exact match @@ builtin/add.c: int cmd_add(int argc, const char **argv, const char *prefix) + continue; + } + -+ /* Don't complain at 'git add .' inside empty repo. */ ++ /* Don't complain at 'git add .' on empty repo */ + if (!path[0]) + continue; + @@ pathspec.h @@ pathspec.h: 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, - int ignore_skip_worktree); + 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) @@ t/t3705-add-sparse-checkout.sh: setup_gitignore () { EOF } -+test_expect_success 'setup' ' ++test_expect_success 'setup' " + cat >sparse_error_header <<-EOF && -+ The following pathspecs only matched index entries outside the current -+ sparse checkout: ++ 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" ++ 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 && @@ t/t3705-add-sparse-checkout.sh: test_expect_success 'git add --renormalize does - git add --renormalize sparse_entry && + test_must_fail git add --renormalize sparse_entry 2>stderr && + test_i18ncmp error_and_hint stderr && - test_sparse_entry_unchanged - ' - ++ 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_i18ncmp error_and_hint stderr && -+ test_sparse_entry_unchanged -+' -+ + 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 && + test_i18ngrep "fatal: pathspec .nonexistent. did not match any files" stderr && -+ test_i18ngrep ! "The following pathspecs only matched index entries" stderr ++ test_i18ngrep ! -F -f sparse_error_header stderr +' + +test_expect_success 'do not warn when pathspec matches dense entries' ' 7: bb29aa4d8f ! 7: d25ea10560 rm: honor sparse checkout patterns @@ Metadata ## Commit message ## rm: honor sparse checkout patterns - `git add` refrains from adding or updating paths outside the sparsity - rules, but `git rm` doesn't follow the same restrictions. This is - somewhat counter-intuitive and inconsistent. So make `rm` honor the - sparse checkout and advise on how to remove SKIP_WORKTREE entries, just - like `add` does. Also add some tests for the new behavior. + `git add` refrains from adding or updating index entries that are + outside the current sparse checkout, but `git rm` doesn't follow the + same restriction. This is somewhat counter-intuitive and inconsistent. + So make `rm` honor the sparsity rules and advise on how to remove + SKIP_WORKTREE entries, just like `add` does. Also add some tests for the + new behavior. Suggested-by: Elijah Newren <newren@gmail.com> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> @@ Documentation/config/advice.txt: advice.*:: Advice shown if a user runs the add command without providing the pathspec parameter. updateSparsePath:: -- Advice shown if the pathspec given to linkgit:git-add[1] only -- matches index entries outside the current sparse-checkout. -+ Advice shown if the pathspec given to linkgit:git-add[1] or -+ linkgit:git-rm[1] only matches index entries outside the -+ current sparse-checkout. +- Advice shown when linkgit:git-add[1] is asked to update index +- entries outside the current sparse checkout. ++ Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1] ++ is asked to update index entries outside the current sparse ++ checkout. -- ## Documentation/git-rm.txt ## @@ t/t3602-rm-sparse-checkout.sh (new) + +. ./test-lib.sh + -+test_expect_success 'setup' ' ++test_expect_success 'setup' " + mkdir -p sub/dir && + touch a b c sub/d sub/dir/e && + git add -A && + git commit -m files && + -+ cat >sparse_entry_b_error <<-EOF && -+ The following pathspecs only matched index entries outside the current -+ sparse checkout: -+ b ++ 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 >b_error_and_hint sparse_entry_b_error - <<-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" ++ hint: Disable this message with \"git config advice.updateSparsePath false\" + EOF -+' ++ ++ echo b | cat sparse_error_header - >sparse_entry_b_error && ++ cat sparse_entry_b_error sparse_hint >b_error_and_hint ++" + +for opt in "" -f --dry-run +do @@ t/t3602-rm-sparse-checkout.sh (new) + git sparse-checkout set a && + test_must_fail git rm nonexistent 2>stderr && + test_i18ngrep "fatal: pathspec .nonexistent. did not match any files" stderr && -+ test_i18ngrep ! "The following pathspecs only matched index entries" stderr ++ test_i18ngrep ! -F -f sparse_error_header stderr +' + +test_expect_success 'do not warn about sparse entries when pathspec matches dense entries' '