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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html