[PATCH v3 0/3] cat-file: add "--literally" option

Third version of the patch submitted to add "-literlly" option
to "cat-file"
http://article.gmane.org/gmane.comp.version-control.git/264383

Thanks to Eric, Junio and David for suggesting changes on my
first version.

Thanks to Junio for suggestions on my second version.

Changes from previous version :

* Made sure no end-user printing is taking place in
   sha1_file.c, now the printing takes place in cat-file.c
* Did not credit Junio on whose work i based my patch,
   Propoer credits given in the commit message in v3.

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 <redacted>
---
 cache.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cache.h b/cache.h
index 4d02efc..949ef4c 100644
--- a/cache.h
+++ b/cache.h

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

Add "sha1_object_info_literally()" which is to be used when
the "literally" option is given to get the type of object
and return it. It internally uses "sha1_object_info_extended()".

Add "unpack_sha1_header_literally()" to unpack sha headers
which may be greater than 32 bytes, which is the threshold
for a regular object header. This code is borrowed from the
suggestions given by Junio C Hamano, it has been tested by me.

Modify "sha1_loose_object_info()" to include a flag argument
and also include changes to call "unpack_sha1_header_literally()"
when the literally flag is passed. Also copies the obtained
type to the typename field of object_info.

Modify "sha1_object_info_extended()" to call the function
"sha1_loose_object_info()" with flags.

Signed-off-by: Karthik Nayak <redacted>
---
 sha1_file.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 7 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 69a60ec..b9e3922 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -1564,6 +1564,36 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma
 	return git_inflate(stream, 0);
 }
 
+static int unpack_sha1_header_literally(git_zstream *stream, unsigned char *map,
+					unsigned long mapsize,
+					struct strbuf *header)
+{
+	unsigned char buffer[32], *cp;
+	unsigned long bufsiz = sizeof(buffer);
+	int status;
+
+	/* Get the data stream */
+	memset(stream, 0, sizeof(*stream));
+	stream->next_in = map;
+	stream->avail_in = mapsize;
+	stream->next_out = buffer;
+	stream->avail_out = bufsiz;
+
+	git_inflate_init(stream);
+
+	do {
+		status = git_inflate(stream, 0);
+		strbuf_add(header, buffer, stream->next_out - buffer);
+		for (cp = buffer; cp < stream->next_out; cp++)
+			if (!*cp)
+				/* Found the NUL at the end of the header */
+				return 0;
+		stream->next_out = buffer;
+		stream->avail_out = bufsiz;
+	} while (status == Z_OK);
+	return -1;
+}
+
 static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1)
 {
 	int bytes = strlen(buffer) + 1;

@@ -2524,13 +2554,16 @@ struct packed_git *find_sha1_pack(const unsigned char *sha1,
 }
 
 static int sha1_loose_object_info(const unsigned char *sha1,
-				  struct object_info *oi)
+				  struct object_info *oi,
+				  int flags)
 {
-	int status;
+	int status = 0;
 	unsigned long mapsize, size;
 	void *map;
 	git_zstream stream;
 	char hdr[32];
+	struct strbuf hdrbuf = STRBUF_INIT;
+	char *hdrp;
 
 	if (oi->delta_base_sha1)
 		hashclr(oi->delta_base_sha1);

@@ -2557,10 +2590,25 @@ static int sha1_loose_object_info(const unsigned char *sha1,
 		return -1;
 	if (oi->disk_sizep)
 		*oi->disk_sizep = mapsize;
-	if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
-		status = error("unable to unpack %s header",
-			       sha1_to_hex(sha1));
-	else if ((status = parse_sha1_header(hdr, &size)) < 0)
+	if ((flags & LOOKUP_LITERALLY)) {
+		if (unpack_sha1_header_literally(&stream, map, mapsize, &hdrbuf) < 0)
+			status = error("unable to unpack %s header with --literally",
+				       sha1_to_hex(sha1));
+		hdrp = hdrbuf.buf;
+	} else {
+		if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0) {
+			status = error("unable to unpack %s header",
+				       sha1_to_hex(sha1));
+		}
+		hdrp = hdr;
+	}
+	if (status)
+		; /* We're already checking for errors */
+	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;

@@ -2568,6 +2616,10 @@ static int sha1_loose_object_info(const unsigned char *sha1,
 	munmap(map, mapsize);
 	if (oi->typep)
 		*oi->typep = status;
+	if (oi->typename && 0 <= status && typename(status))
+		strbuf_addstr(oi->typename, typename(status));
+	if (hdrp == hdrbuf.buf)
+		strbuf_release(&hdrbuf);
 	return 0;
 }
 

@@ -2594,7 +2646,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
 
 	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;
 		}

@@ -2635,6 +2687,23 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 	return type;
 }
 
+const char *sha1_object_info_literally(const unsigned char *sha1)
+{
+	enum object_type type;
+	struct strbuf sb = STRBUF_INIT;
+	struct object_info oi = {NULL};
+
+	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;
+}
+
 static void *read_packed_sha1(const unsigned char *sha1,
 			      enum object_type *type, unsigned long *size)
 {

-- 
2.3.1.167.g7f4ba4b.dirty

made changes to "cat-file" to include a "--literally"
option which prints the type of the object without any
complaints.

Signed-off-by: Karthik Nayak <redacted>
---
 builtin/cat-file.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index df99df4..60b9ec4 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c

@@ -9,7 +9,8 @@
 #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;

@@ -23,6 +24,14 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 	buf = NULL;
 	switch (opt) {
 	case 't':
+		if (literally) {
+			buf = sha1_object_info_literally(sha1);
+			if (!buf)
+				die("git cat-file --literally -t %s: failed",
+					obj_name);
+			printf("%s\n", buf);
+			return 0;
+		}
 		type = sha1_object_info(sha1, NULL);
 		if (type > 0) {
 			printf("%s\n", typename(type));

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

@@ -359,6 +368,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	int opt = 0;
 	const char *exp_type = NULL, *obj_name = NULL;
 	struct batch_options batch = {0};
+	int literally = 0;
 
 	const struct option options[] = {
 		OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),

@@ -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")),
 		{ 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)
 		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);
 
-	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);
+
+	return cat_one_file(opt, exp_type, obj_name, literally);
 }

-- 
2.3.1.167.g7f4ba4b.dirty

On Thu, Mar 5, 2015 at 1:18 PM, Karthik Nayak [off-list ref] wrote:

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'

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.

On Thu, Mar 5, 2015 at 1:19 PM, Karthik Nayak [off-list ref] wrote:

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

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

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.

This might read more naturally as:

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

rather than repeating the -t option.

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.

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

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

+
+       return cat_one_file(opt, exp_type, obj_name, literally);
 }
--
2.3.1.167.g7f4ba4b.dirty

On 03/09/2015 04:20 AM, Eric Sunshine wrote:

On Thu, Mar 5, 2015 at 1:19 PM, Karthik Nayak [off-list ref] wrote:

quoted

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

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

quoted

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.

quoted

Signed-off-by: Karthik Nayak <redacted>
---

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index df99df4..60b9ec4 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c

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

quoted

         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.

quoted

                 { 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

                 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);

-       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

+
+       return cat_one_file(opt, exp_type, obj_name, literally);
  }
--
2.3.1.167.g7f4ba4b.dirty

Thanks for the feedback.
Will fix everything you stated in the next patch.

On 03/09/2015 03:55 AM, Eric Sunshine wrote:

On Thu, Mar 5, 2015 at 1:18 PM, Karthik Nayak [off-list ref] wrote:

quoted

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

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

Signed-off-by: Karthik Nayak <redacted>
---

diff --git a/cache.h b/cache.h
index 4d02efc..949ef4c 100644
--- a/cache.h
+++ b/cache.h

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

Hey Eric!
Thanks for the feedback, I guess I need to stick to different patches 
only for logical changes, not considering different files.
Have been reading old commit messages to get a hang of it. Also found 
some blog posts explaining why we should use imperative sentences while 
writing Git commit messages. All makes sense now.

Regards
-Karthik