Re: [PATCH 12/17] revert: Save command-line options for continuing operation

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 12, 2011, 19:52 UTC
Message-ID: <20110712195246.GC14909@elie>
In-Reply-To: <1310396048-24925-13-git-send-email-artagnon@gmail.com>

Hi,

Ramkumar Ramachandra wrote:

Show 14 quoted lines

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -596,6 +596,32 @@ struct commit_list **commit_list_append(struct commit *commit,
>  	return &new->next;
>  }
>  
> +static void format_opts(struct strbuf *buf, struct replay_opts *opts)
> +{
> +	int i;
> +
> +	if (opts->no_commit)
> +		strbuf_addstr(buf, "no-commit = true\n");
> +	if (opts->edit)
> +		strbuf_addstr(buf, "edit = true\n");

[...]

Somewhat repetitive.  Would it make sense to do something like

	add_opt_bool(buf, "no-commit", opts->no_commit);
	add_opt_bool(buf, "edit", opts->edit);
	add_opt_bool(buf, "signoff", opts->signoff);
	if (opts->mainline)
		add_opt_int(buf, "mainline", opts->mainline);
	add_opt_string(buf, "strategy", opts->strategy);
	add_opt_strings(buf, "strategy-option", opts->xopts, opts->xopts_nr);

Show 9 quoted lines

> @@ -694,6 +720,184 @@ error:
>  	die(_("Malformed instruction sheet: %s"), git_path(SEQ_TODO_FILE));
>  }
>  
> +static struct strbuf *parse_value(const char *start, char **end_ptr)
> +{
> +	static struct strbuf value = STRBUF_INIT;
> +	int quote = 0;
> +	char *p = (char *)start;

Are all callers passing a modifiable buffer? If so, why not make the parameter non-const? Is it necessary to modify the buffer to parse it?

> +	char *end;
> +
> +	/* Find and strip '\n', '\r' */
> +	if ((end = strchr(start, '\n'))) {

Style:

	end = strchrnul(start, '\n');
	if (*end && end > start && end[-1] == '\r')
		end--;

	for (p = start; p != end; p++) {
		...
	}

Show 5 quoted lines

> +	for (; *p != '\0'; p ++) {
> +		if (!quote && (*p == ';' || *p == '#'))
> +			/* Ignore comments */
> +			goto ok;
> +		if (*p == '\\') {

What is this syntax?

*thinks* Oh, this looks eerily similar to config.c::parse_value(). Shouldn't the commit message say so? Why not factor out a function so they can share code?

> +			p += 1;

[...]

> +			/* Some pharapters escape as themselves */

Substitution error.

Show 9 quoted lines

> +static char *parse_opt_value(const char *start, void *key,
> +			enum parse_opt_type type, parse_opt_cb *cb_function)
> +{
> +	struct strbuf *value, *subvalue;
> +	struct option opt;
> +	char *p, *cur, *val, *end;
> +
> +	/* Remove spaces before '=' */
> +	for (p = (char *)start; isspace(*p); p++);

Same questions apply.  Stopping here.

Hope that helps, Jonathan