Re: [PATCH 1/2] pack-refs: write peeled entry for non-tags

From: Jeff King <peff@peff.net>
Date: Mar 17, 2013, 06:02 UTC
Message-ID: <20130317060228.GD16070@sigill.intra.peff.net>
In-Reply-To: <514478C0.6060008@alum.mit.edu>

On Sat, Mar 16, 2013 at 02:50:56PM +0100, Michael Haggerty wrote:

Show 20 quoted lines

> > @@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
> >  		return 0;
> >  
> >  	fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), path);
> > -	if (is_tag_ref) {
> > -		struct object *o = parse_object(sha1);
> > -		if (o->type == OBJ_TAG) {
> > -			o = deref_tag(o, path, 0);
> > -			if (o)
> > -				fprintf(cb->refs_file, "^%s\n",
> > -					sha1_to_hex(o->sha1));
> > -		}
> > +
> > +	o = parse_object(sha1);
> > +	if (o->type == OBJ_TAG) {
> 
> You suggested that I add a test (o != NULL) at the equivalent place in
> my code (which was derived from this code).  Granted, my code was
> explicitly intending to pass invalid SHA1 values to parse_object().  But
> wouldn't it be a good defensive step to add the same check here?

Hmm, yeah. That is not new code, but rather just reindented from above ("diff -w" makes it much more obvious what is going on).

It is probably worth dying rather than segfaulting, though it should be a separate patch (and I do not think it is sane to do anything except die here). I almost wonder if parse_object should die by default on bogus or missing objects, and the few callers who really want to handle the error can call parse_object_gently. I do not relish analyzing each caller, though. It would be simpler to add parse_object_or_die.

Show 7 quoted lines

> > +# This matches show-ref's output
> > +print_ref() {
> > +	echo "`git rev-parse "$1"` $1"
> > +}
> > +
> 
> CodingGuidelines prefers $() over ``.

Old habits die hard. :)

I'll re-roll with your suggestions in a moment.

-Peff