From d7dc1e1668fcbd5af019d0bc059b8dcb35743c86 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:45 +0000 Subject: 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.h | 1 - 1 file changed, 1 deletion(-) (limited to 'unpack-trees.h') 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 }; -- cgit v1.2.3 From fa0bde45cdebee8bac95e66e4bf5ba16c77fbdba Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:47 +0000 Subject: 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 --- unpack-trees.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'unpack-trees.h') 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; -- cgit v1.2.3 From 7af7a25853cb87f34d192ba1d813ca09ee15d9f4 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:52 +0000 Subject: 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.h | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'unpack-trees.h') 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); -- cgit v1.2.3 From cd002c15611a995a29b9e5881745bba402d63952 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:55 +0000 Subject: 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.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'unpack-trees.h') 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 }; -- cgit v1.2.3 From 1ac83f42da6437d74098f162fcfa37474c94e223 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:56 +0000 Subject: 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.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'unpack-trees.h') 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, }; /* -- cgit v1.2.3 From 6271d77cb1ccfb7e4124593b4463446fe1188cda Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:57 +0000 Subject: 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 --- unpack-trees.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'unpack-trees.h') 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; -- cgit v1.2.3 From ebb568b9e2539ce2dbb3853ff3d4869a5c690601 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 27 Mar 2020 00:48:59 +0000 Subject: 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 --- unpack-trees.h | 1 + 1 file changed, 1 insertion(+) (limited to 'unpack-trees.h') 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, -- cgit v1.2.3