Re: [PATCH v3] cherry-pick: make sure all input objects are commits

Junio C Hamano [off-list ref] writes:

Miklos Vajna [off-list ref] writes:

quoted

When a single argument was a non-commit, the error message used to be:

	fatal: BUG: expected exactly one commit from walk

For multiple arguments, when none of the arguments was a commit, the error was:

	fatal: empty commit set passed

Finally, when some of the arguments were non-commits, we ignored those
arguments.  Fix this bug and make sure all arguments are commits, and
for the first non-commit, error out with:

	fatal: <name>: Can't cherry-pick a <type>

Signed-off-by: Miklos Vajna <redacted>

This turns out to be an irritatingly stupid change.  While I am
rebuilding a privately tagged tip of 'maint', I am seeing:

	fatal: v1.8.2.3: Can't cherry-pick a tag

You would want to reject non committish, not non commit.

I'd apply this before -rc2.  I _think_ it is also OK to just let
lookup_commit_reference_gently() barf with its standard message

	error: Object %s is a %s, not a commit

without an extra sha1_object_info() call in the error codepath, but
I did not bother, as this is meant to be an emergency fix.

-- >8 --
Subject: cherry-pick: picking a tag that resolves to a commit is OK

Earlier, 21246dbb9e0a (cherry-pick: make sure all input objects are
commits, 2013-04-11) tried to catch an unlikely "git cherry-pick $blob"
as an error, but broke a more important use case to cherry-pick a
tag that points at a commit.

Signed-off-by: Junio C Hamano <redacted>
---
 sequencer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 61fdb68..f2c9d98 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1077,10 +1077,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 			continue;
 
 		if (!get_sha1(name, sha1)) {
-			enum object_type type = sha1_object_info(sha1, NULL);
-
-			if (type > 0 && type != OBJ_COMMIT)
+			if (!lookup_commit_reference_gently(sha1, 1)) {
+				enum object_type type = sha1_object_info(sha1, NULL);
 				die(_("%s: can't cherry-pick a %s"), name, typename(type));
+			}
 		} else
 			die(_("%s: bad revision"), name);
 	}

On Thu, May 09, 2013 at 01:27:49PM -0700, Junio C Hamano [off-list ref] wrote:

I'd apply this before -rc2.  I _think_ it is also OK to just let
lookup_commit_reference_gently() barf with its standard message

	error: Object %s is a %s, not a commit

without an extra sha1_object_info() call in the error codepath, but
I did not bother, as this is meant to be an emergency fix.

Yes, that makes a lot of sense. I myself never cherry-pick tags, but I
understand that is part of some workflow.