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

[Ramkumar Ramachandra <artagnon@gmail.com>]

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.