Re: [ANNOUNCE] Git v2.29.0-rc0

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 7, 2020, 18:41 UTC
Message-ID: <20201007184143.1304267-1-jonathantanmy@google.com>
In-Reply-To: <xmqqlfgi2axr.fsf@gitster.c.googlers.com>

Show 28 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > Hmm. That commit removes the call to display_progress() from the main
> > loop of threaded_second_pass(), and doesn't appear to add another one
> > anywhere. Is the solution really as simple as adding it back in? I.e.
> >
> > diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> > index 8acd078aa0..6dbb4317e0 100644
> > --- a/builtin/index-pack.c
> > +++ b/builtin/index-pack.c
> > @@ -1028,6 +1028,10 @@ static void *threaded_second_pass(void *data)
> >  		struct object_entry *child_obj;
> >  		struct base_data *child;
> >  
> > +		counter_lock();
> > +		display_progress(progress, nr_resolved_deltas);
> > +		counter_unlock();
> > +
> >  		work_lock();
> >  		if (list_empty(&work_head)) {
> >  			/*
> >
> > That _seems_ to work fine, but I'm not sure why it was removed in the
> > first place (for a good reason, or simply as an accident when rewriting
> > the variable declarations at the top of the loop?).
> 
> The above looks like an obvious and trivial fix to go back closer to
> the original.

Thanks for noticing it, Peff. Yes, that looks like the correct fix. I thought that it might have crept in when I was rebasing, but looks like it was present in the original version [1].

[1] https://lore.kernel.org/git/505d8e79cd983d5b3dfd56c4f0432ad647132957.1570663470.git.jonathantanmy@google.com/

Show 6 quoted lines

> We seem to have removed find_unresolved_deltas() helper function in
> that series, but there remains a mention to it in a comment, which
> we would probably should rethink (it just may be the matter of
> removing the mention, or if "just like in ..." may have been the
> last example of doing what the comment suggests all code to do, it
> may reveal a need for larger clean-up---I dunno).

Maybe the whole comment block should be deleted and replaced with something like:

 Ensure that this node has been uncompressed and return its contents.

 In the typical and best case, this node would already be uncompressed
 (through the invocation to resolve_delta() in threaded_second_pass())
 and it would not be pruned. However, if pruning of this node was
 necessary due to reaching delta_base_cache_limit, this function will
 find the closest ancestor with uncompressed data that has not been
 pruned (or if there is none, the ultimate base object), and uncompress
 each node in the delta chain in order to generate the uncompressed data
 for this node.

(I'm using "uncompress" here because I find the original "deflate" term confusing - I thought that "deflate" meant compress, and thus the "data" here would be the uncompressed form, and hence undeflated or inflated.)

The original comment [2] was describing two paths in which uncompressed forms could be generated - one in find_unresolved_deltas() going from parent to children, and one here from "current node to top parent"; and it said that all generated uncompressed forms (here, and "just like in find_unresolved_deltas()" - as Junio quoted) can be pruned. I think my suggested comment block above retains that information.

[2] https://kernel.googlesource.com/pub/scm/git/git/+/refs/tags/v2.28.0/builtin/index-pack.c#868