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

Hi Jonathan,

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

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?

quoted

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.

quoted

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