From 031ba55b6b474559dd8db6df5479416964e0dc30 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:44 +0000 Subject: [PATCH 01/18] unpack-trees: fix minor typo in comment Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- unpack-trees.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unpack-trees.c b/unpack-trees.c index 1ecdab3304..0d0eec0221 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1618,7 +1618,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options /* * Sparse checkout loop #2: set NEW_SKIP_WORKTREE on entries not in loop #1 - * If the will have NEW_SKIP_WORKTREE, also set CE_SKIP_WORKTREE + * If they will have NEW_SKIP_WORKTREE, also set CE_SKIP_WORKTREE * so apply_sparse_checkout() won't attempt to remove it from worktree */ mark_new_skip_worktree(o->pl, &o->result, From d7dc1e1668fcbd5af019d0bc059b8dcb35743c86 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:45 +0000 Subject: [PATCH 02/18] unpack-trees: remove unused error type commit 08402b0409 ("merge-recursive: distinguish "removed" and "overwritten" messages", 2010-08-11) split ERROR_WOULD_LOSE_UNTRACKED into both ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN ERROR_WOULD_LOSE_UNTRACKED_REMOVED and also split ERROR_WOULD_LOSE_ORPHANED into both ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN ERROR_WOULD_LOSE_ORPHANED_REMOVED However, despite the split only three of these four types were used. ERROR_WOULD_LOSE_ORPHANED_REMOVED was not put into use when it was introduced and nothing else has used it in the intervening decade either. Remove it. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- unpack-trees.c | 4 ---- unpack-trees.h | 1 - 2 files changed, 5 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index 0d0eec0221..f72a7a21f9 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -49,8 +49,6 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = { /* ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN */ "Working tree file '%s' would be overwritten by sparse checkout update.", - /* ERROR_WOULD_LOSE_ORPHANED_REMOVED */ - "Working tree file '%s' would be removed by sparse checkout update.", /* ERROR_WOULD_LOSE_SUBMODULE */ "Submodule '%s' cannot checkout new HEAD.", @@ -172,8 +170,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, _("Cannot update sparse checkout: the following entries are not up to date:\n%s"); msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] = _("The following working tree files would be overwritten by sparse checkout update:\n%s"); - msgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] = - _("The following working tree files would be removed by sparse checkout update:\n%s"); msgs[ERROR_WOULD_LOSE_SUBMODULE] = _("Cannot update submodule:\n%s"); diff --git a/unpack-trees.h b/unpack-trees.h index ae1557fb80..6d7c7b6c2e 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -24,7 +24,6 @@ enum unpack_trees_error_types { ERROR_BIND_OVERLAP, ERROR_SPARSE_NOT_UPTODATE_FILE, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, - ERROR_WOULD_LOSE_ORPHANED_REMOVED, ERROR_WOULD_LOSE_SUBMODULE, NB_UNPACK_TREES_ERROR_TYPES }; From d61633ae186d26ab2c216108c917ec0ae7dd2782 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:46 +0000 Subject: [PATCH 03/18] unpack-trees: simplify verify_absent_sparse() verify_absent_sparse() was introduced in commit 08402b0409 ("merge-recursive: distinguish "removed" and "overwritten" messages", 2010-08-11), and has always had exactly one caller which always passes error_type == ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN. This function then checks whether error_type is this value, and if so, sets it instead to ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN. It has been nearly a decade and no other caller has been created, and no other value has ever been passed, so just pass the expected value to begin with. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- unpack-trees.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index f72a7a21f9..3af2e126ab 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -506,7 +506,7 @@ static int apply_sparse_checkout(struct index_state *istate, ce->ce_flags &= ~CE_UPDATE; } if (was_skip_worktree && !ce_skip_worktree(ce)) { - if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) + if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, o)) return -1; ce->ce_flags |= CE_UPDATE; } @@ -2026,11 +2026,7 @@ static int verify_absent_sparse(const struct cache_entry *ce, enum unpack_trees_error_types error_type, struct unpack_trees_options *o) { - enum unpack_trees_error_types orphaned_error = error_type; - if (orphaned_error == ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN) - orphaned_error = ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN; - - return verify_absent_1(ce, orphaned_error, o); + return verify_absent_1(ce, error_type, o); } static int merged_entry(const struct cache_entry *ce, From fa0bde45cdebee8bac95e66e4bf5ba16c77fbdba Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:47 +0000 Subject: [PATCH 04/18] unpack-trees: simplify pattern_list freeing commit e091228e17 ("sparse-checkout: update working directory in-process", 2019-11-21) allowed passing a pre-defined set of patterns to unpack_trees(). However, if o->pl was NULL, it would still read the existing patterns and use those. If those patterns were read into a data structure that was allocated, naturally they needed to be free'd. However, despite the same function being responsible for knowing about both the allocation and the free'ing, the logic for tracking whether to free the pattern_list was hoisted to an outer function with an additional flag in unpack_trees_options. Put the logic back in the relevant function and discard the now unnecessary flag. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/sparse-checkout.c | 1 - unpack-trees.c | 6 ++++-- unpack-trees.h | 3 +-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 740da4b6d5..d102a9697f 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -122,7 +122,6 @@ static int update_working_directory(struct pattern_list *pl) o.dst_index = r->index; o.skip_sparse_checkout = 0; o.pl = pl; - o.keep_pattern_list = !!pl; resolve_undo_clear_index(r->index); setup_work_tree(); diff --git a/unpack-trees.c b/unpack-trees.c index 3af2e126ab..d2863fa031 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1503,6 +1503,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options int i, ret; static struct cache_entry *dfc; struct pattern_list pl; + int free_pattern_list = 0; if (len > MAX_UNPACK_TREES) die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES); @@ -1519,6 +1520,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options else o->pl = &pl; free(sparse); + free_pattern_list = 1; } memset(&o->result, 0, sizeof(o->result)); @@ -1686,9 +1688,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options o->src_index = NULL; done: - trace_performance_leave("unpack_trees"); - if (!o->keep_pattern_list) + if (free_pattern_list) clear_pattern_list(&pl); + trace_performance_leave("unpack_trees"); return ret; return_failed: diff --git a/unpack-trees.h b/unpack-trees.h index 6d7c7b6c2e..d3516267f3 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -58,8 +58,7 @@ struct unpack_trees_options { quiet, exiting_early, show_all_errors, - dry_run, - keep_pattern_list; + dry_run; const char *prefix; int cache_bottom; struct dir_struct *dir; From 72064ee578a59d4511cab17496c5246af02397f3 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:48 +0000 Subject: [PATCH 05/18] t1091: make some tests a little more defensive against failures Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t1091-sparse-checkout-builtin.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 44a91205d6..8607a8e6d1 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -278,6 +278,7 @@ test_expect_success 'cone mode: add parent path' ' ' test_expect_success 'revert to old sparse-checkout on bad update' ' + test_when_finished git -C repo sparse-checkout disable && test_when_finished git -C repo reset --hard && git -C repo sparse-checkout set deep && echo update >repo/deep/deeper2/a && @@ -328,6 +329,7 @@ test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status' ' test_expect_success 'cone mode: set with core.ignoreCase=true' ' + rm repo/.git/info/sparse-checkout && git -C repo sparse-checkout init --cone && git -C repo -c core.ignoreCase=true sparse-checkout set folder1 && cat >expect <<-\EOF && From b0a5a12a60393237f98ec4b0fcf2b7d3c3232a2a Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:49 +0000 Subject: [PATCH 06/18] unpack-trees: allow check_updates() to work on a different index check_updates() previously assumed it was working on o->result. We want to use this function in combination with a different index_state, so take the intended index_state as a parameter. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- unpack-trees.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index d2863fa031..dde50047a8 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -353,12 +353,12 @@ static void report_collided_checkout(struct index_state *index) string_list_clear(&list, 0); } -static int check_updates(struct unpack_trees_options *o) +static int check_updates(struct unpack_trees_options *o, + struct index_state *index) { unsigned cnt = 0; int errs = 0; struct progress *progress; - struct index_state *index = &o->result; struct checkout state = CHECKOUT_INIT; int i; @@ -1665,7 +1665,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options } } - ret = check_updates(o) ? (-2) : 0; + ret = check_updates(o, &o->result) ? (-2) : 0; if (o->dst_index) { move_index_extensions(&o->result, o->src_index); if (!ret) { From 3cc7c50402ce1cbac9735da64844eb00bc096f46 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:50 +0000 Subject: [PATCH 07/18] unpack-trees: do not mark a dirty path with SKIP_WORKTREE If a path is dirty, removing from the working tree risks losing data. As such, we want to make sure any such path is not marked with SKIP_WORKTREE. While the current callers of this code detect this case and re-populate with a previous set of sparsity patterns, we want to allow some paths to be marked with SKIP_WORKTREE while others are left unmarked without it being considered an error. The reason this shouldn't be considered an error is that SKIP_WORKTREE has always been an advisory-only setting; merge and rebase for example were free to materialize paths and clear the SKIP_WORKTREE bit in order to accomplish their work even though they kept the SKIP_WORKTREE bit set for other paths. Leaving dirty working files in the working tree is thus a natural extension of what we have already been doing. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- unpack-trees.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/unpack-trees.c b/unpack-trees.c index dde50047a8..e8e794880a 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -500,8 +500,11 @@ static int apply_sparse_checkout(struct index_state *istate, * also stat info may have lost after merged_entry() so calling * verify_uptodate() again may fail */ - if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o)) + if (!(ce->ce_flags & CE_UPDATE) && + verify_uptodate_sparse(ce, o)) { + ce->ce_flags &= ~CE_SKIP_WORKTREE; return -1; + } ce->ce_flags |= CE_WT_REMOVE; ce->ce_flags &= ~CE_UPDATE; } From 30e89c12f0afc5c4382b531973a5e82b60d402bd Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:51 +0000 Subject: [PATCH 08/18] unpack-trees: pull sparse-checkout pattern reading into a new function Create a populate_from_existing_patterns() function for reading the path_patterns from $GIT_DIR/info/sparse-checkout so that we can re-use it elsewhere. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- unpack-trees.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index e8e794880a..4733e7eaf8 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1492,6 +1492,20 @@ static void mark_new_skip_worktree(struct pattern_list *pl, clear_ce_flags(istate, select_flag, skip_wt_flag, pl, show_progress); } +static void populate_from_existing_patterns(struct unpack_trees_options *o, + struct pattern_list *pl) +{ + char *sparse = git_pathdup("info/sparse-checkout"); + + pl->use_cone_patterns = core_sparse_checkout_cone; + if (add_patterns_from_file_to_list(sparse, "", 0, pl, NULL) < 0) + o->skip_sparse_checkout = 1; + else + o->pl = pl; + free(sparse); +} + + static int verify_absent(const struct cache_entry *, enum unpack_trees_error_types, struct unpack_trees_options *); @@ -1512,18 +1526,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES); trace_performance_enter(); - memset(&pl, 0, sizeof(pl)); if (!core_apply_sparse_checkout || !o->update) o->skip_sparse_checkout = 1; if (!o->skip_sparse_checkout && !o->pl) { - char *sparse = git_pathdup("info/sparse-checkout"); - pl.use_cone_patterns = core_sparse_checkout_cone; - if (add_patterns_from_file_to_list(sparse, "", 0, &pl, NULL) < 0) - o->skip_sparse_checkout = 1; - else - o->pl = &pl; - free(sparse); + memset(&pl, 0, sizeof(pl)); free_pattern_list = 1; + populate_from_existing_patterns(o, &pl); } memset(&o->result, 0, sizeof(o->result)); From 7af7a25853cb87f34d192ba1d813ca09ee15d9f4 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:52 +0000 Subject: [PATCH 09/18] unpack-trees: add a new update_sparsity() function Previously, the only way to update the SKIP_WORKTREE bits for various paths was invoking `git read-tree -mu HEAD` or calling the same code that this codepath invoked. This however had a number of problems if the index or working directory were not clean. First, let's consider the case: Flipping SKIP_WORKTREE -> !SKIP_WORKTREE (materializing files) If the working tree was clean this was fine, but if there were files or directories or symlinks or whatever already present at the given path then the operation would abort with an error. Let's label this case for later discussion: A) There is an untracked path in the way Now let's consider the opposite case: Flipping !SKIP_WORKTREE -> SKIP_WORKTREE (removing files) If the index and working tree was clean this was fine, but if there were any unclean paths we would run into problems. There are three different cases to consider: B) The path is unmerged C) The path has unstaged changes D) The path has staged changes (differs from HEAD) If any path fell into case B or C, then the whole operation would be aborted with an error. With sparse-checkout, the whole operation would be aborted for case D as well, but for its predecessor of using `git read-tree -mu HEAD` directly, any paths that fell into case D would be removed from the working copy and the index entry for that path would be reset to match HEAD -- which looks and feels like data loss to users (only a few are even aware to ask whether it can be recovered, and even then it requires walking through loose objects trying to match up the right ones). Refusing to remove files that have unsaved user changes is good, but refusing to work on any other paths is very problematic for users. If the user is in the middle of a rebase or has made modifications to files that bring in more dependencies, then for their build to work they need to update the sparse paths. This logic has been preventing them from doing so. Sometimes in response, the user will stage the files and re-try, to no avail with sparse-checkout or to the horror of losing their changes if they are using its predecessor of `git read-tree -mu HEAD`. Add a new update_sparsity() function which will not error out in any of these cases but behaves as follows for the special cases: A) Leave the file in the working copy alone, clear the SKIP_WORKTREE bit, and print a warning (thus leaving the path in a state where status will report the file as modified, which seems logical). B) Do NOT mark this path as SKIP_WORKTREE, and leave it as unmerged. C) Do NOT mark this path as SKIP_WORKTREE and print a warning about the dirty path. D) Mark the path as SKIP_WORKTREE, but do not revert the version stored in the index to match HEAD; leave the contents alone. I tried a different behavior for A (leave the SKIP_WORKTREE bit set), but found it very surprising and counter-intuitive (e.g. the user sees it is present along with all the other files in that directory, tries to stage it, but git add ignores it since the SKIP_WORKTREE bit is set). A & C seem like optimal behavior to me. B may be as well, though I wonder if printing a warning would be an improvement. Some might be slightly surprised by D at first, but given that it does the right thing with `git commit` and even `git commit -a` (`git add` ignores entries that are marked SKIP_WORKTREE and thus doesn't delete them, and `commit -a` is similar), it seems logical to me. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- unpack-trees.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++ unpack-trees.h | 9 ++++++ 2 files changed, 86 insertions(+) diff --git a/unpack-trees.c b/unpack-trees.c index 4733e7eaf8..a5bc0a3a16 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1714,6 +1714,83 @@ return_failed: goto done; } +/* + * Update SKIP_WORKTREE bits according to sparsity patterns, and update + * working directory to match. + * + * CE_NEW_SKIP_WORKTREE is used internally. + */ +enum update_sparsity_result update_sparsity(struct unpack_trees_options *o) +{ + enum update_sparsity_result ret = UPDATE_SPARSITY_SUCCESS; + struct pattern_list pl; + int i, empty_worktree; + unsigned old_show_all_errors; + int free_pattern_list = 0; + + old_show_all_errors = o->show_all_errors; + o->show_all_errors = 1; + + /* Sanity checks */ + if (!o->update || o->index_only || o->skip_sparse_checkout) + BUG("update_sparsity() is for reflecting sparsity patterns in working directory"); + if (o->src_index != o->dst_index || o->fn) + BUG("update_sparsity() called wrong"); + + trace_performance_enter(); + + /* If we weren't given patterns, use the recorded ones */ + if (!o->pl) { + memset(&pl, 0, sizeof(pl)); + free_pattern_list = 1; + populate_from_existing_patterns(o, &pl); + if (o->skip_sparse_checkout) + goto skip_sparse_checkout; + } + + /* Set NEW_SKIP_WORKTREE on existing entries. */ + mark_all_ce_unused(o->src_index); + mark_new_skip_worktree(o->pl, o->src_index, 0, + CE_NEW_SKIP_WORKTREE, o->verbose_update); + + /* Then loop over entries and update/remove as needed */ + ret = UPDATE_SPARSITY_SUCCESS; + empty_worktree = 1; + for (i = 0; i < o->src_index->cache_nr; i++) { + struct cache_entry *ce = o->src_index->cache[i]; + + if (apply_sparse_checkout(o->src_index, ce, o)) + ret = UPDATE_SPARSITY_WARNINGS; + + if (!ce_skip_worktree(ce)) + empty_worktree = 0; + } + + /* + * Sparse checkout is meant to narrow down checkout area + * but it does not make sense to narrow down to empty working + * tree. This is usually a mistake in sparse checkout rules. + * Do not allow users to do that. + */ + if (o->src_index->cache_nr && empty_worktree) { + unpack_failed(o, "Sparse checkout leaves no entry on working directory"); + ret = UPDATE_SPARSITY_INDEX_UPDATE_FAILURES; + goto done; + } + +skip_sparse_checkout: + if (check_updates(o, o->src_index)) + ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES; + +done: + display_error_msgs(o); + o->show_all_errors = old_show_all_errors; + if (free_pattern_list) + clear_pattern_list(&pl); + trace_performance_leave("update_sparsity"); + return ret; +} + /* Here come the merge functions */ static int reject_merge(const struct cache_entry *ce, diff --git a/unpack-trees.h b/unpack-trees.h index d3516267f3..5cf41ef5b5 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -88,6 +88,15 @@ struct unpack_trees_options { int unpack_trees(unsigned n, struct tree_desc *t, struct unpack_trees_options *options); +enum update_sparsity_result { + UPDATE_SPARSITY_SUCCESS = 0, + UPDATE_SPARSITY_WARNINGS = 1, + UPDATE_SPARSITY_INDEX_UPDATE_FAILURES = -1, + UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2 +}; + +enum update_sparsity_result update_sparsity(struct unpack_trees_options *options); + int verify_uptodate(const struct cache_entry *ce, struct unpack_trees_options *o); From f56f31af0301f6446f8689ba1a997d74fb82f45f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:53 +0000 Subject: [PATCH 10/18] sparse-checkout: use new update_sparsity() function Remove the equivalent of 'git read-tree -mu HEAD' in the sparse-checkout codepaths for setting the SKIP_WORKTREE bits and instead use the new update_sparsity() function. Note that when an issue is hit, the error message splits 'error' and 'Cannot update sparse checkout' on separate lines. For now, we use two greps to find both pieces of the error message but subsequent commits will clean up the messages reported to the user. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/sparse-checkout.c | 40 ++++++++---------------------- t/t1091-sparse-checkout-builtin.sh | 39 +++++++++++++++++++++-------- 2 files changed, 39 insertions(+), 40 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index d102a9697f..a55c60d759 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -94,49 +94,35 @@ static int sparse_checkout_list(int argc, const char **argv) static int update_working_directory(struct pattern_list *pl) { - int result = 0; + enum update_sparsity_result result; struct unpack_trees_options o; struct lock_file lock_file = LOCK_INIT; - struct object_id oid; - struct tree *tree; - struct tree_desc t; struct repository *r = the_repository; - if (repo_read_index_unmerged(r)) - die(_("you need to resolve your current index first")); - - if (get_oid("HEAD", &oid)) - return 0; - - tree = parse_tree_indirect(&oid); - parse_tree(tree); - init_tree_desc(&t, tree->buffer, tree->size); - memset(&o, 0, sizeof(o)); o.verbose_update = isatty(2); - o.merge = 1; o.update = 1; - o.fn = oneway_merge; o.head_idx = -1; o.src_index = r->index; o.dst_index = r->index; o.skip_sparse_checkout = 0; o.pl = pl; - resolve_undo_clear_index(r->index); setup_work_tree(); - cache_tree_free(&r->index->cache_tree); - repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR); - core_apply_sparse_checkout = 1; - result = unpack_trees(1, &t, &o); + result = update_sparsity(&o); - if (!result) { - prime_cache_tree(r, r->index, tree); + if (result == UPDATE_SPARSITY_WARNINGS) + /* + * We don't do any special handling of warnings from untracked + * files in the way or dirty entries that can't be removed. + */ + result = UPDATE_SPARSITY_SUCCESS; + if (result == UPDATE_SPARSITY_SUCCESS) write_locked_index(r->index, &lock_file, COMMIT_LOCK); - } else + else rollback_lock_file(&lock_file); return result; @@ -303,8 +289,6 @@ static int sparse_checkout_init(int argc, const char **argv) }; repo_read_index(the_repository); - require_clean_work_tree(the_repository, - N_("initialize sparse-checkout"), NULL, 1, 0); argc = parse_options(argc, argv, NULL, builtin_sparse_checkout_init_options, @@ -559,8 +543,6 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix, }; repo_read_index(the_repository); - require_clean_work_tree(the_repository, - N_("set sparse-checkout patterns"), NULL, 1, 0); argc = parse_options(argc, argv, prefix, builtin_sparse_checkout_set_options, @@ -576,8 +558,6 @@ static int sparse_checkout_disable(int argc, const char **argv) struct strbuf match_all = STRBUF_INIT; repo_read_index(the_repository); - require_clean_work_tree(the_repository, - N_("disable sparse-checkout"), NULL, 1, 0); memset(&pl, 0, sizeof(pl)); hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0); diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 8607a8e6d1..a991e0a80d 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -277,16 +277,23 @@ test_expect_success 'cone mode: add parent path' ' check_files repo a deep folder1 ' -test_expect_success 'revert to old sparse-checkout on bad update' ' +test_expect_success 'not-up-to-date does not block rest of sparsification' ' test_when_finished git -C repo sparse-checkout disable && test_when_finished git -C repo reset --hard && git -C repo sparse-checkout set deep && + echo update >repo/deep/deeper2/a && cp repo/.git/info/sparse-checkout expect && - test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err && - test_i18ngrep "cannot set sparse-checkout patterns" err && - test_cmp repo/.git/info/sparse-checkout expect && - check_files repo/deep a deeper1 deeper2 + test_write_lines "!/deep/*/" "/deep/deeper1/" >>expect && + + git -C repo sparse-checkout set deep/deeper1 2>err && + + test_i18ngrep "Cannot update sparse checkout" err && + test_cmp expect repo/.git/info/sparse-checkout && + check_files repo/deep a deeper1 deeper2 && + check_files repo/deep/deeper1 a deepest && + check_files repo/deep/deeper1/deepest a && + check_files repo/deep/deeper2 a ' test_expect_success 'revert to old sparse-checkout on empty update' ' @@ -316,16 +323,28 @@ test_expect_success '.gitignore should not warn about cone mode' ' test_i18ngrep ! "disabling cone patterns" err ' -test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status' ' +test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' ' git clone repo dirty && echo dirty >dirty/folder1/a && - test_must_fail git -C dirty sparse-checkout init && - test_must_fail git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* && - test_must_fail git -C dirty sparse-checkout disable && + + git -C dirty sparse-checkout init 2>err && + test_i18ngrep "error" err && + test_i18ngrep "Cannot update sparse checkout" err && + + git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err && + test_i18ngrep "error" err && + test_i18ngrep "Cannot update sparse checkout" err && + test_path_is_file dirty/folder1/a && + + git -C dirty sparse-checkout disable 2>err && + test_must_be_empty err && + git -C dirty reset --hard && git -C dirty sparse-checkout init && git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* && - git -C dirty sparse-checkout disable + test_path_is_missing dirty/folder1/a && + git -C dirty sparse-checkout disable && + test_path_is_file dirty/folder1/a ' test_expect_success 'cone mode: set with core.ignoreCase=true' ' From 4ee5d50fc39f8d41f67c3c9a936084fb914c9a50 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:54 +0000 Subject: [PATCH 11/18] sparse-checkout: use improved unpack_trees porcelain messages setup_unpack_trees_porcelain() provides much improved error/warning messages; instead of a message that assumes that there is only one path with a given problem despite being used by code that intentionally is grouping and showing errors together, it uses a message designed to be used with groups of paths. For example, this transforms error: Entry ' folder1/a folder2/a ' not uptodate. Cannot update sparse checkout. into error: Cannot update sparse checkout: the following entries are not up to date: folder1/a folder2/a In the past the suboptimal messages were never actually triggered because we would error out if the working directory wasn't clean before we even called unpack_trees(). The previous commit changed that, though, so let's use the better error messages. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/sparse-checkout.c | 2 ++ t/t1091-sparse-checkout-builtin.sh | 6 ++---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index a55c60d759..aa81199f85 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -112,7 +112,9 @@ static int update_working_directory(struct pattern_list *pl) repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR); + setup_unpack_trees_porcelain(&o, "sparse-checkout"); result = update_sparsity(&o); + clear_unpack_trees_porcelain(&o); if (result == UPDATE_SPARSITY_WARNINGS) /* diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index a991e0a80d..9bc65d32f0 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -328,12 +328,10 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' echo dirty >dirty/folder1/a && git -C dirty sparse-checkout init 2>err && - test_i18ngrep "error" err && - test_i18ngrep "Cannot update sparse checkout" err && + test_i18ngrep "error.*Cannot update sparse checkout" err && git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err && - test_i18ngrep "error" err && - test_i18ngrep "Cannot update sparse checkout" err && + test_i18ngrep "error.*Cannot update sparse checkout" err && test_path_is_file dirty/folder1/a && git -C dirty sparse-checkout disable 2>err && From cd002c15611a995a29b9e5881745bba402d63952 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:55 +0000 Subject: [PATCH 12/18] unpack-trees: move ERROR_WOULD_LOSE_SUBMODULE earlier A minor change, but we want to convert the sparse messages to warnings and this allows us to group warnings and errors. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- unpack-trees.c | 12 ++++++------ unpack-trees.h | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index a5bc0a3a16..eeac309e30 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -43,15 +43,14 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = { /* ERROR_BIND_OVERLAP */ "Entry '%s' overlaps with '%s'. Cannot bind.", + /* ERROR_WOULD_LOSE_SUBMODULE */ + "Submodule '%s' cannot checkout new HEAD.", + /* ERROR_SPARSE_NOT_UPTODATE_FILE */ "Entry '%s' not uptodate. Cannot update sparse checkout.", /* ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN */ "Working tree file '%s' would be overwritten by sparse checkout update.", - - - /* ERROR_WOULD_LOSE_SUBMODULE */ - "Submodule '%s' cannot checkout new HEAD.", }; #define ERRORMSG(o,type) \ @@ -166,12 +165,13 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, */ msgs[ERROR_BIND_OVERLAP] = _("Entry '%s' overlaps with '%s'. Cannot bind."); + msgs[ERROR_WOULD_LOSE_SUBMODULE] = + _("Cannot update submodule:\n%s"); + msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] = _("Cannot update sparse checkout: the following entries are not up to date:\n%s"); msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] = _("The following working tree files would be overwritten by sparse checkout update:\n%s"); - msgs[ERROR_WOULD_LOSE_SUBMODULE] = - _("Cannot update submodule:\n%s"); opts->show_all_errors = 1; /* rejected paths may not have a static buffer */ diff --git a/unpack-trees.h b/unpack-trees.h index 5cf41ef5b5..3e996a6c0a 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -22,9 +22,9 @@ enum unpack_trees_error_types { ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, ERROR_BIND_OVERLAP, + ERROR_WOULD_LOSE_SUBMODULE, ERROR_SPARSE_NOT_UPTODATE_FILE, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, - ERROR_WOULD_LOSE_SUBMODULE, NB_UNPACK_TREES_ERROR_TYPES }; From 1ac83f42da6437d74098f162fcfa37474c94e223 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:56 +0000 Subject: [PATCH 13/18] unpack-trees: rename ERROR_* fields meant for warnings to WARNING_* We want to treat issues with setting the SKIP_WORKTREE bit as a warning rather than an error; rename the enum values to reflect this intent as a simple step towards that goal. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- unpack-trees.c | 12 ++++++------ unpack-trees.h | 8 +++++--- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index eeac309e30..2a2306c5c2 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -46,10 +46,10 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = { /* ERROR_WOULD_LOSE_SUBMODULE */ "Submodule '%s' cannot checkout new HEAD.", - /* ERROR_SPARSE_NOT_UPTODATE_FILE */ + /* WARNING_SPARSE_NOT_UPTODATE_FILE */ "Entry '%s' not uptodate. Cannot update sparse checkout.", - /* ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN */ + /* WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN */ "Working tree file '%s' would be overwritten by sparse checkout update.", }; @@ -168,9 +168,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, msgs[ERROR_WOULD_LOSE_SUBMODULE] = _("Cannot update submodule:\n%s"); - msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] = + msgs[WARNING_SPARSE_NOT_UPTODATE_FILE] = _("Cannot update sparse checkout: the following entries are not up to date:\n%s"); - msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] = + msgs[WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN] = _("The following working tree files would be overwritten by sparse checkout update:\n%s"); opts->show_all_errors = 1; @@ -509,7 +509,7 @@ static int apply_sparse_checkout(struct index_state *istate, ce->ce_flags &= ~CE_UPDATE; } if (was_skip_worktree && !ce_skip_worktree(ce)) { - if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, o)) + if (verify_absent_sparse(ce, WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN, o)) return -1; ce->ce_flags |= CE_UPDATE; } @@ -1875,7 +1875,7 @@ int verify_uptodate(const struct cache_entry *ce, static int verify_uptodate_sparse(const struct cache_entry *ce, struct unpack_trees_options *o) { - return verify_uptodate_1(ce, o, ERROR_SPARSE_NOT_UPTODATE_FILE); + return verify_uptodate_1(ce, o, WARNING_SPARSE_NOT_UPTODATE_FILE); } /* diff --git a/unpack-trees.h b/unpack-trees.h index 3e996a6c0a..aac1ad4b01 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -23,9 +23,11 @@ enum unpack_trees_error_types { ERROR_WOULD_LOSE_UNTRACKED_REMOVED, ERROR_BIND_OVERLAP, ERROR_WOULD_LOSE_SUBMODULE, - ERROR_SPARSE_NOT_UPTODATE_FILE, - ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, - NB_UNPACK_TREES_ERROR_TYPES + + WARNING_SPARSE_NOT_UPTODATE_FILE, + WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN, + + NB_UNPACK_TREES_ERROR_TYPES, }; /* From 6271d77cb1ccfb7e4124593b4463446fe1188cda Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:57 +0000 Subject: [PATCH 14/18] unpack-trees: split display_error_msgs() into two display_error_msgs() is never called to show messages of both ERROR_* and WARNING_* types at the same time; it is instead called multiple times, separately for each type. Since we want to display these types differently, make two slightly different versions of this function. A subsequent commit will further modify unpack_trees() and how it calls the new display_warning_msgs(). Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t1091-sparse-checkout-builtin.sh | 4 +-- unpack-trees.c | 50 +++++++++++++++++++++++++----- unpack-trees.h | 8 +++-- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 9bc65d32f0..ed5e905996 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -328,10 +328,10 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' echo dirty >dirty/folder1/a && git -C dirty sparse-checkout init 2>err && - test_i18ngrep "error.*Cannot update sparse checkout" err && + test_i18ngrep "warning.*Cannot update sparse checkout" err && git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err && - test_i18ngrep "error.*Cannot update sparse checkout" err && + test_i18ngrep "warning.*Cannot update sparse checkout" err && test_path_is_file dirty/folder1/a && git -C dirty sparse-checkout disable 2>err && diff --git a/unpack-trees.c b/unpack-trees.c index 2a2306c5c2..f9a5626a67 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -24,7 +24,7 @@ * situation better. See how "git checkout" and "git merge" replaces * them using setup_unpack_trees_porcelain(), for example. */ -static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = { +static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = { /* ERROR_WOULD_OVERWRITE */ "Entry '%s' would be overwritten by merge. Cannot merge.", @@ -46,6 +46,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = { /* ERROR_WOULD_LOSE_SUBMODULE */ "Submodule '%s' cannot checkout new HEAD.", + /* NB_UNPACK_TREES_ERROR_TYPES; just a meta value */ + "", + /* WARNING_SPARSE_NOT_UPTODATE_FILE */ "Entry '%s' not uptodate. Cannot update sparse checkout.", @@ -222,7 +225,7 @@ static int add_rejected_path(struct unpack_trees_options *o, /* * Otherwise, insert in a list for future display by - * display_error_msgs() + * display_(error|warning)_msgs() */ string_list_append(&o->unpack_rejects[e], path); return -1; @@ -233,13 +236,16 @@ static int add_rejected_path(struct unpack_trees_options *o, */ static void display_error_msgs(struct unpack_trees_options *o) { - int e, i; - int something_displayed = 0; + int e; + unsigned error_displayed = 0; for (e = 0; e < NB_UNPACK_TREES_ERROR_TYPES; e++) { struct string_list *rejects = &o->unpack_rejects[e]; + if (rejects->nr > 0) { + int i; struct strbuf path = STRBUF_INIT; - something_displayed = 1; + + error_displayed = 1; for (i = 0; i < rejects->nr; i++) strbuf_addf(&path, "\t%s\n", rejects->items[i].string); error(ERRORMSG(o, e), super_prefixed(path.buf)); @@ -247,10 +253,36 @@ static void display_error_msgs(struct unpack_trees_options *o) } string_list_clear(rejects, 0); } - if (something_displayed) + if (error_displayed) fprintf(stderr, _("Aborting\n")); } +/* + * display all the warning messages stored in a nice way + */ +static void display_warning_msgs(struct unpack_trees_options *o) +{ + int e; + unsigned warning_displayed = 0; + for (e = NB_UNPACK_TREES_ERROR_TYPES + 1; + e < NB_UNPACK_TREES_WARNING_TYPES; e++) { + struct string_list *rejects = &o->unpack_rejects[e]; + + if (rejects->nr > 0) { + int i; + struct strbuf path = STRBUF_INIT; + + warning_displayed = 1; + for (i = 0; i < rejects->nr; i++) + strbuf_addf(&path, "\t%s\n", rejects->items[i].string); + warning(ERRORMSG(o, e), super_prefixed(path.buf)); + strbuf_release(&path); + } + string_list_clear(rejects, 0); + } + if (warning_displayed) + fprintf(stderr, _("After fixing the above paths, you may want to run `git sparse-checkout reapply`.\n")); +} static int check_submodule_move_head(const struct cache_entry *ce, const char *old_id, const char *new_id, @@ -1705,8 +1737,10 @@ done: return ret; return_failed: - if (o->show_all_errors) + if (o->show_all_errors) { display_error_msgs(o); + display_warning_msgs(o); + } mark_all_ce_unused(o->src_index); ret = unpack_failed(o, NULL); if (o->exiting_early) @@ -1783,7 +1817,7 @@ skip_sparse_checkout: ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES; done: - display_error_msgs(o); + display_warning_msgs(o); o->show_all_errors = old_show_all_errors; if (free_pattern_list) clear_pattern_list(&pl); diff --git a/unpack-trees.h b/unpack-trees.h index aac1ad4b01..dae948205f 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -24,10 +24,12 @@ enum unpack_trees_error_types { ERROR_BIND_OVERLAP, ERROR_WOULD_LOSE_SUBMODULE, + NB_UNPACK_TREES_ERROR_TYPES, + WARNING_SPARSE_NOT_UPTODATE_FILE, WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN, - NB_UNPACK_TREES_ERROR_TYPES, + NB_UNPACK_TREES_WARNING_TYPES, }; /* @@ -66,13 +68,13 @@ struct unpack_trees_options { struct dir_struct *dir; struct pathspec *pathspec; merge_fn_t fn; - const char *msgs[NB_UNPACK_TREES_ERROR_TYPES]; + const char *msgs[NB_UNPACK_TREES_WARNING_TYPES]; struct argv_array msgs_to_free; /* * Store error messages in an array, each case * corresponding to a error message type */ - struct string_list unpack_rejects[NB_UNPACK_TREES_ERROR_TYPES]; + struct string_list unpack_rejects[NB_UNPACK_TREES_WARNING_TYPES]; int head_idx; int merge_size; From 22ab0b37d85080d8932e992e183b2f86e67bff19 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:58 +0000 Subject: [PATCH 15/18] unpack-trees: make sparse path messages sound like warnings The messages for problems with sparse paths are phrased as errors that cause the operation to abort, even though we are not making the operation abort. Reword the messages to make sense in their new context. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t1091-sparse-checkout-builtin.sh | 6 +++--- unpack-trees.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index ed5e905996..afbde89e60 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -288,7 +288,7 @@ test_expect_success 'not-up-to-date does not block rest of sparsification' ' git -C repo sparse-checkout set deep/deeper1 2>err && - test_i18ngrep "Cannot update sparse checkout" err && + test_i18ngrep "The following paths are not up to date" err && test_cmp expect repo/.git/info/sparse-checkout && check_files repo/deep a deeper1 deeper2 && check_files repo/deep/deeper1 a deepest && @@ -328,10 +328,10 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' echo dirty >dirty/folder1/a && git -C dirty sparse-checkout init 2>err && - test_i18ngrep "warning.*Cannot update sparse checkout" err && + test_i18ngrep "warning.*The following paths are not up to date" err && git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err && - test_i18ngrep "warning.*Cannot update sparse checkout" err && + test_i18ngrep "warning.*The following paths are not up to date" err && test_path_is_file dirty/folder1/a && git -C dirty sparse-checkout disable 2>err && diff --git a/unpack-trees.c b/unpack-trees.c index f9a5626a67..484d30a53a 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -50,10 +50,10 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = { "", /* WARNING_SPARSE_NOT_UPTODATE_FILE */ - "Entry '%s' not uptodate. Cannot update sparse checkout.", + "Path '%s' not uptodate; will not remove from working tree.", /* WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN */ - "Working tree file '%s' would be overwritten by sparse checkout update.", + "Path '%s' already present; will not overwrite with sparse update.", }; #define ERRORMSG(o,type) \ @@ -172,9 +172,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, _("Cannot update submodule:\n%s"); msgs[WARNING_SPARSE_NOT_UPTODATE_FILE] = - _("Cannot update sparse checkout: the following entries are not up to date:\n%s"); + _("The following paths are not up to date and were left despite sparse patterns:\n%s"); msgs[WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN] = - _("The following working tree files would be overwritten by sparse checkout update:\n%s"); + _("The following paths were already present and thus not updated despite sparse patterns:\n%s"); opts->show_all_errors = 1; /* rejected paths may not have a static buffer */ From ebb568b9e2539ce2dbb3853ff3d4869a5c690601 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:59 +0000 Subject: [PATCH 16/18] unpack-trees: provide warnings on sparse updates for unmerged paths too When sparse-checkout runs to update the list of sparsity patterns, it gives warnings if it can't remove paths from the working tree because those files have dirty changes. Add a similar warning for unmerged paths as well. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t1091-sparse-checkout-builtin.sh | 25 +++++++++++++++++++++++++ unpack-trees.c | 30 ++++++++++++++++++++++++++++++ unpack-trees.h | 1 + 3 files changed, 56 insertions(+) diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index afbde89e60..8e2976bc7b 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -345,6 +345,31 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' test_path_is_file dirty/folder1/a ' +test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged status' ' + git clone repo unmerged && + + cat >input <<-EOF && + 0 0000000000000000000000000000000000000000 folder1/a + 100644 $(git -C unmerged rev-parse HEAD:folder1/a) 1 folder1/a + EOF + git -C unmerged update-index --index-info err && + test_i18ngrep "warning.*The following paths are unmerged" err && + + git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* 2>err && + test_i18ngrep "warning.*The following paths are unmerged" err && + test_path_is_file dirty/folder1/a && + + git -C unmerged sparse-checkout disable 2>err && + test_i18ngrep "warning.*The following paths are unmerged" err && + + git -C unmerged reset --hard && + git -C unmerged sparse-checkout init && + git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* && + git -C unmerged sparse-checkout disable +' + test_expect_success 'cone mode: set with core.ignoreCase=true' ' rm repo/.git/info/sparse-checkout && git -C repo sparse-checkout init --cone && diff --git a/unpack-trees.c b/unpack-trees.c index 484d30a53a..dec044339d 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -52,6 +52,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = { /* WARNING_SPARSE_NOT_UPTODATE_FILE */ "Path '%s' not uptodate; will not remove from working tree.", + /* WARNING_SPARSE_UNMERGED_FILE */ + "Path '%s' unmerged; will not remove from working tree.", + /* WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN */ "Path '%s' already present; will not overwrite with sparse update.", }; @@ -173,6 +176,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, msgs[WARNING_SPARSE_NOT_UPTODATE_FILE] = _("The following paths are not up to date and were left despite sparse patterns:\n%s"); + msgs[WARNING_SPARSE_UNMERGED_FILE] = + _("The following paths are unmerged and were left despite sparse patterns:\n%s"); msgs[WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN] = _("The following paths were already present and thus not updated despite sparse patterns:\n%s"); @@ -548,6 +553,23 @@ static int apply_sparse_checkout(struct index_state *istate, return 0; } +static int warn_conflicted_path(struct index_state *istate, + int i, + struct unpack_trees_options *o) +{ + char *conflicting_path = istate->cache[i]->name; + int count = 0; + + add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path); + + /* Find out how many higher stage entries at same path */ + while (++count < istate->cache_nr && + !strcmp(conflicting_path, + istate->cache[i+count]->name)) + /* do nothing */; + return count; +} + static inline int call_unpack_fn(const struct cache_entry * const *src, struct unpack_trees_options *o) { @@ -1793,6 +1815,14 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o) for (i = 0; i < o->src_index->cache_nr; i++) { struct cache_entry *ce = o->src_index->cache[i]; + + if (ce_stage(ce)) { + /* -1 because for loop will increment by 1 */ + i += warn_conflicted_path(o->src_index, i, o) - 1; + ret = UPDATE_SPARSITY_WARNINGS; + continue; + } + if (apply_sparse_checkout(o->src_index, ce, o)) ret = UPDATE_SPARSITY_WARNINGS; diff --git a/unpack-trees.h b/unpack-trees.h index dae948205f..0f7424aec6 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -27,6 +27,7 @@ enum unpack_trees_error_types { NB_UNPACK_TREES_ERROR_TYPES, WARNING_SPARSE_NOT_UPTODATE_FILE, + WARNING_SPARSE_UNMERGED_FILE, WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN, NB_UNPACK_TREES_WARNING_TYPES, From 681c637b4ae1c46c09edda62a1aed6eaa69a2649 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:49:00 +0000 Subject: [PATCH 17/18] unpack-trees: failure to set SKIP_WORKTREE bits always just a warning Setting and clearing of the SKIP_WORKTREE bit is not only done when users run 'sparse-checkout'; other commands such as 'checkout' also run through unpack_trees() which has logic for handling this special bit. As such, we need to consider how they handle special cases. A couple comparison points should help explain the rationale for changing how unpack_trees() handles these bits: Ignoring sparse checkouts for a moment, if you are switching branches and have dirty changes, it is only considered an error that will prevent the branch switching from being successful if the dirty file happens to be one of the paths with different contents. SKIP_WORKTREE has always been considered advisory; for example, if rebase or merge need or even want to materialize a path as part of their work, they have always been allowed to do so regardless of the SKIP_WORKTREE setting. This has been used for unmerged paths, but it was often used for paths it wasn't needed just because it made the code simpler. It was a best-effort consideration, and when it materialized paths contrary to the SKIP_WORKTREE setting, it was never required to even print a warning message. In the past if you trying to run e.g. 'git checkout' and: 1) you had a path that was materialized and had some dirty changes 2) the path was listed in $GITDIR/info/sparse-checkout 3) this path did not different between the current and target branches then despite the comparison points above, the inability to set SKIP_WORKTREE was treated as a *hard* error that would abort the checkout operation. This is completely inconsistent with how SKIP_WORKTREE is handled elsewhere, and rather annoying for users as leaving the paths materialized in the working copy (with a simple warning) should present no problem at all. Downgrade any errors from inability to toggle the SKIP_WORKTREE bit to a warning and allow the operations to continue. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t1011-read-tree-sparse-checkout.sh | 11 +++++----- t/t2018-checkout-branch.sh | 22 ++++++++++++++++++++ unpack-trees.c | 31 ++++++++++++++-------------- 3 files changed, 43 insertions(+), 21 deletions(-) diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh index eb44bafb59..63223e13bd 100755 --- a/t/t1011-read-tree-sparse-checkout.sh +++ b/t/t1011-read-tree-sparse-checkout.sh @@ -233,18 +233,19 @@ test_expect_success 'read-tree --reset removes outside worktree' ' test_must_be_empty result ' -test_expect_success 'print errors when failed to update worktree' ' +test_expect_success 'print warnings when some worktree updates disabled' ' echo sub >.git/info/sparse-checkout && git checkout -f init && mkdir sub && touch sub/added sub/addedtoo && - test_must_fail git checkout top 2>actual && + # Use -q to suppress "Previous HEAD position" and "Head is now at" msgs + git checkout -q top 2>actual && cat >expected <<\EOF && -error: The following untracked working tree files would be overwritten by checkout: +warning: The following paths were already present and thus not updated despite sparse patterns: sub/added sub/addedtoo -Please move or remove them before you switch branches. -Aborting + +After fixing the above paths, you may want to run `git sparse-checkout reapply`. EOF test_i18ncmp expected actual ' diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh index bbca7ef8da..21583154d8 100755 --- a/t/t2018-checkout-branch.sh +++ b/t/t2018-checkout-branch.sh @@ -238,4 +238,26 @@ test_expect_success 'checkout -b after clone --no-checkout does a checkout of HE test_path_is_file dest/a.t ' +test_expect_success 'checkout -b to a new branch preserves mergeable changes despite sparse-checkout' ' + test_when_finished " + git reset --hard && + git checkout branch1-scratch && + test_might_fail git branch -D branch3 && + git config core.sparseCheckout false && + rm .git/info/sparse-checkout" && + + test_commit file2 && + + echo stuff >>file1 && + echo file2 >.git/info/sparse-checkout && + git config core.sparseCheckout true && + + CURHEAD=$(git rev-parse HEAD) && + do_checkout branch3 $CURHEAD && + + echo file1 >expect && + git diff --name-only >actual && + test_cmp expect actual +' + test_done diff --git a/unpack-trees.c b/unpack-trees.c index dec044339d..b43f3e775a 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1701,23 +1701,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options * correct CE_NEW_SKIP_WORKTREE */ if (ce->ce_flags & CE_ADDED && - verify_absent(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) { - if (!o->show_all_errors) - goto return_failed; - ret = -1; - } + verify_absent(ce, WARNING_SPARSE_ORPHANED_NOT_OVERWRITTEN, o)) + ret = 1; + + if (apply_sparse_checkout(&o->result, ce, o)) + ret = 1; - if (apply_sparse_checkout(&o->result, ce, o)) { - if (!o->show_all_errors) - goto return_failed; - ret = -1; - } if (!ce_skip_worktree(ce)) empty_worktree = 0; - } - if (ret < 0) - goto return_failed; /* * Sparse checkout is meant to narrow down checkout area * but it does not make sense to narrow down to empty working @@ -1728,6 +1720,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options ret = unpack_failed(o, "Sparse checkout leaves no entry on working directory"); goto done; } + if (ret == 1) { + /* + * Inability to sparsify or de-sparsify individual + * paths is not an error, but just a warning. + */ + if (o->show_all_errors) + display_warning_msgs(o); + ret = 0; + } } ret = check_updates(o, &o->result) ? (-2) : 0; @@ -1759,10 +1760,8 @@ done: return ret; return_failed: - if (o->show_all_errors) { + if (o->show_all_errors) display_error_msgs(o); - display_warning_msgs(o); - } mark_all_ce_unused(o->src_index); ret = unpack_failed(o, NULL); if (o->exiting_early) From 5644ca28cded68972c74614fc06d6e0e1db1a7de Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:49:01 +0000 Subject: [PATCH 18/18] sparse-checkout: provide a new reapply subcommand If commands like merge or rebase materialize files as part of their work, or a previous sparse-checkout command failed to update individual files due to dirty changes, users may want a command to simply 'reapply' the sparsity rules. Provide one. Reviewed-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- Documentation/git-sparse-checkout.txt | 10 +++++++ builtin/sparse-checkout.c | 10 ++++++- t/t1091-sparse-checkout-builtin.sh | 41 +++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt index c0342e5393..1a3ace6082 100644 --- a/Documentation/git-sparse-checkout.txt +++ b/Documentation/git-sparse-checkout.txt @@ -70,6 +70,16 @@ C-style quoted strings. `core.sparseCheckoutCone` is enabled, the given patterns are interpreted as directory names as in the 'set' subcommand. +'reapply:: + Reapply the sparsity pattern rules to paths in the working tree. + Commands like merge or rebase can materialize paths to do their + work (e.g. in order to show you a conflict), and other + sparse-checkout commands might fail to sparsify an individual file + (e.g. because it has unstaged changes or conflicts). In such + cases, it can make sense to run `git sparse-checkout reapply` later + after cleaning up affected paths (e.g. resolving conflicts, undoing + or committing changes, etc.). + 'disable':: Disable the `core.sparseCheckout` config setting, and restore the working directory to include all files. Leaves the sparse-checkout diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index aa81199f85..95d0882417 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -18,7 +18,7 @@ static const char *empty_base = ""; static char const * const builtin_sparse_checkout_usage[] = { - N_("git sparse-checkout (init|list|set|add|disable) "), + N_("git sparse-checkout (init|list|set|add|reapply|disable) "), NULL }; @@ -554,6 +554,12 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix, return modify_pattern_list(argc, argv, m); } +static int sparse_checkout_reapply(int argc, const char **argv) +{ + repo_read_index(the_repository); + return update_working_directory(NULL); +} + static int sparse_checkout_disable(int argc, const char **argv) { struct pattern_list pl; @@ -603,6 +609,8 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix) return sparse_checkout_set(argc, argv, prefix, REPLACE); if (!strcmp(argv[0], "add")) return sparse_checkout_set(argc, argv, prefix, ADD); + if (!strcmp(argv[0], "reapply")) + return sparse_checkout_reapply(argc, argv); if (!strcmp(argv[0], "disable")) return sparse_checkout_disable(argc, argv); } diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 8e2976bc7b..dee99eeec3 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -370,6 +370,47 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged stat git -C unmerged sparse-checkout disable ' +test_expect_success 'sparse-checkout reapply' ' + git clone repo tweak && + + echo dirty >tweak/deep/deeper2/a && + + cat >input <<-EOF && + 0 0000000000000000000000000000000000000000 folder1/a + 100644 $(git -C tweak rev-parse HEAD:folder1/a) 1 folder1/a + EOF + git -C tweak update-index --index-info err && + test_i18ngrep "warning.*The following paths are not up to date" err && + test_i18ngrep "warning.*The following paths are unmerged" err && + + git -C tweak sparse-checkout set folder2 deep/deeper1 2>err && + test_i18ngrep "warning.*The following paths are not up to date" err && + test_i18ngrep "warning.*The following paths are unmerged" err && + + git -C tweak sparse-checkout reapply 2>err && + test_i18ngrep "warning.*The following paths are not up to date" err && + test_path_is_file tweak/deep/deeper2/a && + test_i18ngrep "warning.*The following paths are unmerged" err && + test_path_is_file tweak/folder1/a && + + git -C tweak checkout HEAD deep/deeper2/a && + git -C tweak sparse-checkout reapply 2>err && + test_i18ngrep ! "warning.*The following paths are not up to date" err && + test_path_is_missing tweak/deep/deeper2/a && + test_i18ngrep "warning.*The following paths are unmerged" err && + test_path_is_file tweak/folder1/a && + + git -C tweak add folder1/a && + git -C tweak sparse-checkout reapply 2>err && + test_must_be_empty err && + test_path_is_missing tweak/deep/deeper2/a && + test_path_is_missing tweak/folder1/a && + + git -C tweak sparse-checkout disable +' + test_expect_success 'cone mode: set with core.ignoreCase=true' ' rm repo/.git/info/sparse-checkout && git -C repo sparse-checkout init --cone &&