Taylor Blau <me@xxxxxxxxxxxx> 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.
