Re: [PATCH v4 8/9] midx: read `RIDX` chunk when present

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 26, 2022, 15:10 UTC
Message-ID: <220126.86lez2edae.gmgdl@evledraar.gmail.com>
In-Reply-To: <94563cf038722852d801bd0a8e744a569799f354.1643150456.git.me@ttaylorr.com>

On Tue, Jan 25 2022, Taylor Blau wrote:

Show 5 quoted lines

> @@ -298,9 +298,26 @@ int load_midx_revindex(struct multi_pack_index *m)
>  {
>  	struct strbuf revindex_name = STRBUF_INIT;
>  	int ret;
> +

nit: good addition for style in general, but a stay whitespace change in an otherwise narrowly focused patch.

Show 21 quoted lines

>  	if (m->revindex_data)
>  		return 0;
>  
> +	if (m->chunk_revindex) {
> +		/*
> +		 * If the MIDX `m` has a `RIDX` chunk, then use its contents for
> +		 * the reverse index instead of trying to load a separate `.rev`
> +		 * file.
> +		 *
> +		 * Note that we do *not* set `m->revindex_map` here, since we do
> +		 * not want to accidentally call munmap() in the middle of the
> +		 * MIDX.
> +		 */
> +		trace2_data_string("load_midx_revindex", the_repository,
> +				   "source", "midx");
> +		m->revindex_data = (const uint32_t *)m->chunk_revindex;
> +		return 0;
> +	}
> +
>  	trace2_data_string("load_midx_revindex", the_repository,
>  			   "source", "rev");

This trace2_data_string() is repeated with just "midx" v.s. "rev" parameters.

I was going to suggest that maybe we should add a "goto" here, since you're trying to juggle the early return and logging this.

But wouldn't this be both simpler and give you better logging if it was the usual region start/end pattern, with just the "data string" in-between?

Then you'd get both performance data on the index loading, and the source.

Or maybe it's overkill in this case, I don't know...