Re: [PATCH 11/18] revert: Save data for continuing after conflict resolution

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -269,7 +275,7 @@ static void write_message(struct strbuf *msgbuf, const char *filename)
>  		die_errno(_("Could not write to %s."), filename);
>  	strbuf_release(msgbuf);
>  	if (commit_lock_file(&msg_file) < 0)
> -		die(_("Error wrapping up %s"), filename);
> +		die(_("Error wrapping up %s."), filename);
>  }
>  
>  static struct tree *empty_tree(void)

Snuck in to the wrong patch, I guess?

[...]
> @@ -582,10 +588,199 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
[...]
> +	if (!prefixcmp(start, "pick ")) {
> +		action = CHERRY_PICK;
> +		insn_len = strlen("pick");
> +		p = start + insn_len + 1;
> +	}
> +	else if (!prefixcmp(start, "revert ")) {
> +		action = REVERT;
> +		insn_len = strlen("revert");
> +		p = start + insn_len + 1;
> +	}
> +	else
> +		return NULL;
> +

Style: git uses "cuddled braces".

	if (...) {
		...
	} else if (...) {
		...
	} ...

[...]
> +	if ((get_sha1(sha1_abbrev, commit_sha1) < 0)
> +		|| !(commit = lookup_commit_reference(commit_sha1)))
> +		return NULL;
> +
> +	return commit;

Would be clearer to write:

	if (get_sha1(...))
		return NULL;

	return lookup_commit_reference(...);

[...]
> @@ -59,6 +60,11 @@ struct replay_opts {
>  };
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
> +#define MAYBE_UNUSED __attribute__((__unused__))
[...]
> +}
> +
> +static void MAYBE_UNUSED read_populate_todo(struct commit_list **todo_list,
> +					struct replay_opts *opts)

This MAYBE_UNUSED with no conditional involved feels ugly.  Why is it
needed?  Why not define read_populate_todo in the patch that first
uses it, or barring that, tolerate a warning in this step within the
patch series?

[...]
> +		if (!(commit = parse_insn_line(p, opts)))
> +			goto error;

See Documentation/CodingGuidelines: "We try to avoid assignments
inside if()".

> +static void walk_revs_populate_todo(struct commit_list **todo_list,
> +				struct replay_opts *opts)
>  {
>  	struct rev_info revs;
>  	struct commit *commit;
> +	struct commit_list **next;
> +
> +	prepare_revs(&revs, opts);
> +
> +	next = todo_list;
> +	while ((commit = get_revision(&revs)))
> +		next = commit_list_append(commit, next);
> +}
> +
> +static void create_seq_dir(void)
> +{
> +	const char *seq_dir = git_path(SEQ_DIR);
> +
> +	if (!file_exists(seq_dir) && mkdir(seq_dir, 0777) < 0)
> +		die_errno(_("Could not create sequencer directory '%s'."), seq_dir);

What happens if seq_dir exists and is a file?

[...]
> @@ -593,14 +788,29 @@ static int pick_commits(struct replay_opts *opts)
[...]
> +	if (get_sha1("HEAD", sha1)) {
> +		if (opts->action == REVERT)
> +			return error(_("Can't revert as initial commit"));
> +		return error(_("Can't cherry-pick into empty head"));
> +	} else
> +		save_head(sha1_to_hex(sha1));

Clearer without the "else":

	if (get_sha1(...)) {
		...
		return error(...);
	}
	save_head(sha1);

> +	save_todo(todo_list, opts);
> +
> +	for (cur = todo_list; cur; cur = cur->next) {
> +		save_todo(cur, opts);
> +		res = do_pick_commit(cur->item, opts);
>  		if (res)
>  			return res;
>  	}

Isn't the save_todo() before the "for" loop redundant?
>  
> +	/*
> +	 * Sequence of picks finished successfully; cleanup by
> +	 * removing the .git/sequencer directory
> +	 */
> +	strbuf_addf(&buf, "%s", git_path(SEQ_DIR));
> +	remove_dir_recursively(&buf, 0);

What is the content of "buf" before?  If it's supposed to be empty,
I'd suggest using "strbuf_reset" to make sure that remains so for
the reader's peace of mind.

Ciao,
Jonathan