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

2013-04-11 Thread Miklos Vajna 
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 vmik...@suse.cz
---

On Mon, Apr 08, 2013 at 09:56:55AM -0700, Junio C Hamano gits...@pobox.com 
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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-04-11 Thread Ramkumar Ramachandra 
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.

 diff --git a/sequencer.c b/sequencer.c
 index baa0310..eb25101 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -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));
 +   }

else?  What happens if get_sha1() fails?

 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-04-11 Thread Miklos Vajna 
On Thu, Apr 11, 2013 at 03:52:44PM +0530, Ramkumar Ramachandra 
artag...@gmail.com wrote:
  +   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.

  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. :-)


signature.asc
Description: Digital signature

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

2013-04-11 Thread Ramkumar Ramachandra 
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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html