On Wed, May 2, 2018 at 2:11 AM, brian m. carlson
<[email protected]> wrote:
> 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.
Yeah I checked out your part14 branch after writing this note :P You
still need to rename the function though. I can remind that again when
part14 is sent 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.
I probably miss something, is hashcmp() supposed to stay after the
conversion? And will it compare any hash (as configured in the_algo)
or will it for SHA-1 only?
If hashcmp() will eventually compare the_algo->rawsz then yes this makes sense.
>> > @@ -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.
Ah ok, it's the "sha1" in the name that bugged me. I'm all good then.
--
Duy
