Re: [PATCH v2 0/8] repack: introduce `--write-midx`

From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, peff@peff.net, avarab@gmail.com
Subject: Re: [PATCH v2 0/8] repack: introduce `--write-midx`
Date: Wed, 15 Sep 2021 12:22:08 -0700	[thread overview]
Message-ID: <xmqqmtodwsnz.fsf@gitster.g> (raw)
In-Reply-To: <cover.1631730270.git.me@ttaylorr.com> (Taylor Blau's message of "Wed, 15 Sep 2021 14:24:33 -0400")

Taylor Blau <me@ttaylorr.com> writes:

> Nothing substantial has changed since last time. Mostly re-wrapping lines and
> removing braces in macro'd for_each_xyz() loops....
> Otherwise, this series is unchagned. It depends on tb/multi-pack-bitmaps, which
> has graduated to master. Range-diff below:

Despite the explanation above ...

>      @@ midx.c: static void add_pack_to_midx(const char *full_path, size_t full_path_len,
>     - 
>     - 	if (ends_with(file_name, ".idx")) {
>       		display_progress(ctx->progress, ++ctx->pack_paths_checked);
>     --		if (ctx->m && midx_contains_pack(ctx->m, file_name))
>     --			return;
>     -+		if (ctx->m) {
>     -+			if (midx_contains_pack(ctx->m, file_name))
>     -+				return;
>     -+		} else if (ctx->to_include) {
>     -+			if (!string_list_has_string(ctx->to_include, file_name))
>     -+				return;
>     -+		}
>     + 		if (ctx->m && midx_contains_pack(ctx->m, file_name))
>     + 			return;
>     ++		else if (ctx->to_include &&
>     ++			 !string_list_has_string(ctx->to_include, file_name))
>     ++			return;

... this part seems to change what the code does quite a bit.

The original used to skip .to_include checks when .m is set but it
did not pass midx_contains_pack().  Now .to_include check is done
in a context with .m that does not pass midex_contains_pack() check.

I suspect that this change since v1 is a bugfix.  We make an early
return based on midx_contains_pack() but make sure .m is not NULL,
in order to be able to run the check.  There is another early return
condition that is orthogonal, and we do string_list check, but that
is only doable if the .to_include is not NULL.  The logic in v2 
clearly expresses that, but v1 was simply buggy.

But if it is the case, I'd step back a bit and further question if
"else if" is a good construct to use here.  We'd return if .m passes
midx_contains_pack() check, and another check based on .to_include
gives us an orthogonal chance to return early, so two "if" statement
that are independent sitting next to each other may have avoided
such a bug from the beginning, perhaps?

	if (ctx->m && midx_contains...)
		return;
	if (ctx->to_include && !string_list_has...)
		return;

Of course you could

	if ((ctx->m && midx_contains...) ||
	    (ctx->to_include && !string_list_has...))
		return;

but that may not scale as well as two independent if statements.

Everything else in the range-diff looked cosmetic as explained in
the cover letter.