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

[Jeff King <peff@peff.net>]

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

> > @@ -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.

> > +# 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