Re: [PATCH 3/3] sha1_file: use hex_to_bytes()

From: Jeff King <peff@peff.net>
Date: Nov 1, 2017, 19:58 UTC
Message-ID: <20171101195853.tu7wfpzo5cqoqwbo@sigill.intra.peff.net>
In-Reply-To: <d8f73744-b3f5-0fca-d58c-1f60e79214e0@web.de>

On Tue, Oct 31, 2017 at 02:50:06PM +0100, René Scharfe wrote:

Show 9 quoted lines

> The path of a loose object contains its hash value encoded into two
> substrings of 2 and 38 hexadecimal digits separated by a slash.  The
> first part is handed to for_each_file_in_obj_subdir() in decoded form as
> subdir_nr.  The current code builds a full hexadecimal representation of
> the hash in a temporary buffer, then uses get_oid_hex() to decode it.
> 
> Avoid the intermediate step by taking subdir_nr as-is and using
> hex_to_bytes() directly on the second substring.  That's shorter and
> easier.

This raises some of the same questions as the previous one on whether hex_to_bytes() is the ideal abstraction. But as before, I'm on the fence.

Show 29 quoted lines

> @@ -1908,20 +1911,15 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
>  		strbuf_setlen(path, baselen);
>  		strbuf_addf(path, "/%s", de->d_name);
>  
> -		if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2)  {
> -			char hex[GIT_MAX_HEXSZ+1];
> -			struct object_id oid;
> -
> -			xsnprintf(hex, sizeof(hex), "%02x%s",
> -				  subdir_nr, de->d_name);
> -			if (!get_oid_hex(hex, &oid)) {
> -				if (obj_cb) {
> -					r = obj_cb(&oid, path->buf, data);
> -					if (r)
> -						break;
> -				}
> -				continue;
> +		if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&
> +		    !hex_to_bytes(oid.hash + 1, de->d_name,
> +				  GIT_SHA1_RAWSZ - 1)) {
> +			if (obj_cb) {
> +				r = obj_cb(&oid, path->buf, data);
> +				if (r)
> +					break;
>  			}
> +			continue;
>  		}
>  
>  		if (cruft_cb) {

Now that this is one big conditional for "is this a valid object filename", I think we could get rid of the "continue" in favor of:

  if (...looks like an object...)
          ...call obj_cb...
  else if (cruft_cb)
          ...call cruft_cb...

Not a big deal, but it may make the flow more clear (the original had to use a continue because there were multiple independent steps to determining it was an object file, so we had to "break out" from the inner conditional).

-Peff