Re: [RFC PATCH v6 3/3] git-remote-ext

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 8, 2010, 22:02 UTC
Message-ID: <20101008220220.GA15214@burratino>
In-Reply-To: <1286571713-7755-4-git-send-email-ilari.liusvaara@elisanet.fi>

Ilari Liusvaara wrote:

> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>

Just a few random nits.

> +++ b/builtin/remote-ext.c
> @@ -0,0 +1,243 @@

[...] [...]

> +			default:
> +				die("Bad remote-ext placeholder '\\%c'.",
> +					str[rpos]);

Should be "Bad remote-ext placeholder '%%%c'.", I think.

Show 6 quoted lines

> +static const char **parse_argv(const char *arg, const char *service)
> +{
> +	int arguments = 0;
> +	int i;
> +	char **ret;
> +	char *(temparray[MAXARGUMENTS + 1]);

Would be more idiomatic to leave out the parentheses.

> +
> +	while (*arg) {
> +		char *ret;

Would be clearer without shadowing.  Maybe

		char *expanded;

[...]

Show 5 quoted lines

> +	ret = xcalloc(arguments + 1, sizeof(char *));
> +	for (i = 0; i < arguments; i++)
> +		ret[i] = temparray[i];
> +
> +	return (const char **)ret;

Maybe

	ret = xmalloc(...
	for (...

	ret[arguments] = NULL;
	return ret;

(to avoid clearing memory that is about to be overwritten) or even

	ret = xmalloc(...
	memcpy(ret, temparray, arguments * sizeof(*ret));
	ret[arguments] = NULL;
	return ret;

(to emphasize the copy) would make sense?

Why is ret of type char ** and then cast away rather than being const char ** in the first place?

Show 7 quoted lines

> +static int command_loop(const char *child)
> +{
> +	char buffer[MAXCOMMAND];
> +
> +	while (1) {
> +		if (!fgets(buffer, MAXCOMMAND - 1, stdin))
> +			exit(0);

Won't this exit(0) for I/O errors?

> +		/* Strip end of line characters. */
> +		while (isspace((unsigned char)buffer[strlen(buffer) - 1]))
> +			buffer[strlen(buffer) - 1] = 0;

This is wasteful: strlen() has to look for the null byte three times just to strip a \r\n from the end. See remote-fd for an example of how to avoid that overhead.