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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 21, 2011, 17:19 UTC
Message-ID: <20110621171933.GP15461@elie>
In-Reply-To: <1308661489-20080-14-git-send-email-artagnon@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

[...]

Show 18 quoted lines

> @@ -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.