Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

[Jonathan Nieder <jrnieder@xxxxxxxxx>]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> Another thought.  I wonder if it's possible to leave
>> sequencer_parse_args() private to builtin/revert.c, making the split
>> a little more logical:
>
> Yes, I'd like this too.  However, there are two new issues:
> revert_or_cherry_pick_usage and action_name.  The former has two
> callsites: one in prepare_revs (in sequencer.c) and another in
> parse_args (in builtin/revert.c).

So it sounds like the answer is "no, it's not possible without
further changes".  Alas. :)  Thanks for checking.

(Q. Wait, what further changes?

 A. The above suggests that the setup_revisions call should also be
    the responsibility of the builtin, and that it could communicate
    revs and other rev-list options using a

	struct rev_info *revs;

    instead of

	int commit_argc;
	const char **commit_argv;

    Like this, maybe:

 builtin/revert.c |   52 ++++++++++++++++++++++++++++++----------------------
 1 files changed, 30 insertions(+), 22 deletions(-)

diff --git i/builtin/revert.c w/builtin/revert.c
index 8b452e81..f602ece0 100644
--- i/builtin/revert.c
+++ w/builtin/revert.c
@@ -55,13 +55,14 @@ struct replay_opts {
 	int allow_rerere_auto;
 
 	int mainline;
-	int commit_argc;
-	const char **commit_argv;
 
 	/* Merge strategy */
 	const char *strategy;
 	const char **xopts;
 	size_t xopts_nr, xopts_alloc;
+
+	/* Only used by the default subcommand ("git revert <revs>") */
+	struct rev_info *revs;
 };
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -164,7 +165,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			die(_("program error"));
 	}
 
-	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
+	argc = parse_options(argc, argv, NULL, options, usage_str,
 					PARSE_OPT_KEEP_ARGV0 |
 					PARSE_OPT_KEEP_UNKNOWN);
 
@@ -201,9 +202,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				NULL);
 	}
 
-	else if (opts->commit_argc < 2)
-		usage_with_options(usage_str, options);
-
 	if (opts->allow_ff)
 		verify_opt_compatible(me, "--ff",
 				"--signoff", opts->signoff,
@@ -211,7 +209,23 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"-x", opts->record_origin,
 				"--edit", opts->edit,
 				NULL);
-	opts->commit_argv = argv;
+
+	if (opts->subcommand != REPLAY_NONE) {
+		opts->revs = NULL;
+		if (argc > 1)
+			usage_with_options(usage_str, options);
+	} else {
+		opts->revs = xmalloc(sizeof(*opts->revs));
+		init_revisions(opts->revs, NULL);
+		opts->revs->no_walk = 1;
+
+		if (argc < 2)
+			usage_with_options(usage_str, options);
+
+		argc = setup_revisions(argc, argv, opts->revs, NULL);
+		if (argc > 1)
+			usage_with_options(usage_str, options);
+	}
 }
 
 struct commit_message {
@@ -612,23 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
+static void prepare_revs(struct replay_opts *opts)
 {
-	int argc;
-
-	init_revisions(revs, NULL);
-	revs->no_walk = 1;
 	if (opts->action != REVERT)
-		revs->reverse = 1;
+		opts->revs->reverse ^= 1;
 
-	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
-	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage(opts));
-
-	if (prepare_revision_walk(revs))
+	if (prepare_revision_walk(opts->revs))
 		die(_("revision walk setup failed"));
 
-	if (!revs->commits)
+	if (!opts->revs->commits)
 		die(_("empty commit set passed"));
 }
 
@@ -825,14 +831,13 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 static void walk_revs_populate_todo(struct commit_list **todo_list,
 				struct replay_opts *opts)
 {
-	struct rev_info revs;
 	struct commit *commit;
 	struct commit_list **next;
 
-	prepare_revs(&revs, opts);
+	prepare_revs(opts);
 
 	next = todo_list;
-	while ((commit = get_revision(&revs)))
+	while ((commit = get_revision(opts->revs)))
 		next = commit_list_append(commit, next);
 }
 
@@ -955,6 +960,9 @@ static int pick_revisions(struct replay_opts *opts)
 	struct commit_list *todo_list = NULL;
 	unsigned char sha1[20];
 
+	if (opts->subcommand == REPLAY_NONE)
+		assert(opts->revs);
+
 	read_and_refresh_cache(opts);
 
 	/*
-- 
)
--
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