Re: [PATCH 04/10] get_short_sha1: peel tags when looking for treeish

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

Jeff King <peff@peff.net> writes:

> The treeish disambiguation function tries to peel tags, but
> it does so by calling:
>
>   deref_tag(lookup_object(sha1), ...);
>
> This will only work if we have previously looked at the tag
> and created a "struct tag" for it. Since parsing revision
> arguments typically happens before anything else, this is
> usually not the case, and we would fail to peel the tag (we
> are lucky that deref_tag() gracefully handles the NULL and
> does not segfault).

Makes perfect sense.

> Instead, we can use parse_object(). Note that this is the
> same fix done by 94d75d1 (get_short_sha1(): correctly
> disambiguate type-limited abbreviation, 2013-07-01), but
> that commit fixed only the committish disambiguator, and
> left the bug in the treeish one.

Can you share your secret tool you use to find this kind of thing?
Yes, the patch from that commit does look very similar to what we
see in this patch, but I'd love to see "I am fixing an incorrect
call to lookup-object by replacing it with parse-object; has there
been a similar fix?" automated ;-)

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  sha1_name.c                         | 2 +-
>  t/t1512-rev-parse-disambiguation.sh | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 38e51d9..432a308 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -269,7 +269,7 @@ static int disambiguate_treeish_only(const unsigned char *sha1, void *cb_data_un
>  		return 0;
>  
>  	/* We need to do this the hard way... */
> -	obj = deref_tag(lookup_object(sha1), NULL, 0);
> +	obj = deref_tag(parse_object(sha1), NULL, 0);
>  	if (obj && (obj->type == OBJ_TREE || obj->type == OBJ_COMMIT))
>  		return 1;
>  	return 0;
> diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
> index 30e0b80..dfd3567 100755
> --- a/t/t1512-rev-parse-disambiguation.sh
> +++ b/t/t1512-rev-parse-disambiguation.sh
> @@ -264,6 +264,13 @@ test_expect_success 'ambiguous commit-ish' '
>  	test_must_fail git log 000000000...
>  '
>  
> +# There are three objects with this prefix: a blob, a tree, and a tag. We know
> +# the blob will not pass as a treeish, but the tree and tag should (and thus
> +# cause an error).
> +test_expect_success 'ambiguous tags peel to treeish' '
> +	test_must_fail git rev-parse 0000000000f^{tree}
> +'
> +
>  test_expect_success 'rev-parse --disambiguate' '
>  	# The test creates 16 objects that share the prefix and two
>  	# commits created by commit-tree in earlier tests share a