From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 11 May 2011 11:49:09 GMT
Subject: Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code
Message-ID: <20110511114909.GE2676@elie>
In-Reply-To: <1305100822-20470-5-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> Reading the Git configuration, setting environment variables, parsing
> command-line arguments, and populating the options structure should be
> done in cmd_cherry_pick/ cmd_revert.

Yes, but why? :)

> The job pick_commits of
> simplified into setting up the revision walker and calling
> do_pick_commit in a loop- later in the series, it will handle
> failures, and serve as the starting point for continuation.

ENOPARSE.  I assume the idea is that callers will want to decide what
they want pick_commits to do and specify it by filling a struct
instead of argc and argv.  Is that it?

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>  	return 0;
>  }
>  
> -static int revert_or_cherry_pick(int argc, const char **argv,
> -				struct replay_opts *opts)
> +static int pick_commits(struct replay_opts *opts)
>  {
>  	struct rev_info revs;
>  	struct commit *commit;
> -	const char *me;
>  	int res;
>  
> -	git_config(git_default_config, NULL);
> -	me = (opts->action == REVERT ? "revert" : "cherry-pick");
> -	setenv(GIT_REFLOG_ACTION, me, 0);
> -	parse_args(argc, argv, opts);
> -
>  	if (opts->allow_ff) {

I don't see why the caller sets up GIT_REFLOG_ACTION, since the caller
is not making the commits.  Is there an example where it would use
something other than "cherry-pick" or "revert"?

Aside from clarifying that detail, this one looks good.