Re: [PATCH 5/3] revert: introduce --abort to cancel a failed cherry-pick

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

> After running some ill-advised command like "git cherry-pick
> HEAD..linux-next", the bewildered novice may want to return to more
> familiar territory.  Introduce a "git cherry-pick --abort" command
> that rolls back the entire cherry-pick sequence and places the
> repository back on solid ground.

This is confusing; if you have many commits in the range, and a handful of
them replayed without conflicts and then you hit a conflict, where should
(I am not asking "where does ... with your patch") abort take us? The
state after the random commit that happened to have replayed successfully?
The state before the entire cherry-pick sequence started?  "back on solid
ground" does not tell us which one you meant.

I am assuming that it is the latter.

> diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
> index 75f8e869..5747f442 100644
> --- a/Documentation/sequencer.txt
> +++ b/Documentation/sequencer.txt
> @@ -7,3 +7,6 @@
>  	Forget about the current operation in progress.  Can be used
>  	to clear the sequencer state after a failed cherry-pick or
>  	revert.
> +
> +--abort::
> +	Cancel the operation and return to the pre-sequence state.

Ok, it is the latter.

> +static int reset_for_rollback(const unsigned char *sha1)
> +{
> +	const char *argv[4];	/* reset --merge <arg> + NULL */
> +	argv[0] = "reset";
> +	argv[1] = "--merge";
> +	argv[2] = sha1_to_hex(sha1);
> +	argv[3] = NULL;
> +	return run_command_v_opt(argv, RUN_GIT_CMD);
> +}

So you give the value of the HEAD before the sequence started to this
function and all should go well. Where do you read that value from?

> +static int rollback_single_pick(void)
> +{
> +	unsigned char head_sha1[20];
> +
> +	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
> +	    !file_exists(git_path("REVERT_HEAD")))
> +		return error(_("no cherry-pick or revert in progress"));
> +	if (!resolve_ref("HEAD", head_sha1, 0, NULL))
> +		return error(_("cannot resolve HEAD"));
> +	if (is_null_sha1(head_sha1))
> +		return error(_("cannot abort from a branch yet to be born"));

Ok, this is for single-pick so HEAD is where we came from. Good.

> +	return reset_for_rollback(head_sha1);
> +}
> +
> +static int sequencer_rollback(struct replay_opts *opts)
> +{
> +	const char *filename;
> +	FILE *f;
> +	unsigned char sha1[20];
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	filename = git_path(SEQ_HEAD_FILE);
> +	f = fopen(filename, "r");
> +	if (!f && errno == ENOENT) {
> +		/*
> +		 * There is no multiple-cherry-pick in progress.
> +		 * If CHERRY_PICK_HEAD or REVERT_HEAD indicates
> +		 * a single-cherry-pick in progress, abort that.
> +		 */
> +		return rollback_single_pick();
> +	}
> +	if (!f)
> +		return error(_("cannot open %s: %s"), filename,
> +						strerror(errno));
> +	if (strbuf_getline(&buf, f, '\n')) {
> +		error(_("cannot read %s: %s"), filename, ferror(f) ?
> +			strerror(errno) : _("unexpected end of file"));
> +		goto fail;
> +	}

And when we are in multi-pick, SEQ_HEAD_FILE has it.

Looks good from a cursory review. Thanks.