From: Jeff King <peff@peff.net>
Date: Wed, 28 Oct 2015 17:27:58 GMT
Subject: Re: [PATCH] add_submodule_odb: initialize alt_odb list earlier
Message-ID: <20151028172758.GA21851@sigill.intra.peff.net>
In-Reply-To: <xmqqa8r3m2xq.fsf@gitster.mtv.corp.google.com>

On Wed, Oct 28, 2015 at 08:24:17AM -0700, Junio C Hamano wrote:

> > Note that we can remove the prepare_alt_odb call from the
> > end. It is guaranteed to be a noop, since we will have
> > called it earlier.
> 
> Thanks for a quick and detailed diagnosis and a fix.
> 
> The removal is correct, but even without this fix, the order of
> calls in the original should have screamed "bug" loudly at us, I
> think.  We shouldn't be reading data from alternates file without
> first preparing the place we read data into.

Yeah, I agree. I spent a long time trying to figure out if that
prepare_alt_odb was actually doing something useful (like if it was
needed to somehow "cement" the new alt into place).

But I don't think it was.

In the majority of cases, it was a noop (we had already prepared when we
looked up the first object). But for other cases...

  - if read_info_alternates actually did something, we segfaulted (i.e.,
    this bug)

  - otherwise, we would prepare on _top_ of what we just added to the
    list, which was probably buggy (I didn't dig far enough to see if
    prepare_alt_odb() would overwrite what we just added to the list).

So some pretty dark corners of the code. :)

-Peff