Re: [PATCH] ref-filter: fix NULL check for parse object failure

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

Jeff King <peff@peff.net> writes:

> Here's a patch to fix it. This is mostly orthogonal to your patch
> series. It happens to use a similar recipe to reproduce, but that is not
> the only way to do it, and the fix and the test shouldn't conflict
> textually or semantically.
>
> -- >8 --
> Subject: [PATCH] ref-filter: fix NULL check for parse object failure
>
> After we run parse_object_buffer() to get an object's contents, we try
> to check that the return value wasn't NULL. However, since our "struct
> object" is a pointer-to-pointer, and we assign like:
>
>   *obj = parse_object_buffer(...);
>
> it's not correct to check:
>
>   if (!obj)
>
> That will always be true, since our double pointer will continue to
> point to the single pointer (which is itself NULL). This is a regression
> that was introduced by aa46a0da30 (ref-filter: use oid_object_info() to
> get object, 2018-07-17); since that commit we'll segfault on a parse
> failure, as we try to look at the NULL object pointer.
>
> There are many ways a parse could fail, but most of them are hard to set
> up in the tests (it's easy to make a bogus object, but update-ref will
> refuse to point to it). The test here uses a tag which points to a wrong
> object type. A parse of just the broken tag object will succeed, but
> seeing both tag objects in the same process will lead to a parse error
> (since we'll see the pointed-to object as both types).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  ref-filter.c            |  2 +-
>  t/t6300-for-each-ref.sh | 10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)

Makes sense.  Will queue.

> diff --git a/ref-filter.c b/ref-filter.c
> index f0bd32f714..a0adb4551d 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1608,7 +1608,7 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj
>  
>  	if (oi->info.contentp) {
>  		*obj = parse_object_buffer(the_repository, &oi->oid, oi->type, oi->size, oi->content, &eaten);
> -		if (!obj) {
> +		if (!*obj) {
>  			if (!eaten)
>  				free(oi->content);
>  			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index cac7f443d0..2e7c32d50c 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -1134,4 +1134,14 @@ test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'for-each-ref reports broken tags' '
> +	git tag -m "good tag" broken-tag-good HEAD &&
> +	git cat-file tag broken-tag-good >good &&
> +	sed s/commit/blob/ <good >bad &&
> +	bad=$(git hash-object -w -t tag bad) &&
> +	git update-ref refs/tags/broken-tag-bad $bad &&
> +	test_must_fail git for-each-ref --format="%(*objectname)" \
> +		refs/tags/broken-tag-*
> +'
> +
>  test_done