Re: [PATCH v2] bisect: don't use invalid oid as rev when starting
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 24, 2020, 18:55 UTC
- Message-ID
- <xmqqsgb7m2bq.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20200924060344.15541-1-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
Show 7 quoted lines
> - } 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.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.
We may need to audit recent additions of get_oid_committish() calls in our codebase. I suspect there may be other instances of the same mistake.
Other than that, the code structure does look more straight-forward compared to the previous round. A fix on this version would involve peeling what is in oid down to commit, and you need to handle errors during that process, so it may not stay pretty with a fix, though. I dunno.
Thanks.
Show 29 quoted lines
> string_list_append(&revs, oid_to_hex(&oid));
> - free(commit_id);
> - }
> + else if (has_double_dash)
> + die(_("'%s' does not appear to be a valid "
> + "revision"), arg);
> + else
> + break;
> }
> pathspec_pos = i;
>
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index b886529e59..70c39a9459 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' '
> git bisect bad $HASH4
> '
>
> +test_expect_success 'bisect start without -- uses unknown arg as path restriction' '
> + git bisect reset &&
> + git bisect start foo bar &&
> + grep foo ".git/BISECT_NAMES" &&
> + grep bar ".git/BISECT_NAMES"
> +'
> +
> test_expect_success 'bisect reset: back in the master branch' '
> git bisect reset &&
> echo "* master" > branch.expect &&