Re: [PATCH] unpack-trees: also allow get_progress() to work on a different index

[Elijah Newren <newren@gmail.com>]

On Thu, May 14, 2020 at 3:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> 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 <torvalds@linux-foundation.org>
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.