Re: [PATCH v2] Allow commit (and tag) messages to be edited when $EDITOR has arguments

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 16, 2007, 15:36 UTC
Message-ID: <Pine.LNX.4.64.0712161525090.27959@racer.site>
In-Reply-To: <20071216073408.GA5343@midwinter.com>

Hi,

On Sat, 15 Dec 2007, Steven Grimm wrote:

Show 9 quoted lines

> @@ -238,7 +186,7 @@ struct cmd_struct {
>  	int option;
>  };
>  
> -static int run_command(struct cmd_struct *p, int argc, const char **argv)
> +static int run_git_command(struct cmd_struct *p, int argc, const char **argv)
>  {
>  	int status;
>  	struct stat st;

Funny ;-) I have a similar change in my (now-inactive until 1.5.4) tree, but I renamed it to execv_git_builtin (because it is not supposed to return in case of success).

Show 14 quoted lines

> diff --git a/run-command.c b/run-command.c
> index 476d00c..3ae55ec 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -237,3 +237,68 @@ int finish_async(struct async *async)
>  		ret = error("waitpid (async) failed");
>  	return ret;
>  }
> +
> +/*
> + * Parses a command line into an array of char* representing the tokens
> + * on the command line.  Pass in a count to reserve some number of additional
> + * slots in the allocated array, e.g., so the caller can add a filename
> + * argument without having to reallocate the array.

In the editor call, you said "2" for this, but I suspect that you counted the NULL extra. However, in git.c you said "0", thus not counting the NULL. IMHO it makes sense _not_ to count the NULL (and NULL-terminate the list _always_).

Show 8 quoted lines

> +int split_cmdline(char *cmdline, const char ***argv, int extra_slots)
> +{
> +	int src, dst, count = 0, size = extra_slots + 16;
> +	char quoted = 0;
> +
> +	*argv = xmalloc(sizeof(char*) * size);
> +
> +	/* split alias_string */

s/alias_string/the command line/

Show 9 quoted lines

> +	(*argv)[count++] = cmdline;
> +	for (src = dst = 0; cmdline[src];) {
> +		char c = cmdline[src];
> +		if (!quoted && isspace(c)) {
> +			cmdline[dst++] = 0;
> +			while (cmdline[++src]
> +					&& isspace(cmdline[src]))
> +				; /* skip */
> +			if (count >= size) {

s/count/count + extra_slots/

> +				size += 16;
> +				*argv = xrealloc(*argv, sizeof(char*) * size);

This seems a nice candidate for ALLOC_GROW() in any case:

			ALLOC_GROW(*argv, count + extra_slots + 1, size);

Show 30 quoted lines

> +			}
> +			(*argv)[count++] = cmdline + dst;
> +		} else if(!quoted && (c == '\'' || c == '"')) {
> +			quoted = c;
> +			src++;
> +		} else if (c == quoted) {
> +			quoted = 0;
> +			src++;
> +		} else {
> +			if (c == '\\' && quoted != '\'') {
> +				src++;
> +				c = cmdline[src];
> +				if (!c) {
> +					free(*argv);
> +					*argv = NULL;
> +					return error("cmdline ends with \\");
> +				}
> +			}
> +			cmdline[dst++] = c;
> +			src++;
> +		}
> +	}
> +
> +	cmdline[dst] = 0;
> +
> +	if (quoted) {
> +		free(*argv);
> +		*argv = NULL;
> +		return error("unclosed quote");
> +	}

AFAICT the argv were not NULL terminated before. But now that the function is public, it is safer to do so:

	(*argv)[count] = NULL;

> +
> +	return count;
> +}
> +

Thanks, Dscho