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

From: Jeff King <peff@peff.net>
Date: Mar 13, 2020, 02:34 UTC
Message-ID: <20200313023427.GB900007@coredump.intra.peff.net>
In-Reply-To: <20200313023034.GA900007@coredump.intra.peff.net>

On Thu, Mar 12, 2020 at 10:30:34PM -0400, Jeff King wrote:

Show 7 quoted lines

> 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).

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.

-Peff