Re: [PATCH 2/3] tag: die when listing missing or corrupt objects

From: Jeff King <peff@peff.net>
Date: Feb 6, 2012, 08:34 UTC
Message-ID: <20120206083407.GA9287@sigill.intra.peff.net>
In-Reply-To: <7vk4408ir6.fsf@alter.siamese.dyndns.org>

On Mon, Feb 06, 2012 at 12:32:13AM -0800, Junio C Hamano wrote:

Show 24 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > The first case is an indication of a broken or corrupt repo,
> > and we should notify the user of the error.
> >
> > The second case is OK to silently ignore; however, the
> > existing code leaked the buffer returned by read_sha1_file.
> > ...  
> >  	buf = read_sha1_file(sha1, &type, &size);
> > -	if (!buf || !size)
> > +	if (!buf)
> > +		die_errno("unable to read object %s", sha1_to_hex(sha1));
> > +	if (!size) {
> > +		free(buf);
> >  		return;
> > +	}
> >  
> >  	/* skip header */
> >  	sp = strstr(buf, "\n\n");
> 
> Hmm, a pedant in me says a tag object cannot have zero length, so the
> second case is also an indication of a corrupt repository, unless the tag
> happens to be a lightweight one that refers directly to a blob object that
> is empty.

Yes. Or alternatively, it should just be caught in the strstr() case below (which would silently ignore it).

> For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
> make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
> because the definition of "first N lines" is compatible between tag and
> commit for the purpose of the -n option.

Yup. See patch 3. :)

-Peff