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

[Jeff King <peff@peff.net>]

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

> 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