[RFC PATCH] revert: Implement --abort processing

[RFC PATCH] revert: Implement --abort processing

[Ramkumar Ramachandra]

To abort, perform a "rerere clear" and "reset --hard" to the ref
specified by the HEAD file introduced earlier in the series.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 I want some feedback on this -- am I doing this right? Should I be
 re-implementing "reset --hard" like this?

 builtin/revert.c                |   84 +++++++++++++++++++++++++++++++++++++--
 t/t3510-cherry-pick-sequence.sh |   27 ++++++++++++
 2 files changed, 107 insertions(+), 4 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 8eee4d5..ae85df7 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -12,6 +12,7 @@
 #include "revision.h"
 #include "rerere.h"
 #include "merge-recursive.h"
+#include "unpack-trees.h"
 #include "refs.h"
 #include "dir.h"
 
@@ -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)
@@ -647,6 +646,44 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
+static int hard_reset_to_ref(unsigned char *sha1)
+{
+	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+	struct string_list merge_rr = STRING_LIST_INIT_DUP;
+	struct unpack_trees_options unpack_tree_opts;
+	struct tree_desc desc;
+	int fd;
+
+	/* rerere clear */
+	setup_rerere(&merge_rr, 0);
+	rerere_clear(&merge_rr);
+
+	/* Clean worktree, index */
+	read_cache_unmerged();
+
+	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+	unpack_tree_opts.head_idx = 1;
+	unpack_tree_opts.src_index = &the_index;
+	unpack_tree_opts.dst_index = &the_index;
+	unpack_tree_opts.fn = oneway_merge;
+	unpack_tree_opts.merge = 1;
+	unpack_tree_opts.update = 1;
+	unpack_tree_opts.reset = 1;
+
+	fd = hold_locked_index(lock, 1);
+	fill_tree_descriptor(&desc, sha1);
+	if (unpack_trees(1, &desc, &unpack_tree_opts) < 0)
+		return error (_("Failed to unpack trees."));
+	if (write_index(&the_index, fd) ||
+		commit_locked_index(lock))
+		return error(_("Could not write index."));
+
+	/* Update the HEAD ref */
+	if (update_ref("updating HEAD", "HEAD", sha1, NULL, 0, MSG_ON_ERR))
+		return error(_("Unable to update HEAD"));
+	return 0;
+}
+
 static void format_todo(struct strbuf *buf, struct commit_list *list,
 			struct replay_opts *opts)
 {
@@ -743,6 +780,45 @@ static int pick_commits(struct replay_opts *opts)
 	return cleanup_sequencer_dir();
 }
 
+static int process_continuation(struct replay_opts *opts)
+{
+	unsigned char sha1[20];
+	char head[40];
+	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, 40) < 40 || get_sha1_hex(head, sha1) < 0) {
+			close(fd);
+			return error(_("Corrupt '%s': %s"), HEAD_FILE, strerror(errno));
+		}
+		close(fd);
+
+		if (hard_reset_to_ref(sha1) < 0)
+			/* Error already reported */
+			return -1;
+		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;
+	}
+
+	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 +831,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 +851,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..7e22898 100644
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -36,4 +36,31 @@ 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_expect_success '--abort resets worktree and index after failed cherry-pick' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	git add foo &&
+	git cherry-pick --abort &&
+	git update-index -q --ignore-submodules --refresh &&
+	git diff-files --quiet --ignore-submodules &&
+	git diff-index --cached --quiet --ignore-submodules HEAD --
+'
+
 test_done
-- 
1.7.5.GIT

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

[Sverre Rabbelier]

Heya,

On Wed, Jun 1, 2011 at 11:07, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> To abort, perform a "rerere clear" and "reset --hard" to the ref
> specified by the HEAD file introduced earlier in the series.

Isn't this the kind of usecase we added 'git reset --merge' for?

-- 
Cheers,

Sverre Rabbelier

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> To abort, perform a "rerere clear" and "reset --hard" to the ref
> specified by the HEAD file introduced earlier in the series.

What is the scenario this new feature targets? Is it to recover from this?

    % git revert $that_single_faulty_commit
    ... oops conflicted, let's try if I can resolve ...
    % edit $conflicted_files
    ... yuck, I managed to resolve two paths but not other three ...
    ... let's give up for now ...

In that scenario, a recovery may involve "git reset --hard", but clearing
the rerere record for what you have already resolved may or may not be.  I
would imagine that the above "give up _for now_" would be concluded with:

    % git add $files_i_managed_to_resolve_this_round
    % git rerere
    $ git reset --hard

Even if you dropped the unconditional "rerere clear" from the patch, I am
not sure what this new feature buys us. Some people would want the rerere
cache cleared, some others don't. "revert --abort" will forever be to
aborting revert and restoring some but not all the parts of the operation
the user wants to be undone, as you cannot satisfy everybody.  So I am a
bit puzzled why you thought this was even a good idea to begin with.

"reset --hard" has established semantics people already understand what it
does. I would rather see us let them use that, without confusing them with
"what are the differences between the two?" unnecessarily.

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> To abort, perform a "rerere clear" and "reset --hard" to the ref
> specified by the HEAD file introduced earlier in the series.

The above doesn't explain why, so it makes it harder to answer:

>  Should I be
>  re-implementing "reset --hard" like this?

So I'll try to fill in the blanks.  Wouldn't it be better to call
"reset --merge" code?  That is what "git merge --abort" does.

Let's consider an example.

Suppose I have run "git cherry-pick foo" and run into a conflict.  Now
I start to fix things up the way I like, but I suddenly realize that
the cause was cherry-picking the wrong commit; it's better to start
over and do that.  So I try to abort.

I have some changes to files that did not participate in the automatic
cherry-pick:

 1. for unrelated reasons, I bumped the version number in the Makefile
 as a reminder not to forget later, without commiting it or marking
 with "git add";

 2. I (manually) moved a declaration to a different header file to
 reflect differences between the codebase at the time of foo^ and HEAD,
 to get it to compile.  Which works, so I mark it with "git add" for
 incorporation into the corrected cherry-pick commit.

With "git reset --merge", (1) is left alone, while (2) is backed out,
unmerged entries are of course clobbered, and hazy cases in which I
make some changes, "git add", and then make more changes without "git
add" cause the operation to error out.  It would be nicer if git could
read my mind, but at first glance this seems like an okay second-best.

Are there other considerations or situations that would lead to a
different conclusion?  (Not a rhetorical question.)

Hope that helps,
Jonathan

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

[Ramkumar Ramachandra]

Hi Sverre, Junio and Jonathan,

Sverre Rabbelier writes:
> Isn't this the kind of usecase we added 'git reset --merge' for?

Yes, I can see that now.

Junio Hamano writes:
> Even if you dropped the unconditional "rerere clear" from the patch, I am
> not sure what this new feature buys us. Some people would want the rerere
> cache cleared, some others don't. "revert --abort" will forever be to
> aborting revert and restoring some but not all the parts of the operation
> the user wants to be undone, as you cannot satisfy everybody.  So I am a
> bit puzzled why you thought this was even a good idea to begin with.

It's actually very specific to the way I work/ think -- I would have
expected an abort to go back in time, and make it look like the
operation wasn't performed in the first place. My normal workflow: I
make my changes, create a "fixup!" commit, abort, and cherry-pick that
commit from my reflog. Yes, I use "reset --hard" a lot, and yes, it's
a very powerful hammer.

I see now that this probably doesn't fit everyone's usecase. So the
changes I propose are:
1. Don't rerere clear. We can probably document this fact somewhere,
and hint the user about this during the time of abort.
2. Use reset --merge as Sverre suggested.

I'll think about this workflow and post a patch soon.

Jonathan Nieder writes:
> I have some changes to files that did not participate in the automatic
> cherry-pick:
>
>  1. for unrelated reasons, I bumped the version number in the Makefile
>  as a reminder not to forget later, without commiting it or marking
>  with "git add";
>
>  2. I (manually) moved a declaration to a different header file to
>  reflect differences between the codebase at the time of foo^ and HEAD,
>  to get it to compile.  Which works, so I mark it with "git add" for
>  incorporation into the corrected cherry-pick commit.
>
> With "git reset --merge", (1) is left alone, while (2) is backed out,
> unmerged entries are of course clobbered, and hazy cases in which I
> make some changes, "git add", and then make more changes without "git
> add" cause the operation to error out.  It would be nicer if git could
> read my mind, but at first glance this seems like an okay second-best.

Thanks for the excellent explanation. I'll think about this workflow
for a while before posting another iteration of this patch.

-- Ram

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> I see now that this probably doesn't fit everyone's usecase. So the
> changes I propose are:
> 1. Don't rerere clear. We can probably document this fact somewhere,
> and hint the user about this during the time of abort.
> 2. Use reset --merge as Sverre suggested.
>
> I'll think about this workflow and post a patch soon.

Please do the former but I would advice you not to do the latter.

If you do not clear rerere then you are not serving the original audience
you wanted to serve, and if you do, you are hurting people when they do
want to keep rerere. You cannot win either way.

The users are better off being exposed to "reset --merge" than kept
unaware of the concepts necessary to do what they want hidden behind
"revert --abort" that has fuzzy semantics that restores some but not all,
with the definition of "some" being something we happened to decide here,
which can never match what the user in every situation would want.