Re: [PATCH 3/5] gc: refactor a "call me once" pattern

From: Jeff King <peff@peff.net>
Date: Mar 14, 2019, 00:30 UTC
Message-ID: <20190314003016.GH31968@sigill.intra.peff.net>
In-Reply-To: <20190313235439.30439-4-avarab@gmail.com>

On Thu, Mar 14, 2019 at 12:54:37AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

> Change an idiom we're using to ensure that gc_before_repack() only
> does work once (see 62aad1849f ("gc --auto: do not lock refs in the
> background", 2014-05-25)) to be more obvious.
> 
> Nothing except this function cares about the "pack_refs" and
> "prune_reflogs" variables, so let's not leave the reader wondering if
> they're being zero'd out for later use somewhere else.

I agree the existing code is not very obvious about what it's trying to do. I think a comment would have helped a lot.

Your rewrite is definitely better than the original, but I think it might also benefit from a comment. I.e., something like:

>  static void gc_before_repack(void)
>  {

	/*
	 * We may be called twice, as both the pre- and post-daemonized
	 * phases will call us, but running these commands more than
	 * once is pointless and wasteful.
	 */

> +	static int done = 0;
> +	if (done++)
> +		return;

-Peff