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.
> Same comment for other hashcmp in this patch.
>
> > @@ -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.
> > @@ -1678,10 +1683,10 @@ int bsearch_pack(const struct object_id *oid, const struct packed_git *p, uint32
> >
> > index_lookup = index_fanout + 4 * 256;
> > if (p->index_version == 1) {
> > - index_lookup_width = 24;
> > + index_lookup_width = hashsz + 4;
>
> You did good research to spot this 24 constant ;-)
Thanks.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Attachment:
signature.asc
Description: PGP signature
