From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 01 Dec 2010 19:44:42 GMT
Subject: Re: [PATCH] commit: Remove backward goto in read_craft_line()
Message-ID: <20101201194441.GB27347@burratino>
In-Reply-To: <1291230959-3894-1-git-send-email-ralf.thielow@googlemail.com>

Ralf Thielow wrote:

> In case of bad graft data which is determine on many
> places we go back to the first place of detection,
> so move it to the end of the function.

I like it.  Three tiny nits.

Perhaps the log message could be clarified like so:

 bad graft data is noticed in several places in read_graft_line and
 in each case we go back to the first site of detection.  Move the
 error handling to the end of the function for better readability.

> --- a/commit.c
> +++ b/commit.c
> @@ -137,12 +137,8 @@ struct commit_graft *read_graft_line(char *buf, int len)
>  		buf[--len] = '\0';
>  	if (buf[0] == '#' || buf[0] == '\0')
>  		return NULL;
> -	if ((len + 1) % 41) {
> -	bad_graft_data:
> -		error("bad graft data: %s", buf);
> -		free(graft);
> -		return NULL;
> -	}
> +	if ((len + 1) % 41) 
> +		goto bad_graft_data;

Trailing whitespace.  (You can avoid this in the future with
"git diff --check".)

[...]
> @@ -155,6 +151,11 @@ struct commit_graft *read_graft_line(char *buf, int len)
>  			goto bad_graft_data;
>  	}
>  	return graft;
> +
> +bad_graft_data:
> +	error("bad graft data: %s", buf);

A space before the "bad_graft_data:" label would improve future
diff --show-c-function output.

Except as noted above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.
---
diff --git a/commit.c b/commit.c
index d86159a..d5144f6 100644
--- a/commit.c
+++ b/commit.c
@@ -137,7 +137,7 @@ struct commit_graft *read_graft_line(char *buf, int len)
 		buf[--len] = '\0';
 	if (buf[0] == '#' || buf[0] == '\0')
 		return NULL;
-	if ((len + 1) % 41) 
+	if ((len + 1) % 41)
 		goto bad_graft_data;
 	i = (len + 1) / 41 - 1;
 	graft = xmalloc(sizeof(*graft) + 20 * i);
@@ -152,7 +152,7 @@ struct commit_graft *read_graft_line(char *buf, int len)
 	}
 	return graft;
 
-bad_graft_data:
+ bad_graft_data:
 	error("bad graft data: %s", buf);
 	free(graft);
 	return NULL;