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

[Miklos Vajna <vmiklos@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]