cat-file: teach cat-file a '--literally' option

Update sha1_loose_object_info() to optionally allow it to read from a loose object file of unknown/bogus type; as the function usually returns the type of the object it read in the form of enum for known types, add an optional "typename" field to receive the name of the type in textual form and a flag to indicate the reading of a loose object file of unknown/bogus type.

Add parse_sha1_header_extended() which acts as a wrapper around parse_sha1_header() allowing more information to be obtained.

Add unpack_sha1_header_to_strbuf() to unpack sha1 headers of unknown/corrupt objects which have a unknown sha1 header size to a strbuf structure. This was written by Junio C Hamano but tested by me.

Helped-by Junio C Hamano <[email protected]>

Helped-by Eric Sunshine <[email protected]>

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

---

sha1_file.c +12 -9

index 980ce6b..ac8fffc 100644
@@ -1614,27 +1642,24 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s
  * too permissive for what we want to check. So do an anal
  * object header parse by hand.
  */
-int parse_sha1_header(const char *hdr, unsigned long *sizep)
+int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
+                              unsigned int flags)
 {
-       char type[10];
-       int i;
+       struct strbuf typename = STRBUF_INIT;
        unsigned long size;
+       int type;
 
        /*
         * The type can be at most ten bytes (including the
         * terminating '\0' that we add), and is followed by
         * a space.
         */
-       i = 0;
        for (;;) {
                char c = *hdr++;
                if (c == ' ')
                        break;
-               type[i++] = c;
-               if (i >= sizeof(type))
-                       return -1;
+               strbuf_addch(&typename, c);
        }
-       type[i] = 0;
 
        /*
         * The length must follow immediately, and be in canonical

@@ -1652,12 +1677,39 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)
                        size = size * 10 + c;
                }
        }
-       *sizep = size;
+
+       type = type_from_string_gently(typename.buf, typename.len, 1);
+       if (oi->sizep)
+               *oi->sizep = size;
+       if (oi->typename)
+               strbuf_addbuf(oi->typename, &typename);
+       strbuf_release(&typename);
 
        /*

sha1_file.c +12 -1

+       /*
         * The length must be followed by a zero byte
         */
-       return *hdr ? -1 : type_from_string(type);
+       return *hdr ? -1 : type;
+}
+
+int parse_sha1_header(const char *hdr, unsigned long *sizep)
+{
+       struct object_info oi;
+
+       oi.sizep = sizep;
+       oi.typename = NULL;
+       oi.typep = NULL;
+       return parse_sha1_header_extended(hdr, &oi, LOOKUP_REPLACE_OBJECT);
 }
 
 static void *unpack_sha1_file(void *map, unsigned long mapsize, enum object_type *type, unsigned long *size, const unsigned char *sha1)

sha1_file.c int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, · line 2656

Quoted 9 lines · Karthik Nayak

sha1_file.c +2 -0

@@ -2586,13 +2649,15 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
              *(oi->disk_sizep) = 0;
          if (oi->delta_base_sha1)
              hashclr(oi->delta_base_sha1);
+        if (oi->typename)
+            strbuf_addstr(oi->typename, typename(co->type));
          oi->whence = OI_CACHED;
          return 0;
      }

Just before the pre-context of this hunk, there is this bit:

    if (oi->typep)
        *(oi->typep) = co->type;

which tells me that the callers of this function is allowed to pass a NULL in oi->typep when they are not interested in the type of the object.

sha1_file.c int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, · line 2685

Quoted 13 lines

sha1_file.c +3 -1

      if (!find_pack_entry(real, &e)) {
          /* Most likely it's a loose object. */
-        if (!sha1_loose_object_info(real, oi)) {
+        if (!sha1_loose_object_info(real, oi, flags)) {
              oi->whence = OI_LOOSE;
              return 0;
          }

@@ -2616,6 +2681,8 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
          oi->u.packed.is_delta = (rtype == OBJ_REF_DELTA ||
                       rtype == OBJ_OFS_DELTA);
      }
+    if (oi->typename)
+        strbuf_addstr(oi->typename, typename(*oi->typep));

So, it makes me wonder what guarantee we have that this does not dereference a NULL here.

Quoted 3 lines

So, it makes me wonder what guarantee we have that this does not dereference a NULL here.

As per my code, oi->typename is only pointing to something when oi->typep is ( As oi->typename is currently only used in cat-file.c).
But what you're saying also is true, there is no other guarantee, as a user may set oi->typename to point to a struct strbuf and leave out oi->typep.

if (oi->typename && oi->typep)

         strbuf_addstr(oi->typename, typename(*oi->typep));

This should suffice. Do you want me to re-roll this?

Quoted 8 lines · karthik nayak

Quoted 3 lines

So, it makes me wonder what guarantee we have that this does not dereference a NULL here.

As per my code, oi->typename is only pointing to something when oi->typep is ( As oi->typename is currently only used in cat-file.c).
But what you're saying also is true, there is no other guarantee, as a user may set oi->typename to point to a struct strbuf and leave out oi->typep.

Quoted 4 lines

  if (oi->typename && oi->typep)
          strbuf_addstr(oi->typename, typename(*oi->typep));

This should suffice. Do you want me to re-roll this?

I'd rather avoid the thinking along the lines of "at this moment, there happens to be only one caller that asks for typename and the caller also sets typep, so we will be safe as long as we make sure the caller passed typep before giving him typename back".

Somebody else may write new code that wants to learn the typename, forgets to set typep, calls into this codepath, and ends up scratching his head wondering why the typename string is returned to him. Surely the code may not crash at the new code you wrote, but you are not helping him.

If it semantically does not make sense to ask for the typename without asking for the type code, then we can and should make that as a new calling convention _all_ callers must follow.

In other words, I think it is better to have

	if (oi->typename && !oi->typep)
		die("BUG");

somewhere near the beginning of the callchain that takes oi; that will ensure all callers understand the rule.

Quoted 6 lines · Junio C Hamano

If it semantically does not make sense to ask for the typename without asking for the type code, then we can and should make that as a new calling convention _all_ callers must follow.

In other words, I think it is better to have

Quoted 5 lines

	if (oi->typename && !oi->typep)
		die("BUG");

somewhere near the beginning of the callchain that takes oi; that will ensure all callers understand the rule.

Yes! this is a better approach as it will enforce that typep must be set when typename is set.

Quoted 8 lines · karthik nayak

Quoted 6 lines · Junio C Hamano

If it semantically does not make sense to ask for the typename without asking for the type code, then we can and should make that as a new calling convention _all_ callers must follow.

In other words, I think it is better to have

Quoted 8 lines

Quoted 5 lines

    if (oi->typename && !oi->typep)
        die("BUG");

somewhere near the beginning of the callchain that takes oi; that will ensure all callers understand the rule.

Yes! this is a better approach as it will enforce that typep must be set when typename is set.

Not so fast ;-)

The key phrase in what I wrote above is "If it does not make sense", and I am not yet convinced if that is the case or not. If we are to change the calling convention for the callers, the reason why it does not make sense to ask only for typename but not typep must be explained in the log message.

And if it turns out that the answer to that question is "it is valid to ask only for typename", then it would be wrong to force everybody who wants to learn the stringified typename to supply typep. And in such a case it might be better to do something like this instead (on top of your patch I am responding to):

  sha1_file.c | 12 ++++++++++++
  1 file changed, 12 insertions(+)

sha1_file.c +10 -0

index f4055dd..26fbb7c 100644
@@ -2639,6 +2639,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
      struct cached_object *co;
      struct pack_entry e;
      int rtype;
+    enum object_type real_type;
      const unsigned char *real = lookup_replace_object_extended(sha1, flags);
 
      co = find_cached_object(real);

@@ -2670,9 +2671,18 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
              return -1;
      }
 
+    /*
+     * packed_object_info() does not follow the delta chain to
+     * find out the real type, unless it is given oi->typep.
+     */
+    if (oi->typename && !oi->typep)
+        oi->typep = &real_type;
+
      rtype = packed_object_info(e.p, e.offset, oi);
      if (rtype < 0) {
          mark_bad_packed_object(e.p, real);
+        if (oi->typep == &real_type)
+            oi->typep = NULL;
          return sha1_object_info_extended(real, oi, 0);
      } else if (in_delta_base_cache(e.p, e.offset)) {
          oi->whence = OI_DBCACHED;

Currently 'git cat-file' throws an error while trying to print the type or size of a broken/corrupt object which is created using 'git hash-object --literally'. This is because these objects are usually of unknown types.

Teach git cat-file a '--literally' option where it prints the type or size of a broken/corrupt object without throwing an error.

Modify '-t' and '-s' options to call sha1_object_info_extended() directly to support the '--literally' option.

Helped-by Junio C Hamano <[email protected]>

Helped-by Eric Sunshine <[email protected]>

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

---

builtin/cat-file.c +8 -1

index df99df4..91ceae0 100644
@@ -9,13 +9,20 @@
 #include "userdiff.h"
 #include "streaming.h"
 
-static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
+static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
+                       int literally)
 {
        unsigned char sha1[20];
        enum object_type type;
        char *buf;
        unsigned long size;
        struct object_context obj_context;
+       struct object_info oi = {NULL};
+       struct strbuf sb = STRBUF_INIT;
+       unsigned flags = LOOKUP_REPLACE_OBJECT;
+
+       if (literally)
+               flags |= LOOKUP_LITERALLY;
 
        if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
                die("Not a valid object name %s", obj_name);

builtin/cat-file.c +0 -0

                { OPTION_CALLBACK, 0, "batch", &batch, "format",
                        N_("show info and content of objects fed from the standard input"),
                        PARSE_OPT_OPTARG, batch_option_callback },

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

---

Documentation/git-cat-file.txt +1 -1

index f6a16f4..8bac7bd 100644
@@ -9,7 +9,7 @@ git-cat-file - Provide content or type and size information for repository objec
 SYNOPSIS
 --------
 [verse]
-'git cat-file' (-t | -s | -e | -p | <type> | --textconv ) <object>
+'git cat-file' (-t [--literally]| -s [--literally]| -e | -p | <type> | --textconv ) <object>
 'git cat-file' (--batch | --batch-check) < <list-of-objects>
 
 DESCRIPTION

Documentation/git-cat-file.txt +0 -1

 OUTPUT
 ------
 If '-t' is specified, one of the <type>.
--

2.4.0.rc1.249.g9f2ee54

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

---

t/t1006-cat-file.sh +13 -0

index ab36b1e..5b74044 100755
@@ -47,6 +47,18 @@ $content"
        test_cmp expect actual
     '
 
+    test_expect_success "Type of $type is correct using --literally" '
+       echo $type >expect &&
+       git cat-file -t --literally $sha1 >actual &&
+       test_cmp expect actual
+    '
+
+    test_expect_success "Size of $type is correct using --literally" '
+       echo $size >expect &&
+       git cat-file -s --literally $sha1 >actual &&
+       test_cmp expect actual
+    '
+
     test -z "$content" ||
     test_expect_success "Content of $type is correct" '
        maybe_remove_timestamp "$content" $no_ts >expect &&

@@ -296,4 +308,19 @@ test_expect_success '%(deltabase) reports packed delta bases' '
        }
 '
 
+bogus_type="bogus"

t/t1006-cat-file.sh +4 -1

+       git cat-file -s --literally $bogus_sha1 >actual &&
+       test_cmp expect actual
+'
+
 test_done
--

2.4.0.rc1.249.g9f2ee54

Quoted 38 lines · Karthik Nayak

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

---

t/t1006-cat-file.sh +21 -0

index ab36b1e..5b74044 100755
@@ -47,6 +47,18 @@ $content"
         test_cmp expect actual
      '
 
+    test_expect_success "Type of $type is correct using --literally" '
+       echo $type >expect &&
+       git cat-file -t --literally $sha1 >actual &&
+       test_cmp expect actual
+    '
+
+    test_expect_success "Size of $type is correct using --literally" '
+       echo $size >expect &&
+       git cat-file -s --literally $sha1 >actual &&
+       test_cmp expect actual
+    '
+
      test -z "$content" ||
      test_expect_success "Content of $type is correct" '
         maybe_remove_timestamp "$content" $no_ts >expect &&

@@ -296,4 +308,19 @@ test_expect_success '%(deltabase) reports packed delta bases' '
         }
  '
 
+bogus_type="bogus"
+bogus_sha1=$(git hash-object -t $bogus_type --literally -w --stdin </dev/null)
+
+test_expect_success "Type of broken object is correct" '
+       echo $bogus_type >expect &&
+       git cat-file -t --literally $bogus_sha1 >actual &&
+       test_cmp expect actual
+'
+

t/t1006-cat-file.sh test_expect_success '%(deltabase) reports packed delta bases' ' · line 321

Quoted 2 lines

t/t1006-cat-file.sh +2 -0

+test_expect_success "Size of broken object is correct" '
+       echo "0" >expect &&

Zero is such a common fallback value when things go wrong that it may not be the best choice for this test. Consequently, it might be better to create the bogus object with non-zero length. Take a look at how 'hello_length' and 'hello_sha1' are computed elsewhere in this script for inspiration.

Quoted 8 lines

t/t1006-cat-file.sh +4 -1

+       git cat-file -s --literally $bogus_sha1 >actual &&
+       test_cmp expect actual
+'
+
  test_done
--

2.4.0.rc1.249.g9f2ee54

Quoted 23 lines · Eric Sunshine

Quoted 21 lines · Karthik Nayak

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

---

t/t1006-cat-file.sh +0 -0

index ab36b1e..5b74044 100755
@@ -296,4 +308,19 @@ test_expect_success '%(deltabase) reports packed

delta bases' '

         }

'

t/t1006-cat-file.sh +2 -0

+bogus_type="bogus"
+bogus_sha1=$(git hash-object -t $bogus_type --literally -w --stdin

</dev/null)

t/t1006-cat-file.sh +7 -0

+
+test_expect_success "Type of broken object is correct" '
+       echo $bogus_type >expect &&
+       git cat-file -t --literally $bogus_sha1 >actual &&
+       test_cmp expect actual
+'
+

Quoted 8 lines

t/t1006-cat-file.sh test_expect_success '%(deltabase) reports packed delta bases' ' · line 321

Quoted 2 lines

t/t1006-cat-file.sh +2 -0

+test_expect_success "Size of broken object is correct" '
+       echo "0" >expect &&

Zero is such a common fallback value when things go wrong that it may not be the best choice for this test. Consequently, it might be better to create the bogus object with non-zero length. Take a look at how 'hello_length' and 'hello_sha1' are computed elsewhere in this script for inspiration.

The first part of this patch contains tests which make use of 'hello_length' adn 'hello_sha1', but I get what you're saying, will look into it. Thanks.

Quoted 2 lines · karthik nayak

Quoted 1 line · Eric Sunshine

On Sat, Apr 4, 2015 at 1:44 AM, Karthik Nayak

<[email protected]>

Quoted 8 lines

Quoted 8 lines

t/t1006-cat-file.sh test_expect_success '%(deltabase) reports

Quoted 7 lines · wrote:

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

---

t/t1006-cat-file.sh +0 -0

index ab36b1e..5b74044 100755
@@ -296,4 +308,19 @@ test_expect_success '%(deltabase) reports

packed

Quoted 6 lines

Quoted 6 lines

t/t1006-cat-file.sh

Quoted 6 lines

delta bases' '

         }

'

t/t1006-cat-file.sh +2 -0

+bogus_type="bogus"
+bogus_sha1=$(git hash-object -t $bogus_type --literally -w

--stdin

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

</dev/null)

t/t1006-cat-file.sh +3 -0

+
+test_expect_success "Type of broken object is correct" '
+       echo $bogus_type >expect &&

Quoted 8 lines

Quoted 8 lines

t/t1006-cat-file.sh test_expect_success '%(deltabase) reports packed delta bases' ' · line 321

Quoted 6 lines

t/t1006-cat-file.sh +6 -0

+       git cat-file -t --literally $bogus_sha1 >actual &&
+       test_cmp expect actual
+'
+
+test_expect_success "Size of broken object is correct" '
+       echo "0" >expect &&

Zero is such a common fallback value when things go wrong that it

may

Quoted 1 line

Quoted 1 line

not be the best choice for this test. Consequently, it might be

better

Quoted 2 lines

Quoted 2 lines

to create the bogus object with non-zero length. Take a look at how 'hello_length' and 'hello_sha1' are computed elsewhere in this

script

Quoted 3 lines

Quoted 1 line

for inspiration.

The first part of this patch contains tests which make use of

'hello_length'

Quoted 1 line

adn 'hello_sha1', but I get what you're saying, will look into it.

Thanks.

Just to be sure we're on the same page, I wasn't suggesting re-using 'hello_size' and 'hello_sha1', but merely to use that as an example of how to avoid hard-coding the length of your non-zero-length bogus object. So, something like this, perhaps:

   bogus_content='bogus'
   bogus_size=$(strlen "$bogus_content")
   bogus_sha1=$(echo_without_newline "$bogus_content" |
       git hash-object -t $bogus_type --literally -w --stdin)
   ...
   test_expect_success "Size of broken object is correct" '
       echo $bogus_size >expect &&
       ...