Re: [PATCH v2 6/8] midx-write.c: support reading an existing MIDX with `packs_to_include`

From: Jeff King <peff@peff.net>
Date: May 30, 2024, 07:05 UTC
Message-ID: <20240530070540.GB1949834@coredump.intra.peff.net>
In-Reply-To: <fe187b1939d9c74782306cd3e4fcd2f25451ad69.1717023301.git.me@ttaylorr.com>

On Wed, May 29, 2024 at 06:55:39PM -0400, Taylor Blau wrote:

>  static int should_include_pack(const struct write_midx_context *ctx,
> -			       const char *file_name)
> +			       const char *file_name,
> +			       int exclude_from_midx)

Hmm, so we grow a new flag...

Show 24 quoted lines

>  {
> -	/*
> -	 * Note that at most one of ctx->m and ctx->to_include are set,
> -	 * so we are testing midx_contains_pack() and
> -	 * string_list_has_string() independently (guarded by the
> -	 * appropriate NULL checks).
> -	 *
> -	 * We could support passing to_include while reusing an existing
> -	 * MIDX, but don't currently since the reuse process drags
> -	 * forward all packs from an existing MIDX (without checking
> -	 * whether or not they appear in the to_include list).
> -	 *
> -	 * If we added support for that, these next two conditional
> -	 * should be performed independently (likely checking
> -	 * to_include before the existing MIDX).
> -	 */
> -	if (ctx->m && midx_contains_pack(ctx->m, file_name))
> +	if (exclude_from_midx && ctx->m && midx_contains_pack(ctx->m, file_name))
>  		return 0;
> -	else if (ctx->to_include &&
> -		 !string_list_has_string(ctx->to_include, file_name))
> +	if (ctx->to_include && !string_list_has_string(ctx->to_include,
> +						       file_name))
>  		return 0;

...that disables half of what the function is checking.

It makes me wonder if it should just be two functions (or if the callers should just inline them, since they are themselves basically one-liners). But I think this is getting into bike-shedding territory. I'm OK with it as-is.

-Peff