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

Jeff King [off-list ref] 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.

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.

For example, in the kernel repository, what would this do, I have to
wonder:

    $ git tag c2.6.12 v2.6.12^{commit}
    $ git tag t2.6.12 v2.6.12^{tree}
    $ git tag -l -n 12 c2.6.12 t2.6.12

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

Jeff King [off-list ref] writes:

quoted

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