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

From: Miklos Vajna <vmiklos@suse.cz>
Date: Apr 11, 2013, 11:03 UTC
Message-ID: <20130411110324.GD12770@suse.cz>
In-Reply-To: <CALkWK0n6FjGbXTqiOT_O6NbB5h0DLaNWKCCTQAFSO_BL-pPdBA@mail.gmail.com>

On Thu, Apr 11, 2013 at 03:52:44PM +0530, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 12 quoted lines

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

Show 16 quoted lines

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