Re: [PATCH] connected.c: reprepare packs for corner cases

From: Derrick Stolee <stolee@gmail.com>
Date: Mar 13, 2020, 12:43 UTC
Message-ID: <8eb7c0f3-0667-2eb7-7101-5790bcff784c@gmail.com>
In-Reply-To: <20200313023427.GB900007@coredump.intra.peff.net>

On 3/12/2020 10:34 PM, Jeff King wrote:

Show 20 quoted lines

> On Thu, Mar 12, 2020 at 10:30:34PM -0400, Jeff King wrote:
> 
>> So that would actually argue that your patch is putting it in the right
>> place. It's _not_ fetch's responsibility to reprepare_packed_git(). It's
>> the loop in check_connected() that is skipping the usual reprepare
>> logic, and shouldn't.
>>
>> And one fix (which you did) is to just preemptively reprepare right
>> above that loop. Some other solutions are:
> 
> I know I've now suggested that your patch is both wrong and right. :)
> 
> Just to be clear, at this point I think I'd be OK with either solution.
> 
> If it's going into check_connected(), the commit message should argue
> that the loop there is at fault for not doing the usual fallback, and a
> single explicit reprepare() is cheap enough to cover the case we care
> about (and that we don't have to worry about racing with somebody else
> repacking because the point of that flag is that we're in a brand new
> repo).

Thank you for bringing extra clarity here. We don't need to put the reprepare in the fetch logic because _most_ kinds of object lookups will reprepare when failing to find an expected object. This loop is special in that it is restricted to search the promisor packs. And, because it only happens when a special mode (opt->check_refs_are_promisor_objects_only) it is reasonable to assume that we are specifically in the case that a pack was added recently.

We could save some work in most cases by repreparing only after failing
to find the object, but that also makes the code more complicated for
low value. At least, that is my opinion.

> Repreparing earlier in the transport-helper code _could_ still protect
> against other similar loops, which is an argument for putting it there.
> But I'd be inclined to say those other loops should be corrected.

Right. We should make it the responsibility of the code that is scanning pack-files that they should be able to reprepare when failing to find "expected" objects. This also handles the concurrent repack case that Junio mentioned.

Taking a quick glance at the callers of get_all_packs(), I see this is the only such loop that both has an ability to "fail" based on an expected object and doesn't have a fallback to reprepare.

Thanks, -Stolee