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

From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: Duy Nguyen <pclouds@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH 08/41] packfile: abstract away hash constant values
Date: Wed, 2 May 2018 00:11:41 +0000	[thread overview]
Message-ID: <20180502001140.GH13217@genre.crustytoothpaste.net> (raw)
In-Reply-To: <20180501102243.GE15820@duynguyen.home>

[-- Attachment #1: Type: text/plain, Size: 3245 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]