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

From

Jeff King <peff@peff.net>

Date

Mar 1, 2016, 04:40 UTC

Message-ID

<20160301044033.GA19272@sigill.intra.peff.net>

In-Reply-To

<CABaesJK+zuuYAJ6YaEugLMFywMqE8V0W1=_4mJPGDAnfT9yXJg@mail.gmail.com>

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
> <Johannes.Schindelin@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.

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