Re: [PATCH] cherry-pick: allow to pick to unborn branches

Michael J Gruber [off-list ref] writes:

Currently, cherry-pick allows tp pick single commits to an empty HEAD
but not multiple commits.

Allow the multiple commit case, too.

Reported-by: Fabrizio Cucci <redacted>
Signed-off-by: Michael J Gruber <redacted>
---
 sequencer.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Thanks; we'd probably want a few tests in somewhere near 3503 or
3505.

Is this error-fail desirable?  How do we get here?  Did a user start
a cherry-pick on an unborn branch and then told us to abort that?
Shouldn't we be taking the user back to the "orphan" state if that
is the case?

Not a new issue, but I cannot quite fathom what this error message
wants to say.  "Can't revert an initial commit"?

 	save_head(sha1_to_hex(sha1));
 	save_opts(opts);
 	return pick_commits(todo_list, opts);

Junio C Hamano venit, vidit, dixit 06.06.2016 22:06:

Michael J Gruber [off-list ref] writes:

quoted

Currently, cherry-pick allows tp pick single commits to an empty HEAD
but not multiple commits.

Allow the multiple commit case, too.

Reported-by: Fabrizio Cucci <redacted>
Signed-off-by: Michael J Gruber <redacted>
---
 sequencer.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Thanks; we'd probably want a few tests in somewhere near 3503 or
3505.

I'll try my best.

quoted

diff --git a/sequencer.c b/sequencer.c
index 4687ad4..c6362d6 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -888,6 +888,10 @@ static int sequencer_rollback(struct replay_opts *opts)
 			git_path_head_file());
 		goto fail;
 	}
+	if (is_null_sha1(sha1)) {
+		error(_("cannot abort from a branch yet to be born"));
+		goto fail;
+	}

Is this error-fail desirable?  How do we get here?  Did a user start
a cherry-pick on an unborn branch and then told us to abort that?
Shouldn't we be taking the user back to the "orphan" state if that
is the case?

Here and below, I'm mimicking/copying the behavior that we have right
now already. I asked myself the same question - rolling back to orphan
state shouldn't be that hard after all. But that would be a change in
behavior that - if considered a fix/improvement - would be orthogonal to
the multi-pick fix.

quoted

 	if (reset_for_rollback(sha1))
 		goto fail;
 	remove_sequencer_state();

@@ -1086,11 +1090,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	walk_revs_populate_todo(&todo_list, opts);
 	if (create_seq_dir() < 0)
 		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"));
-	}
+	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
+		return error(_("Can't revert as initial commit"));

Not a new issue, but I cannot quite fathom what this error message
wants to say.  "Can't revert an initial commit"?

Cannot create a "revert commit" as the initial commit on a yet unborn
branch. Maybe "nothing to revert" would be clearer, but then another
user might ask: If I say "git revert deabeef" and there is a commit
"deadbeef" then why is there "nothing to revert"?

Applying the reverse of a patch to an empty tree should result in an
empty tree, and creating a commit with that empty tree as "This reverts
commit deadbeef" is what we are refusing here, unless I'm confused.

quoted

 	save_head(sha1_to_hex(sha1));
 	save_opts(opts);
 	return pick_commits(todo_list, opts);