On Thu, Sep 24, 2020 at 8:55 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> 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.
