Hi Jonathan,
Jonathan Nieder writes:
> 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? :)
Haven't I explained this sufficiently well in the next sentence?
> > 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?
Sorry about the ENOPARSE. Yes, exactly.
> > --- 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"?
Nice catch! Yes, GIT_REFLOG_ACTION should be in pick_commits.
> Aside from clarifying that detail, this one looks good.
Thanks for the review. This patch is probably the one with the least
number of mistakes :)
-- Ram
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html