[v2,09/18] unpack-trees: add a new update_sparsity() function

Commit Message

Johannes Schindelin via GitGitGadget March 21, 2020, 6 p.m. UTC

From: Elijah Newren <[e-mail]>

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 <[e-mail]>
---
 unpack-trees.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.h |  9 ++++++
 2 files changed, 87 insertions(+)

Comments

Derrick Stolee March 23, 2020, 6:02 p.m. UTC | #1

On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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

Elijah Newren March 23, 2020, 6:10 p.m. UTC | #2

On Mon, Mar 23, 2020 at 11:02 AM Derrick Stolee <[e-mail]> wrote:
>
> On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:

> > +     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

Will clean it up.

> > +     }
> > +
> > +     /*
> > +      * 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()?

You mean move it to unpack-trees.c?  If I did that, the code in
sparse-checkout.c that uses two of these values would fail to compile.

> >  /*
> >   * 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.

Whoops!  Will fix.  (Interesting that the compiler didn't flag any
kind of warning based on mismatch of declared function return types
for update_sparsity() in the .c and .h files...)

Thanks,
Elijah

Derrick Stolee March 23, 2020, 6:21 p.m. UTC | #3

On 3/23/2020 2:10 PM, Elijah Newren wrote:
> On Mon, Mar 23, 2020 at 11:02 AM Derrick Stolee <[e-mail]> wrote:
>>
>> On 3/21/2020 2:00 PM, Elijah Newren via GitGitGadget wrote:
>>> +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()?
> 
> You mean move it to unpack-trees.c?  If I did that, the code in
> sparse-checkout.c that uses two of these values would fail to compile.

No, I just meant the function declaration below in unpack-trees.h.

>>> +int update_sparsity(struct unpack_trees_options *options);
>>> +
>>
>> This appears to not use the enum as it should.
> 
> Whoops!  Will fix.  (Interesting that the compiler didn't flag any
> kind of warning based on mismatch of declared function return types
> for update_sparsity() in the .c and .h files...)

*shrug* enums are essentially decoration over an int, so I'm not
surprised it can happen.

-Stolee

Junio C Hamano March 23, 2020, 8:24 p.m. UTC | #4

Derrick Stolee <[e-mail]> writes:

>>>> +int update_sparsity(struct unpack_trees_options *options);
>>>> +
>>>
>>> This appears to not use the enum as it should.
>> 
>> Whoops!  Will fix.  (Interesting that the compiler didn't flag any
>> kind of warning based on mismatch of declared function return types
>> for update_sparsity() in the .c and .h files...)
>
> *shrug* enums are essentially decoration over an int, so I'm not
> surprised it can happen.

;-)  

Yeah, the value of preferring enum over preprocessor macros because
it helps debuggers to show symbolic constants, is oversold, I think.
Types of variables and functions' return values need to be set to
the enum or the benefit won't come to us X-<.

Patch

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();
+
+	/* 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 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
+};
+
 /*
  * 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);
+
 int verify_uptodate(const struct cache_entry *ce,
 		    struct unpack_trees_options *o);