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

From: Jeff King <peff@peff.net>
Date: Nov 1, 2017, 22:15 UTC
Message-ID: <20171101221541.6dwezd4t4hweuis2@sigill.intra.peff.net>
In-Reply-To: <0e7f821a-94ae-3946-fb99-c508a06a6847@web.de>

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

Show 7 quoted lines

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

Show 11 quoted lines

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