On Thu, Apr 27, 2017 at 01:52:09AM -0400, Jeff King wrote:

On Sun, Apr 23, 2017 at 09:34:35PM +0000, brian m. carlson wrote:

quoted

Use a
struct object_id to hold the pack checksum, even though it is not
strictly an object ID.  Doing so ensures resilience against future hash
size changes, and allows us to remove hard-coded assumptions about how
big the buffer needs to be.

But this part seems questionable to me. Sure, we may change the pack
checksum in the future. But there is a reasonable chance that it won't
follow the same rules for selecting a hash as object_id. And even if it
does, calling it object_id just seems misleading.

What's the gain in converting it here? I know we want to get rid of the
bare "20", but we could switch it out for GIT_SHA1_RAWSZ. I suspect you
prefer in the long run to get rid of even those GIT_SHA1_RAWSZ defines,
though.  Could we define a new struct csumfile_hash, csumfile_cmp, etc
(and arguably change the name of "struct sha1file" and friends).  They'd
be boring wrappers around sha1 now, but I think the endgame will involve
us being able to read multiple versions of packs, with distinct
checksum algorithms.

When I wrote this originally, the GIT_MAX_*SZ patch was in
object-id-part9 and hadn't been merged yet.  And I think your concerns
about this being kinda gross are legitimate.  I'll admit I had some
hesitance about it at first.

So I'll reroll this leaving it as an unsigned char with GIT_MAX_RAWSZ.
I feel confident that we're not going to pick a third, different
algorithm for the pack checksum, so that will get us to the point that
we have a big enough buffer, and we can incrementally improve on it
later by using a different struct if we like.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204