Re: [PATCH] commit: Remove backward goto in read_craft_line()

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 1, 2010, 19:44 UTC
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.

Show 14 quoted lines

> --- 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".)

[...]

Show 7 quoted lines

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