All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Junio C Hamano <gitster@pobox.com>,
	Eric Sunshine <sunshine@sunshineco.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2 0/2] Avoid rewriting "packed-refs" unnecessarily
Date: Wed, 1 Nov 2017 03:34:14 -0400	[thread overview]
Message-ID: <20171101073414.rwi426whpinv226l@sigill.intra.peff.net> (raw)
In-Reply-To: <cover.1509181545.git.mhagger@alum.mit.edu>

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

      parent reply	other threads:[~2017-11-01  7:38 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-28  9:16 [PATCH v2 0/2] Avoid rewriting "packed-refs" unnecessarily Michael Haggerty
2017-10-28  9:16 ` [PATCH v2 1/2] t1409: check that `packed-refs` is not rewritten unnecessarily Michael Haggerty
2017-10-28  9:16 ` [PATCH v2 2/2] files-backend: don't rewrite the `packed-refs` file unnecessarily Michael Haggerty
2017-10-30  4:52   ` Junio C Hamano
2017-11-01  7:34 ` Jeff King [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171101073414.rwi426whpinv226l@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mhagger@alum.mit.edu \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.