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

From: Miklos Vajna <[email-masked]>
Date: Thu, 11 Apr 2013 13:03:25 +0200

On Thu, Apr 11, 2013 at 03:52:44PM +0530, Ramkumar Ramachandra <[email-masked]> 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. :-)