From: Junio C Hamano <junkio@cox.net>
Date: Wed, 02 Nov 2005 21:18:43 GMT
Subject: Re: [PATCH] Warn when calling deref_tag() on broken tags
Message-ID: <7vbr12px3g.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20051102204101.GE1431@pasky.or.cz>

Petr Baudis <pasky@suse.cz> writes:

> +struct object *deref_tag(struct object *o, const char *refname)
>  {
> +	struct object *o2 = o;
>  	while (o && o->type == tag_type)
> -		o = parse_object(((struct tag *)o)->tagged->sha1);
> +		o = parse_object(((struct tag *)(o2 = o))->tagged->sha1);
> +	if (!o)
> +		fprintf(stderr, "warning: invalid tag %s\n",
> +		        refname ? refname
> +		        : (o2 ? sha1_to_hex(o2->sha1)
> +		           : "<unknown> (this is probably internal GIT error)"));
>  	return o;
>  }

I wonder if it would make more sense to keep the root of the
traversal for this function (i.e. the original value of o) for
error reporting purposes, like this:

        struct object *deref_tag(struct object *o, const char *refname)
        {
                struct object *o2 = o;
                while (o && o->type == tag_type)
                        o = parse_object(((struct tag *)o)->tagged->sha1);
                if (!o)
                        fprintf(stderr, "warning: invalid tag %s\n",
                                refname ? refname
                                : (o2 ? sha1_to_hex(o2->sha1)
                                   : "<unknown> (this is probably internal GIT error)"));
                return o;
        }

What do you think?

Other than that, the patch is OK by me.  Thanks.