Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit

Miklos Vajna [off-list ref] writes:

When copy&paste goes wrong, and the user e.g. tries to cherry-pick a
blob, the error message used to be:

It is the other way around.  When the user tries to cherry-pick a
non-commit we say a correct but nonspecific "expected one commit",
and it does not matter how the user threw a non-commit at us.  One
possibility could be copy&paste going wrong.

	fatal: BUG: expected exactly one commit from walk

Instead, now it is:

	fatal: Can't cherry-pick a blob

I wonder what we would do when "git cherry-pick master: next"
is given.  That is not "single commit input" case and not covered by
this patch, but perhaps something we may want to diagnose?

In other words, perhaps we would want to inspect pending objects
before running prepare_revision_walk and make sure everybody is
commit-ish or something?

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.  Instead, now 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>
---

On Mon, Apr 08, 2013 at 09:56:55AM -0700, Junio C Hamano [off-list ref] wrote:

In other words, perhaps we would want to inspect pending objects
before running prepare_revision_walk and make sure everybody is
commit-ish or something?

Sure, that makes sense to me.

 sequencer.c                         | 13 +++++++++++++
 t/t3508-cherry-pick-many-commits.sh |  6 ++++++
 2 files changed, 19 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index baa0310..eb25101 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1047,6 +1047,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 {
 	struct commit_list *todo_list = NULL;
 	unsigned char sha1[20];
+	int i;
 
 	if (opts->subcommand == REPLAY_NONE)
 		assert(opts->revs);

@@ -1067,6 +1068,18 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	if (opts->subcommand == REPLAY_CONTINUE)
 		return sequencer_continue(opts);
 
+	for (i = 0; i < opts->revs->pending.nr; i++) {
+		unsigned char sha1[20];
+		const char *name = opts->revs->pending.objects[i].name;
+
+		if (!get_sha1(name, sha1)) {
+			enum object_type type = sha1_object_info(sha1, NULL);
+
+			if (type > 0 && type != OBJ_COMMIT)
+				die(_("%s: can't cherry-pick a %s"), name, typename(type));
+		}
+	}
+
 	/*
 	 * If we were called as "git cherry-pick <commit>", just
 	 * cherry-pick/revert it, set CHERRY_PICK_HEAD /

diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 4e7136b..19c99d7 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh

@@ -55,6 +55,12 @@ one
 two"
 '
 
+test_expect_success 'cherry-pick three one two: fails' '
+	git checkout -f master &&
+	git reset --hard first &&
+	test_must_fail git cherry-pick three one two:
+'
+
 test_expect_success 'output to keep user entertained during multi-pick' '
 	cat <<-\EOF >expected &&
 	[master OBJID] second

-- 
1.8.1.4

Miklos Vajna wrote:

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.  Instead, now make sure all arguments are commits, and for
the first non-commit, error out with:

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

Thanks.  This is worth fixing.

else?  What happens if get_sha1() fails?

So you're testing just the third case (where commit objects are mixed
with non-commit objects), which is arguably a bug.  Okay.

On Thu, Apr 11, 2013 at 03:52:44PM +0530, Ramkumar Ramachandra [off-list ref] wrote:

quoted

+       for (i = 0; i < opts->revs->pending.nr; i++) {
+               unsigned char sha1[20];
+               const char *name = opts->revs->pending.objects[i].name;
+
+               if (!get_sha1(name, sha1)) {
+                       enum object_type type = sha1_object_info(sha1, NULL);
+
+                       if (type > 0 && type != OBJ_COMMIT)
+                               die(_("%s: can't cherry-pick a %s"), name, typename(type));
+               }

else?  What happens if get_sha1() fails?

I guess that is a should-not-happen category. parse_args() calls
setup_revisions(), and that will already die() if the argument is not a
valid object at all.

quoted

diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 4e7136b..19c99d7 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh

@@ -55,6 +55,12 @@ one
 two"
 '

+test_expect_success 'cherry-pick three one two: fails' '
+       git checkout -f master &&
+       git reset --hard first &&
+       test_must_fail git cherry-pick three one two:
+'

So you're testing just the third case (where commit objects are mixed
with non-commit objects), which is arguably a bug.  Okay.

Yes. If you would want, I could of course add test cases for two other
cases when we already errored out and now the error message is just
changed, but I don't think duplicating the error message strings from
the code to the testsuite is really wanted. :-)

Miklos Vajna wrote:

I guess that is a should-not-happen category. parse_args() calls
setup_revisions(), and that will already die() if the argument is not a
valid object at all.

Then why do you have an if() guarding the code?  In my opinion, you
should have an else-clause that die()s with an appropriate message.

Yes. If you would want, I could of course add test cases for two other
cases when we already errored out and now the error message is just
changed, but I don't think duplicating the error message strings from
the code to the testsuite is really wanted. :-)

Nope, I'd never suggest that: this is fine.  What I meant is: you
should clarify that you're fixing a bug and adding a test to guard it,
in the commit message.

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>
---

On Thu, Apr 11, 2013 at 05:12:06PM +0530, Ramkumar Ramachandra [off-list ref] wrote:

Then why do you have an if() guarding the code?  In my opinion, you
should have an else-clause that die()s with an appropriate message.

And you were right -- I actually forgot about --stdin, where the 
else-clause is hit. Added that for now, excluding --stdin.

Nope, I'd never suggest that: this is fine.  What I meant is: you
should clarify that you're fixing a bug and adding a test to guard it,
in the commit message.

Done.

 sequencer.c                         | 18 ++++++++++++++++++
 t/t3508-cherry-pick-many-commits.sh |  6 ++++++
 2 files changed, 24 insertions(+)

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

@@ -1047,6 +1047,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 {
 	struct commit_list *todo_list = NULL;
 	unsigned char sha1[20];
+	int i;
 
 	if (opts->subcommand == REPLAY_NONE)
 		assert(opts->revs);

@@ -1067,6 +1068,23 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	if (opts->subcommand == REPLAY_CONTINUE)
 		return sequencer_continue(opts);
 
+	for (i = 0; i < opts->revs->pending.nr; i++) {
+		unsigned char sha1[20];
+		const char *name = opts->revs->pending.objects[i].name;
+
+		/* This happens when using --stdin. */
+		if (!strlen(name))
+			continue;
+
+		if (!get_sha1(name, sha1)) {
+			enum object_type type = sha1_object_info(sha1, NULL);
+
+			if (type > 0 && type != OBJ_COMMIT)
+				die(_("%s: can't cherry-pick a %s"), name, typename(type));
+		} else
+			die(_("%s: bad revision"), name);
+	}
+
 	/*
 	 * If we were called as "git cherry-pick <commit>", just
 	 * cherry-pick/revert it, set CHERRY_PICK_HEAD /

diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 4e7136b..19c99d7 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh

@@ -55,6 +55,12 @@ one
 two"
 '
 
+test_expect_success 'cherry-pick three one two: fails' '
+	git checkout -f master &&
+	git reset --hard first &&
+	test_must_fail git cherry-pick three one two:
+'
+
 test_expect_success 'output to keep user entertained during multi-pick' '
 	cat <<-\EOF >expected &&
 	[master OBJID] second

-- 
1.8.1.4

Miklos Vajna wrote:

Signed-off-by: Miklos Vajna <redacted>

This one looks good.  FWIW,

Reviewed-by: Ramkumar Ramachandra <redacted>