Re: [PATCH v2 0/2] Avoid rewriting "packed-refs" unnecessarily

From: Jeff King <peff@peff.net>
Date: Nov 1, 2017, 07:34 UTC
Message-ID: <20171101073414.rwi426whpinv226l@sigill.intra.peff.net>
In-Reply-To: <cover.1509181545.git.mhagger@alum.mit.edu>

On Sat, Oct 28, 2017 at 11:16:00AM +0200, Michael Haggerty wrote:

Show 18 quoted lines

> This reroll make some logically small changes to v1 [1] that are
> textually very big:
> 
> * Invert the sense of `is_packed_transaction_noop()` and rename it to
>   `is_packed_transaction_needed()`. This makes the logic easier to
>   follow and document.
> 
> * Add a big comment to that function, describing the cases when it
>   returns false positives and explaining why that isn't a problem.
> 
> * In the commit message for patch 02, gives a lot more information
>   about the regression that it is fixing. Thanks to Eric for the
>   suggestion.
> 
> These patches are also available as branch
> `avoid-rewriting-packed-refs` on my GitHub fork [2]. They now use
> `mh/packed-ref-transactions` as the base, since that is where Junio
> chose to apply v1.

This all makes sense to me. I agree that the "is_needed" logic-flip in v2 makes it a little easier to think about.

Like Junio, I was thrown off at first by the HAVE_OLD check. Especially since we would not ever set that flag for the transaction we care about here. But I think the crux of it is that the packed_ref store code could in theory operate independently of the loose ref code, where we feed it more exotic inputs. And what you've written here is future-proofing against the more general case, even though it would not be strictly necessary.

-Peff