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

On Mon, Feb 29, 2016 at 11:08:07PM -0300, Gabriel Souza Franco wrote:

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

quoted

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.

Yes, but there are places where we get_oid_hex and then skip past that,
which could use the skip_oid_hex function, like:

diff --git a/connect.c b/connect.c
index 0478631..ba22ee6 100644
--- a/connect.c
+++ b/connect.c

@@ -149,10 +149,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			continue;
 		}
 
-		if (len < GIT_SHA1_HEXSZ + 2 || get_oid_hex(buffer, &old_oid) ||
-			buffer[GIT_SHA1_HEXSZ] != ' ')
+		if (!skip_oid_hex(buffer, &old_oid, &name) ||
+		    !skip_prefix(name, " ", &name))
 			die("protocol error: expected sha/ref, got '%s'", buffer);
-		name = buffer + GIT_SHA1_HEXSZ + 1;
 
 		name_len = strlen(name);
 		if (len != name_len + GIT_SHA1_HEXSZ + 1) {

_But_, if you look at the context just below, we make another implicit
assumption about GIT_SHA1_HEXSZ. So it's really not buying us that much
(unless we switch around the whole function to keep reading to the final
pointer, and compare "end - start" to the original "len" here).

So I'm not sure it's worth the trouble. I am really happy with the
skip_prefix() function for parsing like this, but I think it's just not
nearly as big a deal with oid-parsing, because we already have a nice
constant of GIT_SHA1_HEXSZ to match it (whereas skipping "foo" requires
us writing the magical "3" somewhere).

Anyway. Whether we want to pursue that or not, I don't think it needs to
be part of your series. Let's focus on the original goal.

-Peff