Re: [PATCH 04/31] pack-bitmap: replace sha1_to_hex

From: Jeff King <peff@peff.net>
Date: Feb 12, 2019, 06:37 UTC
Message-ID: <20190212063749.GC21872@sigill.intra.peff.net>
In-Reply-To: <20190212012256.1005924-5-sandals@crustytoothpaste.net>

On Tue, Feb 12, 2019 at 01:22:29AM +0000, brian m. carlson wrote:

> Replace the uses of sha1_to_hex in the pack bitmap code with hash_to_hex
> to allow the use of SHA-256 as well.  Rename a few variables since they
> are no longer limited to SHA-1.

Sounds good, although...

> -static uint32_t find_object_pos(const unsigned char *sha1)
> +static uint32_t find_object_pos(const unsigned char *hash)

Isn't this really just a "struct object_id"? Why don't we want to use that here?

I suspect it may be partially because our khash is storing raw sha1s. But shouldn't we also be converted that to store object_ids?

In particular:

Show 8 quoted lines

> @@ -181,7 +181,7 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,
>  	stored->root = root;
>  	stored->xor = xor_with;
>  	stored->flags = flags;
> -	oidread(&stored->oid, sha1);
> +	oidread(&stored->oid, hash);
>  
>  	hash_pos = kh_put_sha1(index->bitmaps, stored->oid.hash, &ret);

This last line (which is actually from the previous patch) is going to always truncate the stored data to 20 bytes, isn't it?

I think we need to define a kh_oid. We have the "set" version already in oidset.[ch]; I think we need to make that more public.

-Peff