Re: [PATCH v2 1/8] packfile: prepare for the existence of '*.rev' files

From: Jeff King <peff@peff.net>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, dstolee@microsoft.com, gitster@pobox.com,
	jrnieder@gmail.com
Subject: Re: [PATCH v2 1/8] packfile: prepare for the existence of '*.rev' files
Date: Mon, 25 Jan 2021 13:27:00 -0500	[thread overview]
Message-ID: <YA8NdHJloTf15qXf@coredump.intra.peff.net> (raw)
In-Reply-To: <YA8DlQwSzNZXq+AU@nand.local>

On Mon, Jan 25, 2021 at 12:44:53PM -0500, Taylor Blau wrote:

> Thanks for including a compelling argument in favor of the approach that
> I took in this patch.
> 
> I added something along the lines of what you suggested to the final
> paragraph, so now it concludes nicely instead of ending in a comma. I
> briefly considered whether I should add something about how these
> operations scale and how the warming efforts are really amortized across
> all of the objects, but I decided against it.
> 
> I think that this argument is already documented here, and that there's
> no way to concisely state it in an already long patch. Interested
> readers will easily be able to find our discussion here, which is good.

That sounds good. It is sort of arguing against a strawman anyway.

> > It mostly looks good to me. I agree with Junio that "compute" is a
> > better verb than "load" for generating the in-memory revindex.
> 
> Yeah, I settled on load_pack_revindex() either calling
> "create_pack_revindex_in_memory()" or "load_pack_revindex_from_disk()".

Perfect.

> > If p->revindex_data were "const uint32_t *", then this line would just
> > be:
> >
> >   return get_be32(p->revindex_data + pos);
> >
> > Not a huge deal either way since the whole point is to abstract this
> > behind a function where it only has to be written once. I don't think
> > there is any downside from the compiler's view (and we already use this
> > trick for the bitmap name-hash cache).
> 
> Honestly, I'm not a huge fan of implicitly scaling pos by
> sizeof(*p->revindex_data), but I can understand why it reads more
> clearly here. I don't really feel strongly either way, so I'm happy to
> change it in favor of your suggestion.
> 
> Of course, since RIDX_HEADER_SIZE is in bytes, not uint32_t's (and it
> has to be, since it's also used in the RIDX_MIN_SIZE macro, which is
> compared against the st_size of stating the .rev file), you have to do
> gross stuff like:
> 
>   p->revindex_data = (const uint32_t *)((const char *)p->revindex_map + RIDX_HEADER_SIZE);
> 
> But I guess the tradeoff is worth it, since the readers are easier to
> parse.

Yeah, that is definitely a downside. Perhaps keeping everything in bytes
makes things a bit more obvious. In which case I might suggest that
revindex_data just be a "const char *". You'd have to scale any pointer
computations at the point of use then, but you'd avoid needing to do any
extra casting.

-Peff