Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code

> 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"?