Re: [PATCH 4/5] tree-walk: store object_id in a separate member

From: Jeff King <peff@peff.net>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: git@vger.kernel.org, "Martin Ågren" <martin.agren@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH 4/5] tree-walk: store object_id in a separate member
Date: Thu, 10 Jan 2019 01:49:45 -0500	[thread overview]
Message-ID: <20190110064944.GA6810@sigill.intra.peff.net> (raw)
In-Reply-To: <20190110042551.915769-5-sandals@crustytoothpaste.net>

On Thu, Jan 10, 2019 at 04:25:50AM +0000, brian m. carlson wrote:

> diff --git a/tree-walk.c b/tree-walk.c
> index 1e040fc20e..b6daeab16d 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -48,7 +48,8 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l
>  	/* Initialize the descriptor entry */
>  	desc->entry.path = path;
>  	desc->entry.mode = canon_mode(mode);
> -	desc->entry.oid  = (const struct object_id *)(path + len);
> +	desc->entry.pathlen = len - 1;
> +	memcpy(desc->entry.oid.hash, path + len, the_hash_algo->rawsz);
>  
>  	return 0;
>  }

This one really is a hashcpy() now, right, even after your final patch?
I guess using rawsz explicitly makes it match the computation here:

> @@ -107,7 +108,7 @@ static void entry_extract(struct tree_desc *t, struct name_entry *a)
>  static int update_tree_entry_internal(struct tree_desc *desc, struct strbuf *err)
>  {
>  	const void *buf = desc->buffer;
> -	const unsigned char *end = desc->entry.oid->hash + the_hash_algo->rawsz;
> +	const unsigned char *end = (const unsigned char *)desc->entry.path + desc->entry.pathlen + 1 + the_hash_algo->rawsz;
>  	unsigned long size = desc->size;
>  	unsigned long len = end - (const unsigned char *)buf;

So maybe it's better to be explicit as you have here. (Mostly just as I
was reading it, I was looking for a use of hashcpy and was surprised not
to find it ;) ).

> @@ -175,9 +176,11 @@ void setup_traverse_info(struct traverse_info *info, const char *base)
>  		pathlen--;
>  	info->pathlen = pathlen ? pathlen + 1 : 0;
>  	info->name.path = base;
> -	info->name.oid = (void *)(base + pathlen + 1);
> -	if (pathlen)
> +	info->name.pathlen = pathlen;
> +	if (pathlen) {
> +		memcpy(info->name.oid.hash, (base + pathlen + 1), the_hash_algo->rawsz);
>  		info->prev = &dummy;
> +	}
>  }
>  

Ditto here, I think.

-Peff