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