cat-file --literally

sha1_file.c int sha1_object_info(const unsigned char *sha1, unsigned long *sizep) · line 2647

Quoted 3 lines · Karthik Nayak

sha1_file.c +2 -0

+	unsigned long mapsize;
...
+	map = map_sha1_file(sha1, &mapsize);

I know this is a pre-existing issue, but I'm not sure "unsigned long" is the right type here. Shouldn't it be a size_t?

sha1_file.c int sha1_object_info(const unsigned char *sha1, unsigned long *sizep) · line 2654

Quoted 8 lines

sha1_file.c +7 -0

+	if (!map)
+		return -1;
+	if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
+		status = error("unable to unpack %s header",
+			       sha1_to_hex(sha1));
+
+	for (i = 0; i < 32; i++) {

This number should probably be a constant.

sha1_file.c int sha1_object_info(const unsigned char *sha1, unsigned long *sizep) · line 2660

Quoted 7 lines

sha1_file.c +6 -0

+		if (hdr[i] == ' ') {
+			type[i] = '\0';
+			break;
+		}
+		type[i] = hdr[i];
+	}

type might end up without a trailing \0 here in the case where hdr has no space in it. Is this possible?

Quoted 7 lines · David Turner

sha1_file.c int sha1_object_info(const unsigned char *sha1, unsigned long *sizep) · line 2647

Quoted 3 lines · Karthik Nayak

sha1_file.c +2 -0

+	unsigned long mapsize;
...
+	map = map_sha1_file(sha1, &mapsize);

I know this is a pre-existing issue, but I'm not sure "unsigned long" is the right type here. Shouldn't it be a size_t?

I got the type from the function definition of map_sha1_file(), which is "void *map_sha1_file(const unsigned char *sha1, unsigned long *size)"

Quoted 10 lines

sha1_file.c int sha1_object_info(const unsigned char *sha1, unsigned long *sizep) · line 2654

Quoted 8 lines

sha1_file.c +7 -0

+	if (!map)
+		return -1;
+	if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
+		status = error("unable to unpack %s header",
+			       sha1_to_hex(sha1));
+
+	for (i = 0; i < 32; i++) {

This number should probably be a constant.

Do you want me to define it as a preprocessor directive?

Quoted 9 lines

sha1_file.c int sha1_object_info(const unsigned char *sha1, unsigned long *sizep) · line 2660

Quoted 6 lines

sha1_file.c +6 -0

+		if (hdr[i] == ' ') {
+			type[i] = '\0';
+			break;
+		}
+		type[i] = hdr[i];
+	}

type might end up without a trailing \0 here in the case where hdr has no space in it. Is this possible?

What's possible is when the object type name is greater than 32bytes "hdr" will not be able to hold the whole type, its a tradeoff, I guess I should put a null terminator at the end of "hdr". What do you suggest?

sha1_file: Add sha1_object_type_literally and export it.

sha1_file.c +4 -0

+{
+       int status = 0;
+       unsigned long mapsize;
+       void *map;

sha1_file.c +4 -1

+
+       return status;
+}
+
 static void *read_packed_sha1(const unsigned char *sha1,
                              enum object_type *type, unsigned long *size)
 {
--

2.3.1.129.g11acff1.dirty

I had written a longer review but was interrupted for a several hours, and upon returning found that David and Junio covered many of the same issues or overrode comments I was making, so the below review is pared down quite a bit. Junio's proposed approach negates all of the below review comments, but they may still be meaningful if kept in mind for future submissions.

Quoted 1 line · Karthik Nayak

sha1_file: Add sha1_object_type_literally and export it.

Style: downcase "Add"; drop terminating period.

sha1_file.c int sha1_object_info(const unsigned char *sha1, unsigned long *sizep) · line 2638

Quoted 13 lines

sha1_object_type_literally takes a sha value and
gives the type of the given loose object, used by
git cat-file -t --literally.

Signed-off-by Karthik Nayak <[email protected]>

---

sha1_file.c +1 -0

@@ -2635,6 +2635,33 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
        return type;
 }
 
+int sha1_object_type_literally(const unsigned char *sha1, char *type)

This functionality is very specific to the --literally option you're adding to cat-file, so it would make more sense to make it private to builtin/cat-file.c rather than publishing it globally.

Also, this is an unsafe contract. The caller does not know how many bytes to allocate for 'type', and this new function may write past the end of the buffer. It is more common to also pass in the size of the 'type' buffer and ensure that you do not write beyond that. Or, if this is intended for wider consumption, pass in a strbuf instead.

Quoted 4 lines

sha1_file.c +4 -0

+{
+       int status = 0;
+       unsigned long mapsize;
+       void *map;

sha1_file.c int sha1_object_info(const unsigned char *sha1, unsigned long *sizep) · line 2652

Quoted 10 lines

sha1_file.c +10 -0

+       git_zstream stream;
+       char hdr[32];
+       int i;
+
+       map = map_sha1_file(sha1, &mapsize);
+       if (!map)
+               return -1;
+       if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
+               status = error("unable to unpack %s header",
+                              sha1_to_hex(sha1));

Since 'hdr' unpacking failed, shouldn't you be returning at this point rather than continuing to the 'hdr' processing loop?

sha1_file.c int sha1_object_info(const unsigned char *sha1, unsigned long *sizep) · line 2660

Quoted 7 lines

sha1_file.c +7 -0

+       for (i = 0; i < 32; i++) {
+               if (hdr[i] == ' ') {
+                       type[i] = '\0';
+                       break;
+               }
+               type[i] = hdr[i];
+       }

David already mentioned that this loop is suspect. Perhaps take a look at, sha1_file.c:parse_sha1_header() for an example of cleaner logic.

Quoted 9 lines

sha1_file.c +4 -1

+
+       return status;
+}
+
 static void *read_packed_sha1(const unsigned char *sha1,
                              enum object_type *type, unsigned long *size)
 {
--

2.3.1.129.g11acff1.dirty

Looking at how we collect information on normal objects, it may make more sense to model this after sha1_loose_object_info(), with a tweak to struct object_info datatype, and integrate it into sha1_object_info_extended() may make more sense, perhaps along the lines of the attached patch.

The new helper would mimick what sha1_loose_object_info() is doing, in that it may be used to learn on-disk size, object size, typename string (returned in oi->typename strbuf that is optional). There is no sensible value to stuff in oi->typep if the incoming object name refers to the experimental invalid object, so perhaps you will store OBJ_NONE or something there and the "cat-file --literally" would use the oi->typename to learn the name of the "type".

cat-file: add --literally option.

Add --literally option which when used with -t option gives the type of the object given, irrelevant of the type and its contents.

builtin/cat-file.c +6 -0

+
+       printf("%s\n", type);
+
+       return retval;
+}
+
 struct expand_data {
        unsigned char sha1[20];
        enum object_type type;

builtin/cat-file.c +5 -1

                usage_with_options(cat_file_usage, options);
 
        argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);

@@ -405,5 +431,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
        if (batch.enabled)
                return batch_objects(&batch);
 
+       if (literally && opt == 't')
+               return cat_one_file_literally(opt, exp_type, obj_name);
+       else if (literally)
+               usage_with_options(cat_file_usage, options);
+
        return cat_one_file(opt, exp_type, obj_name);
 }
--

2.3.1.129.g11acff1.dirty