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

[Jeff King <peff@peff.net>]

On Wed, Nov 01, 2017 at 10:59:49PM +0100, René Scharfe wrote:

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

I meant only that most callers that parse oids, both in-file and not,
would want to stop knowing about the length ahead of time. I think
parse_oid_hex() solves that problem for most callers.

> 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 don't know. TBH, I'm not sure anyone has much interest in making
http-push work with new hashes. I'd be OK if it simply doesn't until
somebody interested shows up to change that.

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

You have to know how big the data you have is, but you don't necessarily
know whether that makes a complete hash or not. With a "remove slashes
and then parse" strategy, you can do the removing without worrying about
how big things are _supposed_ to be, and then the parser can tell you if
you have a valid oid or not. The logic for what a hash looks like _and_
how big it must be are both in the parser.

With the new code you have here, we have to be a bit more intimate with
SHA1_HEXSZ in the calling code. It knows that the hash consists of a
certain number of hex bytes.

I'm perfectly willing to punt on it for now. I'm not sure we know 100%
yet what "new"-style hashes will look like, nor how their loose-object
filenames would look.

-Peff