Re: [PATCH 08/41] packfile: abstract away hash constant values

> On Tue, May 01, 2018 at 12:22:43PM +0200, Duy Nguyen wrote:
>> On Mon, Apr 23, 2018 at 11:39:18PM +0000, brian m. carlson wrote:
>> > There are several instances of the constant 20 and 20-based values in
>> > the packfile code.  Abstract away dependence on SHA-1 by using the
>> > values from the_hash_algo instead.
>>
>> While we're abstracting away 20. There's the only 20 left in
>> sha1_file.c that should also be gone. But I guess you could do that
>> later since you need to rename fill_sha1_path to
>> fill_loose_object_path or something.
>
> I'm already working on knocking those out.

>> > @@ -507,15 +509,15 @@ static int open_packed_git_1(struct packed_git *p)
>> >                          " while index indicates %"PRIu32" objects",
>> >                          p->pack_name, ntohl(hdr.hdr_entries),
>> >                          p->num_objects);
>> > -   if (lseek(p->pack_fd, p->pack_size - sizeof(sha1), SEEK_SET) == -1)
>> > +   if (lseek(p->pack_fd, p->pack_size - hashsz, SEEK_SET) == -1)
>> >             return error("end of packfile %s is unavailable", p->pack_name);
>> > -   read_result = read_in_full(p->pack_fd, sha1, sizeof(sha1));
>> > +   read_result = read_in_full(p->pack_fd, hash, hashsz);
>> >     if (read_result < 0)
>> >             return error_errno("error reading from %s", p->pack_name);
>> > -   if (read_result != sizeof(sha1))
>> > +   if (read_result != hashsz)
>> >             return error("packfile %s signature is unavailable", p->pack_name);
>> > -   idx_sha1 = ((unsigned char *)p->index_data) + p->index_size - 40;
>> > -   if (hashcmp(sha1, idx_sha1))
>> > +   idx_hash = ((unsigned char *)p->index_data) + p->index_size - hashsz * 2;
>> > +   if (hashcmp(hash, idx_hash))
>>
>> Since the hash size is abstracted away, shouldn't this hashcmp become
>> oidcmp? (which still does not do the right thing, but at least it's
>> one less place to worry about)
>
> Unfortunately, I can't, because it's not an object ID.  I think the
> decision was made to not transform non-object ID hashes into struct
> object_id, which makes sense.  I suppose we could have an equivalent
> struct hash or something for those other uses.

>> > @@ -675,7 +677,8 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)
>> >     p->pack_size = st.st_size;
>> >     p->pack_local = local;
>> >     p->mtime = st.st_mtime;
>> > -   if (path_len < 40 || get_sha1_hex(path + path_len - 40, p->sha1))
>> > +   if (path_len < the_hash_algo->hexsz ||
>> > +       get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->sha1))
>>
>> get_sha1_hex looks out of place when we start going with
>> the_hash_algo. Maybe change to get_oid_hex() too.
>
> I believe this is the pack hash, which isn't an object ID.  I will
> transform it to be called something other than "sha1" and allocate more
> memory for it in a future series, though.