On Mon, Feb 29, 2016 at 5:30 AM, Johannes Schindelin <johannes.schinde...@gmx.de> 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 <p...@peff.net> wrote: > 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 <gabrielfrancoso...@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(). 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html