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

> 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)?

> On Sun, Feb 28, 2016 at 07:22:24PM -0300, Gabriel Souza Franco wrote:
>
>> 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().