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

[Junio C Hamano <gitster@pobox.com>]

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.

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.

>  			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 &&