Re: [PATCH v6] fetch-pack: always allow fetching of literal SHA1s

From: Jeff King <peff@peff.net>
Date: May 13, 2017, 08:36 UTC
Message-ID: <20170513083615.dyfnmug35ghhy7ba@sigill.intra.peff.net>
In-Reply-To: <20170512204648.10611-1-jonathantanmy@google.com>

On Fri, May 12, 2017 at 01:46:48PM -0700, Jonathan Tan wrote:

> Change from v5: used "ensure_tip_oids_initialized" function instead.
> This removes some of the muddiness (e.g. with newlist being modified
> after the function).

I don't think it really improves the muddiness. You are still calling the ensure function each time through the loop with a potentially modified list, but it doesn't actually look at the list after the first time. So the muddy part is still there.

I think rather than changing the code, you'd do better with a comment like:

  /*
   * Note that this only looks at the ref lists the first time
   * it's called. This works out in filter_refs() because even
   * though it may add to "newlist" between calls, the additions
   * will always be for oids that are already in the set.
   */

At which point the original all-in-one function is probably fine (as it avoids the "return 1 just to join the &&-chain" bit).

-Peff