Re: [PATCH v2] fetch-pack: fix object_id of exact sha1

From: Jeff King <peff@peff.net>
Date: Feb 29, 2016, 10:00 UTC
Message-ID: <20160229100030.GB2950@sigill.intra.peff.net>
In-Reply-To: <1456698144-11519-1-git-send-email-gabrielfrancosouza@gmail.com>

On Sun, Feb 28, 2016 at 07:22:24PM -0300, Gabriel Souza Franco wrote:

Show 10 quoted lines

> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.
> Add support for specifying just a SHA-1 and set the ref name to the same
> value in this case.
> 
> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
> ---
> 
> Not the cleanest conditional I've ever written, but it should handle
> all cases correctly.

I think it does. But I wonder if it wouldn't be more readable to cover the three formats independently, like:

  if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == ' ') {
	/* <sha1> <ref>, find refname */
	name += GIT_SHA1_HEXSZ + 1;
  } else if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == '\0') {
	/* <sha1>, leave sha1 as name */
  } else {
	/* <ref>, clear any cruft from get_oid_hex */
	oidclr(&ref->old_oid);
  }

And as a bonus you get rid of the separate "oid". That does call into get_oid_hex twice, but I doubt the performance impact is measurable.

We could also do:

  if (!get_oid_hex(name, &ref->old_oid)) {
	if (name[GIT_SHA1_HEXSZ] == ' ') {
		/* <sha1> <ref>, find refname */
		name += GIT_SHA1_HEXSZ + 1;
	} else if (name[GIT_SHA1_HEXSZ] == '\0') {
		/* <sha1>, leave sha1 as name */
	} else {
		/* <ref>, clear cruft from oid */
		oidclr(&ref->old_oid);
	}
  } else {
	/* <ref>, clear cruft from get_oid_hex */
	oidclr(&ref->old_oid);
  }

if you want to minimize the calls at the expense of having to repeat the oidclr().

-Peff