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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jan 25 2022, Taylor Blau wrote:

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

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