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

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

quoted

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

quoted

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