Re: [RFC PATCH 13/13] revert: Introduce --continue to continue the operation

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> After resolving a conflict, the user has the option to continue the
> operation.

When reading this, I wonder: "does the user have that option or will
she gain that option after this patch?  And if the latter, what does
the UI look like?  And what is the purpose --- if I just cherry-picked
a commit and ran into a conflict, won't 'git commit' be good enough?"

> Mixing operations (cherry-pick and revert in the same
> todo_list) and command-line options are unsupported at this stage.

It's not obvious what this means.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -614,6 +652,78 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
>  	}
>  }
>  
> +static void read_populate_todo(struct commit_list **todo_list,
> +			struct replay_opts *opts)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	enum replay_action action;
> +	struct commit *commit;
> +	struct commit_list *new_item;
> +	struct commit_list *cur = NULL;
> +	unsigned char commit_sha1[20];
> +	char sha1_abbrev[40];
> +	char *p;
> +	int insn_len = 0;
> +	char *insn;
> +	int fd;

The naive reader is not sure what this function is about and is
intimidated by its size.  (I know you have plans regarding that
already; just mentioning it so it doesn't get forgotten.)

> +		/* Verify that the action matches up with the one in
> +		   opts; we don't support arbitrary instructions */

I haven't reviewed the function in full, but this GNU-style comment
jumped out on the way down while scrolling.  I believe you already
know what comments should look like to be consistent with the rest
of the git codebase.

Ok, I've reached the end.  Probably some of these comments apply to
code that might be changed anyway, but still, hope that helps a
little.  And thanks for working on this.