Re: [PATCH 1/2] add git-quote: shell and C quoting tool

From: Junio C Hamano <junkio@cox.net>
Date: Jul 24, 2006, 05:49 UTC
Message-ID: <7vwta3h7r2.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20060716222757.GA10135@steel.home>

fork0@t-online.de (Alex Riesen) writes:

> In case anyone asks why isn't it a standalone tool nor is it put into
> git-stripspace: I don't know. Maybe it should be.

This organization is fine by me.

 * unquote?
 * doc?

> +enum {SHELL_QUOTE, C_QUOTE};
> +static int style = SHELL_QUOTE,
> +	   use_stdin = 0;

Shouldn't style be of type which is that unnamed enum?

> +static const char *separator = NULL; /* default is space */
> +static unsigned sep_len = 0;

It is not clear if this is separator of input side or output side (after reading the code it becomes somewhat obvious that you are talking about output separator), and if the default is space wouldn't it make more sense to set that here?

Show 14 quoted lines

> +static const char builtin_quote_usage[] =
> +"git-quote [--c] [--sep=<c-quoted> | -z] ( [--stdin] | [--] ... )";
> +
> +static void print_quoted(const char *text)
> +{
> +	switch (style)
> +	{
> +	case SHELL_QUOTE:
> +		sq_quote_print(stdout, text);
> +		break;
> +	case C_QUOTE:
> +		quote_c_style(text, NULL, stdout, 0);
> +		break;
> +	}

Not a big deal but aren't these going to write out things quoted when they do not need to? To help scripts, it might be worth adding "check if this needs quoting" interface perhaps?

Show 21 quoted lines

> +int cmd_quote(int argc, const char **argv, char **envp)
> +{
> +	int i;
> +	for (i = 1; i < argc; i++) {
> +		const char *arg = argv[i];
> +
> +		if (arg[0] != '-')
> +			break;
> +		if (!strcmp(arg, "--")) {
> +			i++;
> +			break;
> +		}
> +		if (!strcmp(arg, "--stdin")) {
> +			use_stdin = 1;
> +			if ( !separator ) {
> +				separator = "\n";
> +				sep_len = 1;
> +			}
> +			break;
> +		}
> +		if (!strcmp(arg, "--c")) {

Perhaps "--c-style" with "-c" as a shorthand, (and "--shell-style" with "-s" to complement)?

Show 8 quoted lines

> +			style = C_QUOTE;
> +			continue;
> +		}
> +		if (!strcmp(arg, "-z")) {
> +			separator = "";
> +			sep_len = 1;
> +			continue;
> +		}

Is it plausible that somebody might want to feed you NUL terminated sequence as input (iow you might want to give them choice of separator on the input side)?

> +		if (!strncmp(arg, "--sep=", 6)) {

Perhaps "--separator" (or "--output-separator") with whatever shorthand is handy?

Show 12 quoted lines

> +			const char *end;
> +			char *tmp;
> +			arg += 6;
> +			if ('"' == *arg)
> +				tmp = strdup(arg);
> +			else {
> +				size_t l = strlen(arg);
> +				tmp = malloc(l + 3);
> +				sprintf(tmp, "\"%s\"", arg);
> +			}
> +			separator = unquote_c_style(tmp, &end);
> +			sep_len = strlen(separator);

If the parameter is a malformed c-quoted string, you would dump core here.

Show 8 quoted lines

> +			/* this will leak if multiple --sep= given */
> +			continue;
> +		}
> +		die(builtin_quote_usage);
> +	}
> +	if (!separator) {
> +		sep_len = 1;
> +		separator = "\x20";

Any reason you needed to spell it in hex? Is this trying to be portable to EBCDIC (or trying to prevent the code to be ported there)? I dunno.

Show 15 quoted lines

> +	}
> +	if (use_stdin) {
> +		size_t size = BUFSIZ;
> +		char *buf = xmalloc(size);
> +		int ch, pos = 0;
> +		while (EOF != (ch = fgetc(stdin))) {
> +			if (pos == size)
> +				buf = xrealloc(buf, size <<= 1);
> +			buf[pos++] = ch;
> +			if ('\n' == ch) {
> +				buf[--pos] = '\0';
> +				pos = 0;
> +				print_quoted(buf);
> +			}
> +		}

Looks similar to strbuf.c::read_line() loop...