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

On Mon, Feb 29, 2016 at 5:30 AM, Johannes Schindelin
[off-list ref] wrote:

Having said that, this *might* be a good opportunity to imitate the
skip_prefix() function. If there are enough similar code constructs, we
could simplify all of them by introducing the function

        skip_oid_hex(const char *str, struct object_id *oid, const char **out)

that returns 1 if and only if an oid was parsed, and stores the pointer
after the oid in "out" (skipping an additional space if there is one)?

I don't think there's any other place that accepts all of "<sha1>",
"<sha1> <ref>" and "<ref>"
based on a quick grep for get_oid_hex.

On Mon, Feb 29, 2016 at 7:00 AM, Jeff King [off-list ref] wrote:

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

quoted

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 <redacted>
---

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().

I think I like this version more, and is close to what I had initially
before I tried to be clever about it.
Besides, this isn't a performance critical function, so it shouldn't
matter much.
Will send a new (and hopefully final) version shortly.

-Peff