Re: [PATCH 14/16] fetch-pack: refactor parsing in get_ack

2014-06-19 Thread Eric Sunshine
On Wed, Jun 18, 2014 at 3:56 PM, Jeff King p...@peff.net wrote:
 There are several uses of the magic number line+45 when
 parsing ACK lines from the server, and it's rather unclear
 why 45 is the correct number. We can make this more clear by
 keeping a running pointer as we parse, using skip_prefix to
 jump past the first ACK , then adding 40 to jump past
 get_sha1_hex (which is still magical, but hopefully 40 is
 less magical to readers of git code).

 Note that this actually puts us at line+44. The original
 required some character between the sha1 and further ACK
 flags (it is supposed to be a space, but we never enforced
 that). We start our search for flags at line+44, which
 meanas we are slightly more liberal than the old code.

s/meanas/means/

 Signed-off-by: Jeff King p...@peff.net
--
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


[PATCH 14/16] fetch-pack: refactor parsing in get_ack

2014-06-18 Thread Jeff King
There are several uses of the magic number line+45 when
parsing ACK lines from the server, and it's rather unclear
why 45 is the correct number. We can make this more clear by
keeping a running pointer as we parse, using skip_prefix to
jump past the first ACK , then adding 40 to jump past
get_sha1_hex (which is still magical, but hopefully 40 is
less magical to readers of git code).

Note that this actually puts us at line+44. The original
required some character between the sha1 and further ACK
flags (it is supposed to be a space, but we never enforced
that). We start our search for flags at line+44, which
meanas we are slightly more liberal than the old code.

Signed-off-by: Jeff King p...@peff.net
---
I actually think we could tighten this even more and drop the strstrs,
too, like:

  arg += 40;
  if (*arg++ != ' ')
return ACK;
  if (!strcmp(arg, continue))
return ACK_continue;

and so on. But I wasn't sure if there was a reason for the use of
strstr. According to pack-protocol.txt, we would only get one at a time,
and always with a single space between them.

 fetch-pack.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 3de3bd5..72ec520 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -189,20 +189,23 @@ static enum ack_type get_ack(int fd, unsigned char 
*result_sha1)
 {
int len;
char *line = packet_read_line(fd, len);
+   const char *arg;
 
if (!len)
die(git fetch-pack: expected ACK/NAK, got EOF);
if (!strcmp(line, NAK))
return NAK;
-   if (starts_with(line, ACK )) {
-   if (!get_sha1_hex(line+4, result_sha1)) {
-   if (len  45)
+   if (skip_prefix(line, ACK , arg)) {
+   if (!get_sha1_hex(arg, result_sha1)) {
+   arg += 40;
+   len -= arg - line;
+   if (len  1)
return ACK;
-   if (strstr(line+45, continue))
+   if (strstr(arg, continue))
return ACK_continue;
-   if (strstr(line+45, common))
+   if (strstr(arg, common))
return ACK_common;
-   if (strstr(line+45, ready))
+   if (strstr(arg, ready))
return ACK_ready;
return ACK;
}
-- 
2.0.0.566.gfe3e6b2

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