Re: [PATCH v3 00/23] Move exported packfile funcs to its own file

From: Jonathan Tan <jonathantanmy@google.com>
Date: Aug 21, 2017, 18:40 UTC
Message-ID: <20170821114015.30e1ab23@twelve2.svl.corp.google.com>
In-Reply-To: <xmqqd17q91y6.fsf@gitster.mtv.corp.google.com>

On Sat, 19 Aug 2017 23:40:33 -0700 Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I have to say that this was a painful topic to integrate.
> >
> > As you may know, the mk/use-size-t-in-zlib topic is being retracted
> > and getting rerolled as a larger size_t series, most of which still
> > needs help in reviewing.
> >
> > The jt/sha1-file-cleanup topic is the only one among the other four
> > that are still not in 'next', and I think that topic, as well as the
> > other three, are all good and serve as a good base to build on top.
> > So I first rebuilt your patches on top of these four topics.  This
> > took some time but it wasn't all that painful.
> 
> ... but it turns out that I screwed it up in at least one place,
> making Linux32 build fail (Thanks Lars and folks who pushed hard to
> arrange Travis to build all my pushes to 'pu').  I'm pushing out my
> second attempt.  Let's see how it goes.

Thanks.

Show 10 quoted lines

> A change like this that only moves code around without changing
> anything is painful to everybody to keep around, as nobody can
> safely touch the affected code while it is in flight.  On the other
> hand, as long as it is only moving code around, such a change is
> reasonably safe, and it is relatively easy to ensure that there is
> no change other than code movement is involved.  So let's 
> 
>  (1) make sure that the topics this depends on are sound by
>      re-reading them once again, and merge them quickly down to
>      'master';

I took a look and they look sound.

 - rs/find-pack-entry-bisection resolves an issue first introduced in
   commit 1f68855 ("[PATCH] Teach read_sha1_file() and friends about
   packed git object store.", 2005-06-27), which already had that issue.
 - jk/drop-sha1-entry-pos is some code deletion.
 - rs/unpack-entry-leakfix ensures that delta_stack is freed. This
   function does not (for example) expose the destination of delta_stack
   to its caller, so it is correct that delta_stack should be freed
   unless it points to the local buffer, just like in the success case.
 - jt/sha1-file-cleanup (my patches) still looks OK to me.

In your latest "What's cooking" (Aug 2017, #04; Fri, 18), you mentioned that the first 3 will be merged to master, and the 4th will be merged to next.

I didn't look at mk/use-size-t-in-zlib, which (as you said) is still under review.

Show 19 quoted lines

>  (2) merge this topic to 'next', optionally after rebasing it on
>      'master', after (1) happens; and
> 
>  (3) quickly merge it to 'master', to get it over with.
> 
> In the meantime we'd need to refrain from taking code that touch
> things that are moved by this series.
> 
> I plan to be offline for a week or so near the end of this month, so
> I am hoping that we can do all of the above before that. That may
> make us break our usual "tip of the 'master' is more stable and
> robust than any released version" promise by potentially leaving it
> broken for a while, but nobody can build on top of a fluid codebase
> that is in the process of moving things around in a big way, so it
> might not be such a bad idea to make it coincide with the period
> when the tree must become quiescent due to my being offline.  We'll
> see.
> 
> Thanks.