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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 15, 2021, 19:22 UTC
Message-ID: <xmqqmtodwsnz.fsf@gitster.g>
In-Reply-To: <cover.1631730270.git.me@ttaylorr.com>

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 ...

Show 18 quoted lines

>      @@ 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.