cat-file: add "--literally" option

cache: modify for "cat-file --literally -t"

Add a "struct strbuf *typename" to object_info to hold the typename when the literally option is used. Add a flag to notify functions when literally is used.

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

---

cache.h +2 -1

index 4d02efc..949ef4c 100644
@@ -830,6 +830,7 @@ extern int is_ntfs_dotgit(const char *name);
 
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
+#define LOOKUP_LITERALLY 2
 extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);
 static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
 {

@@ -1296,6 +1297,7 @@ struct object_info {
        unsigned long *sizep;
        unsigned long *disk_sizep;
        unsigned char *delta_base_sha1;
+       struct strbuf *typename;
 
        /* Response */
        enum {
--

2.3.1.167.g7f4ba4b.dirty

Quoted 1 line · Karthik Nayak

cache: modify for "cat-file --literally -t"

It is desirable for the first line of the commit message to explain, as well as possible, the intent of the patch. The bulk of the commit message then elaborates. Unfortunately, this line says almost nothing. All patches modify, so writing "modify" here is not helpful and merely wastes precious horizontal real estate. A more informative summary might say something like:

cache: add object_info::typename in support of 'cat-file --literally'

Quoted 3 lines

Add a "struct strbuf *typename" to object_info to hold the typename when the literally option is used. Add a flag to notify functions when literally is used.

It's good to split up changes such that each patch comprises one logical step, however, this patch does not really do anything on its own, so having it stand-alone doesn't make much sense. It would make more sense to fold it into the patch which actually requires these changes.

Quoted 20 lines

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

---

cache.h +2 -0

index 4d02efc..949ef4c 100644
@@ -830,6 +830,7 @@ extern int is_ntfs_dotgit(const char *name);
 
  /* object replacement */
  #define LOOKUP_REPLACE_OBJECT 1
+#define LOOKUP_LITERALLY 2
  extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);
  static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
  {

@@ -1296,6 +1297,7 @@ struct object_info {
         unsigned long *sizep;
         unsigned long *disk_sizep;
         unsigned char *delta_base_sha1;
+       struct strbuf *typename;
 

Quoted 4 lines

cache.h +0 -1

         /* Response */
         enum {
--

2.3.1.167.g7f4ba4b.dirty

sha1_file.c +6 -0

+const char *sha1_object_info_literally(const unsigned char *sha1)
+{
+	enum object_type type;
+	struct strbuf sb = STRBUF_INIT;
+	struct object_info oi = {NULL};
+

Quoted 6 lines · Karthik Nayak

sha1_file.c +6 -0

+const char *sha1_object_info_literally(const unsigned char *sha1)
+{
+	enum object_type type;
+	struct strbuf sb = STRBUF_INIT;
+	struct object_info oi = {NULL};
+

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

Quoted 10 lines

sha1_file.c +10 -0

+	oi.typename = &sb;
+	oi.typep = &type;
+	if (sha1_object_info_extended(sha1, &oi, LOOKUP_LITERALLY) < 0)
+		return NULL;
+	if (*oi.typep > 0) {
+		strbuf_release(oi.typename);
+		return typename(*oi.typep);
+	}
+	return oi.typename->buf;
+}

After calling this function to ask the textual type of an object, should the caller free the result it obtains from this function?

oi.typename points at the strbuf on stack and its buf member points at an allocated piece of memory. That must be freed.

On the other hand, typename(*oi.typep) is a pointer into static piece of memory, which must never be freed.

This patch introduces this function without introducing any caller, which makes it unnecessarily harder to judge if this problem is caused by choosing a wrong calling convention, and/or if so what better calling convention can be used to correct the problem, but without looking at the caller that (presumably) will be introduced in a later patch, I suspect that the caller should supply a pointer to struct object_info, i.e. something along these lines:

     struct object_info oi = { NULL };
     struct strbuf sb = STRBUF_INIT;
     enum object_type type;

     ...

     oi.typename = &sb;
     sha1_object_info_literally(sha1, &oi);
     if (!sb.len)
         that is an error;
     else
         use sb.buf as the name;

     strbuf_release(&sb);

As sha1_object_info_extended() takes oi and fills oi.typename when it is supplied for _all_ types, not just the bogus ones, a caller of that function, including sha1_object_info_literally() and its caller, shouldn't have to worry about "is that a known one? then use typename() to convert the enum type to a string. Otherwise use the oi.typename->buf" at all, I would think.

Quoted 17 lines

... I suspect that the caller should supply a pointer to struct object_info, i.e. something along these lines:

     struct object_info oi = { NULL };
     struct strbuf sb = STRBUF_INIT;
     enum object_type type;

     ...

     oi.typename = &sb;
     sha1_object_info_literally(sha1, &oi);
     if (!sb.len)
         that is an error;
     else
         use sb.buf as the name;

     strbuf_release(&sb);

I thought I could get the calling function "cat_one_file()" to send the address to a struct strbuf. Like this ..

struct strbuf sb = STRBUF_INIT;
length = sha1_object_info_literally(sha1, &sb);
if (length < 0)
die("git cat-file --literally -t %s: failed",

            obj_name);

printf("%s\n", sb.buf);
strbuf_release(&sb);
return 0;

What do you think? Is this ok?

Quoted 24 lines · karthik nayak

Quoted 17 lines

... I suspect that the caller should supply a pointer to struct object_info, i.e. something along these lines:

      struct object_info oi = { NULL };
      struct strbuf sb = STRBUF_INIT;
      enum object_type type;

      ...

      oi.typename = &sb;
      sha1_object_info_literally(sha1, &oi);
      if (!sb.len)
          that is an error;
      else
          use sb.buf as the name;

      strbuf_release(&sb);

I thought I could get the calling function "cat_one_file()" to send the address to a struct strbuf. Like this ..

struct strbuf sb = STRBUF_INIT;
length = sha1_object_info_literally(sha1, &sb);
if (length < 0)
die("git cat-file --literally -t %s: failed",

Quoted 6 lines

             obj_name);

printf("%s\n", sb.buf);
strbuf_release(&sb);
return 0;

What do you think? Is this ok?

When I gave you $gmane/264420, I was actually hoping that we do not have to have "object-info-literally" helper at all, and instead the caller in cat-file that deals with "-t" option can become something like this:

	struct object_info oi = { NULL };
	struct strbuf typename = STRBUF_INIT;
	unsigned flags = LOOKUP_REPLACE_OBJECT;

         if (doing the --literally stuff)
		flags |= LOOKUP_LITERALLY;

	...

	switch (...) {
	case 't':
         	oi.typename = &typename;
                 sha1_object_info_extended(sha1, &oi, flags);
		if (typename.len) {
                 	printf("%s\n", typename.buf);
			return 0;
		}
                 break;
	...

The change illustrated in $gmane/264420 is probably incomplete and some calls from the sha1_object_info_extended() after that change may still need to be tweaked to pay attention to LOOKUP_LITERALLY bit (e.g. parse_sha1_header() may want to learn not to barf when seeing an unexpected typename in the header when the caller asks to look up "literally").

On 03/07/2015 12:58 AM, Junio C Hamano wrote:

case 't':

	oi.typep = &type;
	oi.typename = &sb;
	sha1_object_info_extended(sha1, &oi, flags);
	if (sb.len) {
		printf("%s\n", sb.buf);
		strbuf_release(&sb);
		return 0;
	} else if (type) {
		printf("%s\n", typename(type));
		return 0;
	}
	break;

This works but I need an else statement to check the type if not getting the type literally, which is because if not called literally the oi.typename is not set,...

Quoted 16 lines · karthik nayak

On 03/07/2015 12:58 AM, Junio C Hamano wrote:

case 't':

	oi.typep = &type;
	oi.typename = &sb;
	sha1_object_info_extended(sha1, &oi, flags);
	if (sb.len) {
		printf("%s\n", sb.buf);
		strbuf_release(&sb);
		return 0;
	} else if (type) {
		printf("%s\n", typename(type));
		return 0;
	}
	break;

Quoted 3 lines

This works but I need an else statement to check the type if not getting the type literally, which is because if not called literally the oi.typename is not set,...

Hmph, when I outlined that change to object-info-extended, I meant to do it in such a way that when the optional oi->typename is set, it is always filled whether "literally" is asked for andr whether the object is a kosher one or a bogus one.

Without parsing the header, we wouldn't know how long the object would be, so I do not know if not doing some variant of parse_header is an option.

Thanks.

What parse_sha1_header() does to get the type is just find the first occurrence of a " " manually and store everything before it as the type. Then it finds the size of the object if needed. And finally returns the type by calling type_from_string(). This is where we get the undefined type error.

Quoted 5 lines · karthik nayak

What parse_sha1_header() does to get the type is just find the first occurrence of a " " manually and store everything before it as the type. Then it finds the size of the object if needed. And finally returns the type by calling type_from_string(). This is where we get the undefined type error.

Yes, exactly. The change illustrated in $gmane/264420 may be incomplete and some calls from the sha1_object_info_extended() after that change may still need to further be tweaked to pay attention to LOOKUP_LITERALLY bit; for example, parse_sha1_header() may want to learn not to barf when seeing an unexpected typename in the header when the caller asks to look up "literally".

I thought I already said that; sorry if I forgot.

Sorry for the confusion, you did already say that in $gmane/264955 , I'm talking about how I tackled the issue in $gmane/264855.

Like :

    else if ((flags & LOOKUP_LITERALLY)) {
        size_t typelen = strcspn(hdrbuf.buf, " ");
        strbuf_add(oi->typename, hdrbuf.buf, typelen);
    }
    else if ((status = parse_sha1_header(hdrp, &size)) < 0)
        status = error("unable to parse %s header", sha1_to_hex(sha1));
    else if (oi->sizep)
        *oi->sizep = size;

This way, we don't have to modify parse_sha1_header() to worry if "literally" is set or not.

Quoted 2 lines · karthik nayak

Sorry for the confusion, you did already say that in $gmane/264955 , I'm talking about how I tackled the issue in $gmane/264855.

Well, I am suggesting how to improve what you did in your $gmane/264855 and a part of that was to suggest that teaching parse_sha1_header() the "literally" mode may be necessary and why "do not have to call parse_sha1_header()" may not be a good solution.

Unless our goal is only to support "--literally -t" and go home, never intending to support other things like "--literally -s" and "--literally flob $OBJ", that is ;-)

Let's try again.

Quoted 11 lines

Like :

     else if ((flags & LOOKUP_LITERALLY)) {
         size_t typelen = strcspn(hdrbuf.buf, " ");
         strbuf_add(oi->typename, hdrbuf.buf, typelen);
     }
     else if ((status = parse_sha1_header(hdrp, &size)) < 0)
         status = error("unable to parse %s header", sha1_to_hex(sha1));
     else if (oi->sizep)
         *oi->sizep = size;

Quoted 2 lines

This way, we don't have to modify parse_sha1_header() to worry if "literally" is set or not.

When you are dealing with a crafted object $OBJ of type "florb", how would your

     $ git cat-file --literally florb $OBJ
     $ git cat-file --literally -s $OBJ

be implemented, if parse_sha1_header() has not been enhanced to allow you to say "for this invocation, the type name in the object header may be something unknown to us, and it is OK"?

One possible implementation of "--literally florb $OBJ" would be to still call the same loose object reading codepath, which would eventually hit parse_sha1_header() to see what the type of the object is and how long the object contents is, and error out if the type is not "florb" or the length of the inflated contents does not match the expected size. Wouldn't you need a way for you to say "for this invocation, the type name in the object header may be something unknown to us, and it is OK"?

One possible implementation of "--literally -s $OBJ" would be to change the call to sha1_object_info() to instead to call the _extended() version, just like you did for "--literally -t", and then read the result from *(oi.sizep), but the quoted piece of code above would not let you use it that way, no?

Of course the above two are both "one possible implementation", and not how the implementation ought to be [*1*]. But knowing a bit of this part of the codepath, they look the most natural way to enhance these codepaths, at least to me.

[Footnote]

*1* You could for example sidestep the issue and introduce a parallel implementation of what parse_sha1_header() does, minus the validation to ensure the object type is one of the known ones, and use that function instead of the users of parse_sha1_header() in the codepaths that implement "-s" and "cat-file" itself.

But I do not think that is a good direction to go in to keep the codebase healthy in the longer term. A refactoring that is similar to what we did when we introduced sha1_object_info_extended(), which was done in 9a490590 (sha1_object_info_extended(): expose a bit more info, 2011-05-12) would be more desirable, so that we can avoid such a duplicated parallel implementations. That way, the existing callers that do not have to know about "--literally" can keep using parse_sha1_header(), but parse_sha1_header_extended() is called from the updated parse_sha1_header() behind the scene. And the extended version would let callers that need to care about "--literally" ask "the type name in the object header may be something unknown to us, and it is OK" by passing an extra flag.

made changes to "cat-file" to include a "--literally"

option which prints the type of the object without any complaints.

builtin/cat-file.c +2 -1

+
+       return cat_one_file(opt, exp_type, obj_name, literally);
 }
--

2.3.1.167.g7f4ba4b.dirty

Quoted 1 line · Karthik Nayak

made changes to "cat-file" to include a "--literally"

Write in imperative mood: "Teach cat-file a --literally option..."

Quoted 2 lines

option which prints the type of the object without any complaints.

Unfortunately, this explanation is quite lacking. What "complaints"? What problem is --literally trying to solve? To answer these questions, you will probably want to say something about the sort of object which requires --literally, and how cat-file fails or behaves without it.

builtin/cat-file.c static int batch_objects(struct batch_options *opt) · line 335

Quoted 12 lines

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

---

builtin/cat-file.c +1 -1

index df99df4..60b9ec4 100644
@@ -323,7 +332,7 @@ static int batch_objects(struct batch_options *opt)
  }
 
  static const char * const cat_file_usage[] = {
-       N_("git cat-file (-t | -s | -e | -p | <type> | --textconv) <object>"),
+       N_("git cat-file (-t|-s|-e|-p|<type>|--textconv|-t --literally) <object>"),

This might read more naturally as:

     git cat-file (-t [--literally] | -s | -e | -p | <type> |

--textconv) <object>

rather than repeating the -t option.

builtin/cat-file.c int cmd_cat_file(int argc, const char **argv, const char *prefix) · line 383

Quoted 9 lines

builtin/cat-file.c +2 -0

         N_("git cat-file (--batch | --batch-check) < <list-of-objects>"),
         NULL
  };

@@ -369,6 +379,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
                 OPT_SET_INT('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
                 OPT_SET_INT(0, "textconv", &opt,
                             N_("for blob objects, run textconv on object's content"), 'c'),
+               OPT_BOOL( 0, "literally", &literally,
+                         N_("show the type of the given loose object, use for debugging")),

Taking other help strings into account, there is no need for the long-winded "type of the given loose object" when "loose object's type" will suffice. More importantly, thought, you should try to say something about how --literally is actually useful, such as for "broken" objects or objects not of a known type.

builtin/cat-file.c int cmd_cat_file(int argc, const char **argv, const char *prefix) · line 395

Quoted 9 lines

builtin/cat-file.c +1 -1

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

@@ -380,7 +392,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 
         git_config(git_cat_file_config, NULL);
 
-       if (argc != 3 && argc != 2)
+       if (argc != 3 && argc != 2 && argc != 4)

Perhaps it's time to rephrase this as "if (argc < 2 || argc > 4)"?

Quoted 7 lines

builtin/cat-file.c +0 -0

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

@@ -405,5 +417,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
         if (batch.enabled)
                 return batch_objects(&batch);
 

builtin/cat-file.c int cmd_cat_file(int argc, const char **argv, const char *prefix) · line 423

Quoted 5 lines

builtin/cat-file.c +4 -1

-       return cat_one_file(opt, exp_type, obj_name);
+       if (literally && opt == 't')
+               return cat_one_file(opt, exp_type, obj_name, literally);
+       else if (literally)
+               usage_with_options(cat_file_usage, options);

I realize that existing cases in cat-file are already guilty of this transgression, but it is quite annoying when a program merely spits out its usage statement without actually telling you what you did wrong; and it's often difficult to figure out why it was rejected. It would be much more helpful in a case like this to state explicitly that --literally was given without -t. (But perhaps such a "friendliness" change is fodder for a separate patch.)

Quoted 5 lines

builtin/cat-file.c +2 -1

+
+       return cat_one_file(opt, exp_type, obj_name, literally);
  }
--

2.3.1.167.g7f4ba4b.dirty