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

From: Taylor Blau <me@ttaylorr.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, dstolee@microsoft.com, jrnieder@gmail.com,
	peff@peff.net
Subject: Re: [PATCH v2 1/8] packfile: prepare for the existence of '*.rev' files
Date: Thu, 14 Jan 2021 13:28:47 -0500	[thread overview]
Message-ID: <YACNX+ExF1yAn4g3@nand.local> (raw)
In-Reply-To: <xmqqwnwgx9rm.fsf@gitster.c.googlers.com>

On Wed, Jan 13, 2021 at 11:22:53PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > +== pack-*.rev files have the format:
> > +
> > +  - A 4-byte magic number '0x52494458' ('RIDX').
> > +
> > +  - A 4-byte version identifier (= 1)
> > +
> > +  - A 4-byte hash function identifier (= 1 for SHA-1, 2 for SHA-256)
>
> These two are presumably 4-byte-wide network byte order integers.
> We should spell it out.

Yep, all entries are network order. I added a sentence at the bottom of
this sub-section to say as much.

> We've seen hardcoded constant "12" twice so far in this patch.
>
> We need a C proprocessor macro "#define RIDX_FILE_HEADER_SIZE 12" or
> something, perhaps?

Good idea, thanks.

> > -	return p->revindex[pos].nr;
> > +
> > +	if (p->revindex)
> > +		return p->revindex[pos].nr;
> > +	else
> > +		return get_be32((char *)p->revindex_data + (pos * sizeof(uint32_t)));
>
> Good.  We are using 32-bit uint in network byte order.  We should
> document it as such.
>
> Let's not strip const away while casting, though.  get_be32()
> ensures that it only reads and never writes thru the pointer, and
> p->revindex_data is a "const void *".

Agreed, and thanks for the suggestion. I take it that what you mean is:

-		return get_be32((char *)p->revindex_data + (pos * sizeof(uint32_t)));
+		return get_be32((const char *)p->revindex_data + (pos * sizeof(uint32_t)));

...yes?

> > diff --git a/pack-revindex.h b/pack-revindex.h
> > index 6e0320b08b..01622cf21a 100644
> > --- a/pack-revindex.h
> > +++ b/pack-revindex.h
> > @@ -21,6 +21,9 @@ struct packed_git;
> >  /*
> >   * load_pack_revindex populates the revindex's internal data-structures for the
> >   * given pack, returning zero on success and a negative value otherwise.
> > + *
> > + * If a '.rev' file is present, it is checked for consistency, mmap'd, and
> > + * pointers are assigned into it (instead of using the in-memory variant).
>
> Hmph, I missed where it got checked for consistency, though.  If the
> file is corrupt and has say duplicated entries, we'd happily grab
> the data via get_be32(), for example.

It doesn't, I'm mistaken. I removed that incorrect detail from this
comment. Thanks for catching it.

Thanks,
Taylor