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

From: René Scharfe <l.s.r@web.de>
Date: Nov 1, 2017, 21:59 UTC
Message-ID: <0e7f821a-94ae-3946-fb99-c508a06a6847@web.de>
In-Reply-To: <20171101195530.odscytks7jvgukbs@sigill.intra.peff.net>

Am 01.11.2017 um 20:55 schrieb Jeff King:

Show 19 quoted lines

> On Tue, Oct 31, 2017 at 02:49:56PM +0100, René Scharfe wrote:
> 
>> 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'm confused by the word "many". After this series there are three callers of hex_to_bytes() and I don't expect that number to grow.

Would loose objects be stored at paths containing only a subset of their new hash value? If they won't then there will be two acceptable lengths instead of the one we have today, which should be easy to handle.

> 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".

Hmm, ignoring any slashes is an interesting idea. Is that too loose, I wonder? And I don't think it makes for a good primitive, as slashes only need to be ignored in a single place so far (here in http-push.c). Collecting special cases in a central place, guarded by flags, doesn't reduce the overall complexity.

> 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.

How so? If you have a buffer then you need to know the size of the data to copy into it as well, or you'll learn it in the process.

The call sites of hex_to_bytes() have to be modified along with the functions in hex.c to support longer hashes, with or without this series.

René