From: Jonathan Nieder <jrnieder@gmail.com>
Date: Fri, 13 May 2011 09:44:49 GMT
Subject: Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code
Message-ID: <20110513094449.GB30396@elie>
In-Reply-To: <20110513093501.GE14272@ramkum.desktop.amazon.com>

Ramkumar Ramachandra wrote:
> Ramkumar Ramachandra writes:
>>> Ramkumar Ramachandra wrote:

>>>> +++ 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)
>>>>  {
[...]
>>>> -	setenv(GIT_REFLOG_ACTION, me, 0);
>>>> -	parse_args(argc, argv, opts);
>>>> -
>>>>  	if (opts->allow_ff) {
[...]
>> Nice catch! Yes, GIT_REFLOG_ACTION should be in pick_commits.
>
> Er, I mean in do_pick_commit.  Right?

It seems somehow cleaner to set the envvar once in pick_commits,
assuming do_pick_commit is a private function that won't be exported.
But either way sounds fine to me.