On Wed, Jan 13, 2021 at 11:22:53PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> 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
