Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Aug 11, 2011, 06:16 UTC
Message-ID: <CALkWK0kxtyPABBUOrXtKDxPCBKt3CynoP4Fm8f_+C1ymkLTo-w@mail.gmail.com>
In-Reply-To: <20110810155332.GB4076@elie.gateway.2wire.net>

Hi Jonathan,

Jonathan Nieder writes:

> Well, "beautiful public API" means "just what cmd_cherry_pick and
> cmd_revert needs", right?  So I'd suggest:

Yes, but I don't want to put stuff that's too specific to cherry-pick/ revert in the sequencer.

> [...]
> Luckily step (1) is already done.  The functions are parse_args()
> and pick_revisions() (though they could presumably use less generic
> names).

Are you certain about pick_revisions?  I've copied over the function
here for your reference.  My issue is that it's too specific
cherry-pick/ revert:
1. See what walk_revs_populate_todo does: It takes all the operands
and fills up "pick" as the operator.  Why would any other caller want
to do this?
2. You mentioned multiple entry points earlier, and that's something
I've been meaning to do: In the long run, I don't want callers to fill
up an opts structure to specify the subcommand! That'd be butt-ugly.

-- 8< --
static int pick_revisions(struct replay_opts *opts)
{
	struct replay_insn_list *todo_list = NULL;
	unsigned char sha1[20];

	read_and_refresh_cache(opts);

	/*
	 * Decide what to do depending on the arguments; a fresh
	 * cherry-pick should be handled differently from an existing
	 * one that is being continued
	 */
	if (opts->subcommand == REPLAY_RESET) {
		remove_sequencer_state(1);
		return 0;
	} else if (opts->subcommand == REPLAY_CONTINUE) {
		if (!file_exists(git_path(SEQ_TODO_FILE)))
			goto error;
		sequencer_read_opts(&opts);
		sequencer_read_todo(&todo_list);

		/* Verify that the conflict has been resolved */
		if (!index_differs_from("HEAD", 0))
			todo_list = todo_list->next;
	} else {
		/*
		 * Start a new cherry-pick/ revert sequence; but
		 * first, make sure that an existing one isn't in
		 * progress
		 */

		walk_revs_populate_todo(&todo_list, opts);
		if (sequencer_create_dir() < 0) {
			error(_("A cherry-pick or revert is in progress."));
			advise(_("Use --continue to continue the operation"));
			advise(_("or --reset to forget about it"));
			return -1;
		}
		if (get_sha1("HEAD", sha1)) {
			if (opts->action == REPLAY_REVERT)
				return error(_("Can't revert as initial commit"));
			return error(_("Can't cherry-pick into empty head"));
		}
		sequencer_save_head(sha1_to_hex(sha1));
		sequencer_save_opts(opts);
	}
	return pick_commits(todo_list, opts);
error:
	return error(_("No %s in progress"), action_name(opts));
}