unpack-trees: also allow get_progress() to work on a different index

Commit Message

Johannes Schindelin via GitGitGadget May 14, 2020, 7:53 p.m. UTC

From: Elijah Newren <[e-mail]>

commit b0a5a12a60 ("unpack-trees: allow check_updates() to work on a
different index", 2020-03-27) allowed check_updates() to work on a
different index, but it called get_progress() which was hardcoded to
work on o->result much like check_updates() had been.  Update it to also
accept an index parameter and have check_updates() pass that parameter
along so that both are working on the same index.

Noticed-by: Jeff Hostetler <[e-mail]>
Signed-off-by: Elijah Newren <[e-mail]>
---
    unpack-trees: also allow get_progress() to work on a different index
    
    This is a fix to a minor 2.27 regression, from the en/sparse-checkout
    series.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-785%2Fnewren%2Frecent-sparse-checkout-bugfix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-785/newren/recent-sparse-checkout-bugfix-v1
Pull-Request: https://github.com/git/git/pull/785

 unpack-trees.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: b994622632154fc3b17fb40a38819ad954a5fb88

Comments

Junio C Hamano May 14, 2020, 8:22 p.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

> From: Elijah Newren <[e-mail]>
>
> commit b0a5a12a60 ("unpack-trees: allow check_updates() to work on a
> different index", 2020-03-27) allowed check_updates() to work on a
> different index, but it called get_progress() which was hardcoded to
> work on o->result much like check_updates() had been.  Update it to also
> accept an index parameter and have check_updates() pass that parameter
> along so that both are working on the same index.
>
> Noticed-by: Jeff Hostetler <[e-mail]>
> Signed-off-by: Elijah Newren <[e-mail]>
> ---
>     unpack-trees: also allow get_progress() to work on a different index
>     
>     This is a fix to a minor 2.27 regression, from the en/sparse-checkout
>     series.

OK, so update_sparsity() calls the function with o->src_index,
get_progress() without this fix would iterate over an unrelated
o->result to count the cache entries that are marked for update or
removal and show the progress based on that number without this
patch?  The other caller in unpack_trees() uses o->result so there
is no change in the behaviour with or without the patch.

The flag bits on cache entries used to count the entries in the
index by the get_progress() function are applied only on the
o->result side when merged_entry() and friends call do_add_entry(),
no?  

Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache
entries in the o->src_index array when get_progress() is fed the
src_index in the first place?

>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-785%2Fnewren%2Frecent-sparse-checkout-bugfix-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-785/newren/recent-sparse-checkout-bugfix-v1
> Pull-Request: https://github.com/git/git/pull/785
>
>  unpack-trees.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 1fe3764f2b2..2e6e9d5eeb8 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -333,10 +333,10 @@ static void load_gitmodules_file(struct index_state *index,
>  	}
>  }
>  
> -static struct progress *get_progress(struct unpack_trees_options *o)
> +static struct progress *get_progress(struct unpack_trees_options *o,
> +				     struct index_state *index)
>  {
>  	unsigned cnt = 0, total = 0;
> -	struct index_state *index = &o->result;
>  
>  	if (!o->update || !o->verbose_update)
>  		return NULL;
> @@ -415,7 +415,7 @@ static int check_updates(struct unpack_trees_options *o,
>  	if (o->clone)
>  		setup_collided_checkout_detection(&state, index);
>  
> -	progress = get_progress(o);
> +	progress = get_progress(o, index);
>  
>  	git_attr_set_direction(GIT_ATTR_CHECKOUT);
>  
>
> base-commit: b994622632154fc3b17fb40a38819ad954a5fb88

Elijah Newren May 14, 2020, 9:46 p.m. UTC | #2

On Thu, May 14, 2020 at 1:22 PM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>
> > From: Elijah Newren <[e-mail]>
> >
> > commit b0a5a12a60 ("unpack-trees: allow check_updates() to work on a
> > different index", 2020-03-27) allowed check_updates() to work on a
> > different index, but it called get_progress() which was hardcoded to
> > work on o->result much like check_updates() had been.  Update it to also
> > accept an index parameter and have check_updates() pass that parameter
> > along so that both are working on the same index.
> >
> > Noticed-by: Jeff Hostetler <[e-mail]>
> > Signed-off-by: Elijah Newren <[e-mail]>
> > ---
> >     unpack-trees: also allow get_progress() to work on a different index
> >
> >     This is a fix to a minor 2.27 regression, from the en/sparse-checkout
> >     series.
>
> OK, so update_sparsity() calls the function with o->src_index,
> get_progress() without this fix would iterate over an unrelated
> o->result to count the cache entries that are marked for update or
> removal and show the progress based on that number without this
> patch?  The other caller in unpack_trees() uses o->result so there
> is no change in the behaviour with or without the patch.

Yes.

> The flag bits on cache entries used to count the entries in the
> index by the get_progress() function are applied only on the
> o->result side when merged_entry() and friends call do_add_entry(),
> no?

Not quite; unpack_trees() also calls apply_sparse_checkout(), passing
it o->result, and apply_sparse_checkout() also sets those bits.

> Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache
> entries in the o->src_index array when get_progress() is fed the
> src_index in the first place?

Yes, before calling check_updates(o, o->src_index), update_sparsity()
loops over o->src_index and calls apply_sparse_checkout() on each of
the non-conflicted cache entries.  apply_sparse_checkout() will set
either CE_UPDATE or CE_WT_REMOVE whenever items flip from or to having
the SKIP_WORKTREE bit set.

Junio C Hamano May 14, 2020, 10:16 p.m. UTC | #3

Elijah Newren <[e-mail]> writes:

>> Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache
>> entries in the o->src_index array when get_progress() is fed the
>> src_index in the first place?
>
> Yes, before calling check_updates(o, o->src_index), update_sparsity()
> loops over o->src_index and calls apply_sparse_checkout() on each of
> the non-conflicted cache entries.  apply_sparse_checkout() will set
> either CE_UPDATE or CE_WT_REMOVE whenever items flip from or to having
> the SKIP_WORKTREE bit set.

Hmph.

I thought that the whole point of splitting o->result from
o->src_index we did long time ago was to allow us to treat
o->src_index constant.  I hope we haven't broken anything by
starting to do things like that X-<.

Anyway, if that is the case, this change won't make things any
worse.  Let's queue it.

Thanks.

Elijah Newren May 14, 2020, 11:21 p.m. UTC | #4

On Thu, May 14, 2020 at 3:17 PM Junio C Hamano <[e-mail]> wrote:
>
> Elijah Newren <[e-mail]> writes:
>
> >> Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache
> >> entries in the o->src_index array when get_progress() is fed the
> >> src_index in the first place?
> >
> > Yes, before calling check_updates(o, o->src_index), update_sparsity()
> > loops over o->src_index and calls apply_sparse_checkout() on each of
> > the non-conflicted cache entries.  apply_sparse_checkout() will set
> > either CE_UPDATE or CE_WT_REMOVE whenever items flip from or to having
> > the SKIP_WORKTREE bit set.
>
> Hmph.
>
> I thought that the whole point of splitting o->result from
> o->src_index we did long time ago was to allow us to treat
> o->src_index constant.  I hope we haven't broken anything by
> starting to do things like that X-<.

I think we're safe there.  No function started modifying o->src_index
directly; they just modify the index they are passed in.  The only
place that passes o->src_index to functions for modification is
update_sparsity(), which unpack_trees() never calls.  In fact,
update_sparsity() was split out from unpack_trees().  But perhaps I
can address your concerns a bit better with the history and from a
high level instead of the low level details...

commit 34110cd4e394e3f92c01a4709689b384c34645d8
Author: Linus Torvalds <[e-mail]>
Date:   Thu Mar 6 18:12:28 2008 -0800

    Make 'unpack_trees()' have a separate source and destination index
...
    This ends up removing a fair number more lines than it adds, for the
    simple reason that we can now skip all the crud that tried to be
    oh-so-careful about maintaining our position in the index as we were
    traversing and modifying it.  Since we don't actually modify the source
    index any more, we can just update the 'o->pos' pointer without worrying
    about whether an index entry got removed or replaced or added to.


unpack_trees() has to do lots of work, including worrying about adding
and removing many cache entries.  Not having to worry about the
position pointer as you're traversing the index makes sense.
update_sparsity(), on the other hand, knows going in that no cache
entries will be added or removed; it will at most toggle some flags on
existing cache entries.  The same concerns thus just aren't relevant.
While we could make it use o->result instead of o->src_index, it would
imply cloning all the index entries before toggling some flags on the
existing entries, which seems like a waste of effort.  (In fact,
update_sparsity() never even initializes o->result, unlike
unpack_trees(), so any attempts to access it are undefined behavior.)

I agree we don't want the two mixed up, but isn't that achieved by the
fact that update_sparsity() was split out of unpack_trees() instead of
making update_sparsity() some kind of extra mode flag passed to
unpack_trees() (and which would have complicated unpack_trees() even
further)?

One thing that could possibly be improved is using o->dst_index
instead of o->src_index to make things slightly clearer, although it
wouldn't actually change things because of this sanity check from the
beginning of update_sparsity():

if (o->src_index != o->dst_index || o->fn)
        BUG("update_sparsity() called wrong");


Do any of those comments help?  Are there still other changes you'd
like to see (separate from this patch)?

> Anyway, if that is the case, this change won't make things any
> worse.  Let's queue it.

Thanks.

Junio C Hamano May 15, 2020, 3:01 p.m. UTC | #5

Elijah Newren <[e-mail]> writes:

> On Thu, May 14, 2020 at 3:17 PM Junio C Hamano <[e-mail]> wrote:
>>
>> Elijah Newren <[e-mail]> writes:
>>
>> >> Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache
>> >> entries in the o->src_index array when get_progress() is fed the
>> >> src_index in the first place?
>> >
>> > Yes, before calling check_updates(o, o->src_index), update_sparsity()
>> > loops over o->src_index and calls apply_sparse_checkout() on each of
>> > the non-conflicted cache entries.  apply_sparse_checkout() will set
>> > either CE_UPDATE or CE_WT_REMOVE whenever items flip from or to having
>> > the SKIP_WORKTREE bit set.
>>
>> Hmph.
>>
>> I thought that the whole point of splitting o->result from
>> o->src_index we did long time ago was to allow us to treat
>> o->src_index constant.  I hope we haven't broken anything by
>> starting to do things like that X-<.
>
> I think we're safe there.  No function started modifying o->src_index
> directly; they just modify the index they are passed in.  The only
> place that passes o->src_index to functions for modification is
> update_sparsity(), which unpack_trees() never calls.

OK.  Thanks for an additional detail.  Let's merge it down by -rc1.

Patch

diff --git a/unpack-trees.c b/unpack-trees.c
index 1fe3764f2b2..2e6e9d5eeb8 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -333,10 +333,10 @@  static void load_gitmodules_file(struct index_state *index,
 	}
 }
 
-static struct progress *get_progress(struct unpack_trees_options *o)
+static struct progress *get_progress(struct unpack_trees_options *o,
+				     struct index_state *index)
 {
 	unsigned cnt = 0, total = 0;
-	struct index_state *index = &o->result;
 
 	if (!o->update || !o->verbose_update)
 		return NULL;
@@ -415,7 +415,7 @@  static int check_updates(struct unpack_trees_options *o,
 	if (o->clone)
 		setup_collided_checkout_detection(&state, index);
 
-	progress = get_progress(o);
+	progress = get_progress(o, index);
 
 	git_attr_set_direction(GIT_ATTR_CHECKOUT);