Re: [PATCH 2/7] reftable: explicitly handle hash format IDs
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Nov 7, 2024, 23:11 UTC
- Message-ID
- <yvdvlf45g4ahqa3uey4zj4evissv2tfkwiy5o3e3vbqtwmc7o4@utqagwvv6yhe>
- In-Reply-To
- <c1bd8e2b3c4c524a21f68e822812e41179827e4d.1729677003.git.ps@pks.im>
On 24/10/23 11:55AM, Patrick Steinhardt wrote:
Show 14 quoted lines
> diff --git a/reftable/basics.h b/reftable/basics.h > index 7aa46d7c30d..86141602e74 100644 > --- a/reftable/basics.h > +++ b/reftable/basics.h > @@ -150,4 +150,11 @@ int common_prefix_size(struct reftable_buf *a, struct reftable_buf *b); > > int hash_size(uint32_t id); > > +/* > + * Format IDs that identify the hash function used by a reftable. Note that > + * these constants end up on disk and thus mustn't change. > + */ > +#define REFTABLE_FORMAT_ID_SHA1 ((uint32_t) 0x73686131) > +#define REFTABLE_FORMAT_ID_SHA256 ((uint32_t) 0x73323536)
This context is provided in the comments for `GIT_*_FORMAT_ID`, but now that they being decoupled it might be nice to mention here that they stand for "sha1" and "s256" respectively.
Show 25 quoted lines
> +
> #endif
> diff --git a/reftable/reader.c b/reftable/reader.c
> index 90dc950b577..64eb6938efe 100644
> --- a/reftable/reader.c
> +++ b/reftable/reader.c
> @@ -109,16 +109,18 @@ static int parse_footer(struct reftable_reader *r, uint8_t *footer,
> if (r->version == 1) {
> r->hash_id = GIT_SHA1_FORMAT_ID;
> } else {
> - r->hash_id = get_be32(f);
> - switch (r->hash_id) {
> - case GIT_SHA1_FORMAT_ID:
> + switch (get_be32(f)) {
> + case REFTABLE_FORMAT_ID_SHA1:
> + r->hash_id = GIT_SHA1_FORMAT_ID;
> break;
> - case GIT_SHA256_FORMAT_ID:
> + case REFTABLE_FORMAT_ID_SHA256:
> + r->hash_id = GIT_SHA256_FORMAT_ID;
> break;
> default:
> err = REFTABLE_FORMAT_ERROR;
> goto done;
> }Instead of directly mapping `hash_id` to `GIT_*_FORMAT_ID`, we use the newly defined constants to map to the corresponding format. As an internal identifier, the format ID can now be freely changed without risk of affecting how on-disk reftable headers are parsed. Makes sense to me and seems like a good change.
-Justin