[RFC PATCH v2] revert: Implement --abort processing

[RFC PATCH v2] revert: Implement --abort processing

[Ramkumar Ramachandra]

To abort, simply reset the HEAD to the location specified by the
HEAD_FILE written earlier, without touching the worktree or index.  It
is upto the user to execute "rerere clear", "reset --hard", and "reset
--merge" as approprite.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 I've persisted the TODO early, and made it complain when an existing
 cherry-pick/ revert is in progress.  Further, as Junio suggested,
 I've made no attempt to touch the index or the worktree during the
 --abort operation: I simply update HEAD and leave the user to do the
 rest.  Should we print some hints about this?

 However, by making it complain everytime a cherry-pick/ revert is in
 progress, I've essentially broken the entire testsuite -- everytime a
 test or even a script (see git-rebase.sh) calls cherry-pick or
 revert, I must call '--abort' to clean up afterwards.  Fixing this
 will require me to touch many seemingly unrelated files.

 Also, note that I'm depending on the "libify reset" patch I posted a
 few minutes ago to print_new_head_line exactly the way reset does.

 In other news, I've started writing '--continue' and '--skip'
 operations, but the limiting factor is the instruction sheet's
 format: I'm still wondering if there's any way to avoid being
 verbose/ ugly and flexible (by allowing command-line options for each
 instruction) at the same time.  Will post RFC patches asking more
 directed questions soon.

 Thanks.

 builtin/revert.c                |   85 +++++++++++++++++++++++++++++++++++---
 t/t3510-cherry-pick-sequence.sh |   16 +++++++
 2 files changed, 94 insertions(+), 7 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index e1a05a3..efcc01f 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -14,6 +14,7 @@
 #include "merge-recursive.h"
 #include "refs.h"
 #include "dir.h"
+#include "reset.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -208,8 +209,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				NULL);
 
 	/* Remove these when the options are actually implemented */
-	if (opts->abort_oper)
-		die("--abort is not implemented yet");
 	if (opts->skip_oper)
 		die("--skip is not implemented yet");
 	if (opts->continue_oper)
@@ -719,6 +718,7 @@ static int pick_commits(struct replay_opts *opts)
 	struct commit *commit;
 	unsigned char sha1[20];
 	const char *head;
+	int res;
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
 
@@ -730,11 +730,30 @@ static int pick_commits(struct replay_opts *opts)
 	persist_head(head);
 	prepare_revs(&revs, opts);
 
+	/* Prepare todo_list and persist it early before picking */
+	struct commit_list *todo_list = NULL;
+	struct commit_list *cur = NULL;
+	struct commit_list *new_item = NULL;
+
+	/* Insert into todo_list in the same order */
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit, opts);
+		new_item = xmalloc(sizeof(struct commit_list));
+		new_item->item = commit;
+		if (cur)
+			cur->next = new_item;
+		else
+			todo_list = new_item;
+		cur = new_item;
+	}
+	cur->next = NULL;
+
+	persist_todo(todo_list, opts);
+
+	for (cur = todo_list; cur; cur = cur->next) {
+		res = do_pick_commit(cur->item, opts);
 		if (res) {
-			commit_list_insert(commit, &revs.commits);
-			persist_todo(revs.commits, opts);
+			commit_list_insert(cur->item, &todo_list);
+			persist_todo(todo_list, opts);
 			return res;
 		}
 	}
@@ -743,6 +762,58 @@ static int pick_commits(struct replay_opts *opts)
 	return cleanup_sequencer_dir();
 }
 
+static int process_continuation(struct replay_opts *opts)
+{
+	unsigned char sha1[20];
+	struct commit *commit;
+	char head_hex[40];
+	char msg[1024];
+	int fd;
+
+	if (opts->abort_oper) {
+		/* First, read the HEAD_FILE */
+		if (!file_exists(HEAD_FILE))
+			goto error;
+		fd = open(HEAD_FILE, O_RDONLY, 0666);
+		if (fd < 0)
+			return error(_("Could not open '%s' for reading: %s"),
+				HEAD_FILE, strerror(errno));
+		if (read_in_full(fd, head_hex, 40) < 40 || get_sha1_hex(head_hex, sha1) < 0) {
+			close(fd);
+			return error(_("Corrupt '%s': %s"), HEAD_FILE, strerror(errno));
+		}
+		close(fd);
+
+		/* Update the HEAD ref */
+		if (snprintf(msg, sizeof(msg), "%s: updating HEAD", me) >= sizeof(msg))
+			warning(_("Reflog action message too long: %.*s..."), 50, msg);
+		if (update_ref(msg, "HEAD", sha1, NULL, 0, MSG_ON_ERR))
+			return -1;
+		commit = lookup_commit_reference(sha1);
+		print_new_head_line(commit);
+
+		return cleanup_sequencer_dir();
+	}
+	else if (opts->skip_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+	}
+	else if (opts->continue_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+	}
+	else if (file_exists(HEAD_FILE)) {
+		error(_("A %s is already in progress"), me);
+		advise(_("Use %s --continue to continue the operation"), me);
+		advise(_("or %s --abort to start afresh"), me);
+		return -1;
+	}
+
+	return pick_commits(opts);
+error:
+	return error(_("No %s in progress"), me);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res;
@@ -755,7 +826,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	me = "revert";
 	parse_args(argc, argv, &opts);
-	res = pick_commits(&opts);
+	res = process_continuation(&opts);
 	if (res > 0)
 		/* Exit status from conflict */
 		return res;
@@ -775,7 +846,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	me = "cherry-pick";
 	parse_args(argc, argv, &opts);
-	res = pick_commits(&opts);
+	res = process_continuation(&opts);
 	if (res > 0)
 		return res;
 	if (res < 0)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index a2e1888..c6ace35 100644
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -36,4 +36,20 @@ test_expect_success 'cherry-pick cleans up sequencer directory upon success' '
 	test_must_fail test -d .git/sequencer
 '
 
+test_expect_success '--abort complains when no cherry-pick is in progress' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --abort >actual 2>&1 &&
+	echo "error: No cherry-pick in progress" >expect &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success '--abort restores HEAD after failed cherry-pick' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	git cherry-pick --abort &&
+	newhead=$(git rev-parse HEAD) &&
+	test "$head" = "$newhead"
+'
+
 test_done
-- 
1.7.4.1

Re: [RFC PATCH v2] revert: Implement --abort processing

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>  I've persisted the TODO early, and made it complain when an existing
>  cherry-pick/ revert is in progress.  Further, as Junio suggested,
>  I've made no attempt to touch the index or the worktree during the
>  --abort operation: I simply update HEAD and leave the user to do the
>  rest.

Is that what Junio was suggesting?  I thought he was saying that
cherry-pick shouldn't support an --abort option at all, since the
"right" semantics are hard to find.

I have sympathy for that point of view, too, but it's hard to
come up with the right choice.

On one hand: "git am" has an --abort option so you can say, "Forget
it; I'm going to go back to my mailer and make a better mailbox."
"git rebase" has an --abort option to let you to walk away in a
comfortable state, instead of leaving the HEAD detached and halfway
through replaying your work.  "git bisect" has a reset subcommand for
a similar reason.  All of the abort actions just mentioned take a
somewhat weird state (e.g., patch series partially applied, maybe with
conflicts) and go back to something more familiar.  Which is useful,
especially for people just starting out.

It applies equally to cherry-pick: a user doing something crazy like

	git cherry-pick HEAD..linux-next

to incorporate all patches from linux-next on top of her local changes
is creating a mess; there is appeal in having a way to say "clear that
away and take me back to something I knew".

On the other hand: all three of the above "abort" actions can be a
pain in the neck in practice.  The worst case is when I forget about
the sequence in progress and do something else, and only later want to
clear the state:

 http://thread.gmane.org/gmane.comp.version-control.git/164002/focus=164046

There are other cases, too, like when after partially rebasing I
decide this isn't a good direction for the original branch after all
but I want to end the rebase and develop HEAD as a new branch instead.
The --abort command is quite destructive and not very flexible.

I guess if I were in your shoes, I'd be tempted to start by saving the
old HEAD and making it visible as a pseudo-ref or something,
advertising that, and then observing people's behavior with the hope
of using that knowledge to come up with good semantics for a command
to cancel cherry-picks of commit ranges.  Do people use "reset --hard"
or "reset --merge"?  Do they throw away all the commits cherry-picked
or just a few?  Do they ever intend to abort like this even after
veering from the standard sequence, like in Linus's example?  After
aborting a multiple cherry-pick, what does a person generally do next?

The documentation could say:

	To wipe out everything and get back to where you started, use:

		git reset --hard PRE_CHERRY_PICK_HEAD

What if instead of --abort something else were simpler to think about?
As a random example, I can imagine a "git sequencer --edit" command
that would present the sequence in an editor and let me revise the
plan --- would that do the trick?

	1
	2
	3
	4
	* YOU ARE HERE
	5
	6
	7
	8

 - Remove lines 5-8: removes sequencer state, leaves HEAD as is
 - Remove everything: rewinds to abort sequence
 - Add a line 2.5 between 2 and 3: rewind to 2, cherry-pick 2.5,
   continue.

And having said that, I personally start to see an --abort command as
interesting, because it is a specific case that could help flesh out a
more general behavior of rewinding some day years from now.

Which is to say: if you have a story about what --abort will be used
for, the life of others evaluating the thing becomes better and the
upsides and downsides can be seen in perspective.  A story like "am
and rebase have --abort, so cherry-pick should have one, too" is
harder to think about.

Hope that helps.

Re: [RFC PATCH v2] revert: Implement --abort processing

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> The documentation could say:
>
>        To wipe out everything and get back to where you started, use:
>
>                git reset --hard PRE_CHERRY_PICK_HEAD

My notion of --abort has changed: I simply want to remove the state
files for the cherry-pick so that the user can execute more
cherry-pick/ revert commands.  I didn't think a soft reset would be
intrusive.  Hm, you're suggesting using a ref -- I was wondering what
to do with CHERY_PICK_HEAD.  I'm not entirely convinced, but I'll wait
for the others to comment and think about this for some time.

> What if instead of --abort something else were simpler to think about?
> As a random example, I can imagine a "git sequencer --edit" command
> that would present the sequence in an editor and let me revise the
> plan --- would that do the trick?
>
>        1
>        2
>        3
>        4
>        * YOU ARE HERE
>        5
>        6
>        7
>        8
>
>  - Remove lines 5-8: removes sequencer state, leaves HEAD as is
>  - Remove everything: rewinds to abort sequence
>  - Add a line 2.5 between 2 and 3: rewind to 2, cherry-pick 2.5,
>   continue.

Interesting perspective.  I essentially have to keep two TODO files
and run a diff until I find the first different line.  Then, I have to
reset to that SHA-1 and replay the rest of the user-edited TODO.  Can
be complicated to get right, but it seems like quite an elegant
interactive solution.  It takes care of all three: --abort and --skip.
 We'd still need a --continue to non-interactively continue after a
conflict resolution though, no?

> Which is to say: if you have a story about what --abort will be used
> for, the life of others evaluating the thing becomes better and the
> upsides and downsides can be seen in perspective.  A story like "am
> and rebase have --abort, so cherry-pick should have one, too" is
> harder to think about.

I agree.  There's little point in being stuck with historical
conventions -- we should try something new and see how it works out :)

Thanks.

-- Ram

Re: [RFC PATCH v2] revert: Implement --abort processing

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> My notion of --abort has changed: I simply want to remove the state
> files for the cherry-pick so that the user can execute more
> cherry-pick/ revert commands.  I didn't think a soft reset would be
> intrusive.

Well, if you understand this part then you can forget most of the
rest of what I said.  Think about this for a second.  New user (or
forgetful, experienced user), has just run

	git cherry-pick HEAD..topic

to integrate the changes from topic in a linear history.  Ran into
conflicts, wanted to give up.  Ran

	git cherry-pick --abort

Would this person expect:

 - that "git diff --cached" would return a pile of changes
 - that "git reset --keep", "git reset --merge", "git checkout",
   "git merge", and various other commands would refuse to do much,
   for fear of clobbering the new "local changes"
 - that the worktree would be unchanged
 - etc

Would they be happy about it?  Just put yourself in their shoes.  A
soft reset is near the most intrusive behavior possible.

And that is a good way to think about the UI for any new facility.  If
you disregard about how flexible it is in abstract, how easy to
implement, how elegant-sounding and just think about a person using it
will find her quality of life improved or worsened, that is (1) a good
sanity-check on a design and (2) basically the only way to explain it
to other people.

Re: [RFC PATCH v2] revert: Implement --abort processing

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Would this person expect:
>
>  - that "git diff --cached" would return a pile of changes
>  - that "git reset --keep", "git reset --merge", "git checkout",
>   "git merge", and various other commands would refuse to do much,
>   for fear of clobbering the new "local changes"
>  - that the worktree would be unchanged
>  - etc
>
> Would they be happy about it?  Just put yourself in their shoes.  A
> soft reset is near the most intrusive behavior possible.

Okay, it sounds pretty terrible when you put it like that :p

> And that is a good way to think about the UI for any new facility.  If
> you disregard about how flexible it is in abstract, how easy to
> implement, how elegant-sounding and just think about a person using it
> will find her quality of life improved or worsened, that is (1) a good
> sanity-check on a design and (2) basically the only way to explain it
> to other people.

Frankly, I'm still trying to understand how other people work -- I
don't use the porcelain much, and I dislike anything but the most
minimalistic automation.  I didn't even like the change to cherry-pick
where you can simply "git commit" after resolving a conflict; I still
prefer and use the more explicit "git commit -c" after removing the
CHERRY_PICK_HEAD.  Also, I *always* use rebase with --onto, because I
explicitly want to know how many commits I'm picking, and I often
don't remember if I've rewritten the ref I'm rebasing onto.  While
rebasing, I use reset --hard a lot, because it's a very effective and
well-understood hammer; I then cherry-pick the refs I want from the
reflog.  I don't know how others work; I think I'm completely confused
about UI because I'm trying to mix my personal tastes, historical
conventions, and suggestions from others.
</rant>

Long story short: I'm hopeless at UI design. I'll think about the
sequencer UI from a fresh perspective again, and hopefully post
something sensible soon.

-- Ram

Re: [RFC PATCH v2] revert: Implement --abort processing

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Frankly, I'm still trying to understand how other people work -- I
> don't use the porcelain much, and I dislike anything but the most
> minimalistic automation.  I didn't even like the change to cherry-pick
> where you can simply "git commit" after resolving a conflict; I still
> prefer and use the more explicit "git commit -c" after removing the
> CHERRY_PICK_HEAD.  Also, I *always* use rebase with --onto

I don't think that should stop you from thinking about how new
facilities help or interfere with work at all.  You use magit, right?
It automates all kinds of things.  And while each person is going to
use tools in different ways, that hasn't kept people from getting
things done in the past.

If you are thinking "I would never use 'git cherry-pick --abort' --- I
would just look in the reflog for a commit to 'reset --hard' to", then
you are *done*.  Just document it, make sure the reflog has useful
content to help out, and wait until someone complains and adds a
shortcut they like.

Addendum:  Personally I was happy about "git commit" that implicitly
takes the commit message from CHERRY_PICK_HEAD because it adds a

	Conflicts:

		Makefile

that I can use as a template for a message about the nature of the
conflict and how I fixed it up.  As a side note, I'm curious about
why you end up needing to remove the CHERRY_PICK_HEAD.  Is "git commit
-c interesting-patch" misbehaving somehow?  (It should ignore the
CHERRY_PICK_HEAD entirely.)

Re: [RFC PATCH v2] revert: Implement --abort processing

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> I don't think that should stop you from thinking about how new
> facilities help or interfere with work at all.  You use magit, right?
> It automates all kinds of things.  And while each person is going to
> use tools in different ways, that hasn't kept people from getting
> things done in the past.

I use Magit only for staging, unstaging, committing, amending, and
reverting portions.

> If you are thinking "I would never use 'git cherry-pick --abort' --- I
> would just look in the reflog for a commit to 'reset --hard' to", then
> you are *done*.  Just document it, make sure the reflog has useful
> content to help out, and wait until someone complains and adds a
> shortcut they like.

Ah, thanks for the helpful advice. I'll stay away from deciding
end-user interfaces altogether, and just write in the infrastructure
for sequencing commits with a minimalistic UI. People can add more
later :)

> As a side note, I'm curious about
> why you end up needing to remove the CHERRY_PICK_HEAD.  Is "git commit
> -c interesting-patch" misbehaving somehow?  (It should ignore the
> CHERRY_PICK_HEAD entirely.)

Sorry, I should have been clearer.  I remove the CHERRY_PICK_HEAD to
avoid hitting "commit", "commit --amend" by mistake.  With "git commit
-c", it displays a pleasant message:

# It looks like you may be committing a cherry-pick.
# If this is not correct, please remove the file
#	.git/CHERRY_PICK_HEAD
# and try again.

-- Ram

Re: [RFC PATCH v2] revert: Implement --abort processing

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> If you are thinking "I would never use 'git cherry-pick --abort' --- I
>> would just look in the reflog for a commit to 'reset --hard' to", then
>> you are *done*.  Just document it, make sure the reflog has useful
>> content to help out, and wait until someone complains and adds a
>> shortcut they like.
>
> Ah, thanks for the helpful advice. I'll stay away from deciding
> end-user interfaces altogether, and just write in the infrastructure
> for sequencing commits with a minimalistic UI. People can add more
> later :)

Well, until "later", this minimalistic UI is an end-user interface.
So just for the record, this does not mean you are free from writing
documentation or thinking about processes that humans use for work.

That said, following a "worse is better" approach and just getting
something simple working (meaning "operable by human beings but not
necessarily perfect") before adding the bells and whistles certainly
makes sense.  I'm not unhappy with that outcome.