On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> 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.
>
> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> ---
> unpack-trees.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
> unpack-trees.h | 9 ++++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 4733e7eaf89..6abea555929 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1714,6 +1714,84 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> 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();
I was about to say "why didn't you use the trace2 regions like in
unpack_trees()?" when I discovered that we haven't sent them [1]
upstream yet. I'll put that on my TODO list.
[1] https://github.com/microsoft/git/commit/9a04644e14fe4aeb556dfc30cb2220b799f53448
> + /* If we weren't given patterns, use the recorded ones */
> + if (!o->pl) {
> + memset(&pl, 0, sizeof(pl));
> + free_pattern_list = 1;
I notice you are using the same free_pattern_list pattern as your
earlier commit. Good.
> + 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;
> +
nit: extra whitespace-only line
> + }
> +
> + /*
> + * 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 d3516267f36..2c5d54cae9f 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -28,6 +28,13 @@ enum unpack_trees_error_types {
> NB_UNPACK_TREES_ERROR_TYPES
> };
>
> +enum update_sparsity_result {
> + UPDATE_SPARSITY_SUCCESS = 0,
> + UPDATE_SPARSITY_WARNINGS = 1,
> + UPDATE_SPARSITY_INDEX_UPDATE_FAILURES = -1,
> + UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
> +};
> +
Is there a reason this isn't located just before
update_sparsity()?
> /*
> * Sets the list of user-friendly error messages to be used by the
> * command "cmd" (either merge or checkout), and show_all_errors to 1.
> @@ -88,6 +95,8 @@ struct unpack_trees_options {
> int unpack_trees(unsigned n, struct tree_desc *t,
> struct unpack_trees_options *options);
>
> +int update_sparsity(struct unpack_trees_options *options);
> +
This appears to not use the enum as it should.
Thanks,
-Stolee
