Re: [PATCH v2] bisect: don't use invalid oid as rev when starting

[Christian Couder <christian.couder@gmail.com>]

On Thu, Sep 24, 2020 at 8:55 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > -             } else {
> > -                     char *commit_id = xstrfmt("%s^{commit}", arg);
> > -                     if (get_oid(commit_id, &oid) && has_double_dash)
> > -                             die(_("'%s' does not appear to be a valid "
> > -                                   "revision"), arg);
> > -
> > +             } else if (!get_oid_committish(arg, &oid))
>
> This is wrong, I think.  The "_committish" only applies to the fact
> that the disambiguation includes only the objects that are
> committishes, and the result are not peeled---you'll get an
> annotated tag back in oid, if you give it an annotated tag that
> points at a commit.
>
> This is not what ^{commit} does.

Thanks for finding this.

> It may happen to work if the downstream consumers peel objects
> (which are appended to the 'revs' here) down to commit when they are
> used, and if somebody verified that is indeed the case, it would be
> OK, but if this patch is presented as "unlike the previous broken
> one, this is the faithful conversion of the original in shell, so
> there is no need to verify anything outside of the patch context",
> that is a problem.

I agree that it's better if there is no need to verify anything
outside of the patch context. So I agree with your fix.

I am also ok with using "pathspec" in the test description of the new test.

Thanks,
Christian.