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