Re: [PATCH 2/3] http-push: use hex_to_bytes()

From: Jeff King <peff@peff.net>
Date: Nov 1, 2017, 19:55 UTC
Message-ID: <20171101195530.odscytks7jvgukbs@sigill.intra.peff.net>
In-Reply-To: <31ce5c1c-cab3-80aa-623b-43f91ddd2162@web.de>

On Tue, Oct 31, 2017 at 02:49:56PM +0100, René Scharfe wrote:

Show 9 quoted lines

> The path of a loose object contains its hash value encoded into two
> substrings of hexadecimal digits, separated by a slash.  The current
> code copies the pieces into a temporary buffer to get rid of the slash
> and then uses get_oid_hex() to decode the hash value.
> 
> Avoid the copy by using hex_to_bytes() directly on the substrings.
> That's shorter and easier.
> 
> While at it correct the length of the second substring in a comment.

I think the patch is correct, but I wonder if this approach will bite us later on when we start dealing with multiple lengths of hashes.

The hex_to_bytes() function requires that the caller make sure they have the right number of bytes. But for many callers, I think they'd want to say "parse this oid, which might be truncated; I can't tell what the length is supposed to be".

I.e., I wonder if the right primitive is really something like parse_oid_hex(), but with a flag to say "skip over interior slashes".

We don't need to deal with that eventuality yet, but I'm on the fence on whether this patch is making that harder down the road or not. The current strategy of "stuff it into a buffer without slashes" would be easier to convert, I think.

-Peff