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

Taylor Blau [off-list ref] 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.