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

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

[karthik nayak]

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

[PATCH v2 1/3] cache: modify for "cat-file --literally -t"

[Karthik Nayak]

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 <karthik.188@gmail.com>
---
 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.169.ga243085.dirty

[PATCH v2 2/3] sha1_file: implement changes for "cat-file --literally -t"

[Karthik Nayak]

add "sha1_object_info_literally()" which is to be used when
the "literally" option is given to get the type of object
and print it, using "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.

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 <karthik.188@gmail.com>
---
 sha1_file.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 77 insertions(+), 7 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 69a60ec..1068ca0 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,24 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 	return type;
 }
 
+int 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 -1;
+	if (*oi.typep > 0)
+		printf("%s\n", typename(*oi.typep));
+	else
+		printf("%s\n", oi.typename->buf);
+	strbuf_release(oi.typename);
+	return 0;
+}
+
 static void *read_packed_sha1(const unsigned char *sha1,
 			      enum object_type *type, unsigned long *size)
 {
-- 
2.3.1.169.ga243085.dirty

Re: [PATCH v2 2/3] sha1_file: implement changes for "cat-file --literally -t"

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> add "sha1_object_info_literally()" which is to be used when
> the "literally" option is given to get the type of object
> and print it, using "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.
>
> 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 <karthik.188@gmail.com>
> ---
>  sha1_file.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 77 insertions(+), 7 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 69a60ec..1068ca0 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)
> +{
> +...
> +}

Looks suspiciously familiar...

> +int 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 -1;
> +	if (*oi.typep > 0)
> +		printf("%s\n", typename(*oi.typep));
> +	else
> +		printf("%s\n", oi.typename->buf);
> +	strbuf_release(oi.typename);
> +	return 0;
> +}
> +

NAK.

Please don't add end-user facing final output to sha1_file.c;
instead have the caller use a helper function like this one to
extract necessary information and perform the end-user interaction
there.

Re: [PATCH v2 2/3] sha1_file: implement changes for "cat-file --literally -t"

[karthik nayak]

On 03/05/2015 02:28 AM, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > add "sha1_object_info_literally()" which is to be used when
> > the "literally" option is given to get the type of object
> > and print it, using "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.
> >
> > 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 <karthik.188@gmail.com>
> > ---
> >   sha1_file.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> >   1 file changed, 77 insertions(+), 7 deletions(-)
> >
> > diff --git a/sha1_file.c b/sha1_file.c
> > index 69a60ec..1068ca0 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)
> > +{
> > +...
> > +}
>
> Looks suspiciously familiar...
Yes, you suggested it.
It has a similar structure to unpack_sha1_header().
Anything wrong with it?
>
> > +int 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 -1;
> > +    if (*oi.typep > 0)
> > +        printf("%s\n", typename(*oi.typep));
> > +    else
> > +        printf("%s\n", oi.typename->buf);
> > +    strbuf_release(oi.typename);
> > +    return 0;
> > +}
> > +
>
> NAK.
>
> Please don't add end-user facing final output to sha1_file.c;
> instead have the caller use a helper function like this one to
> extract necessary information and perform the end-user interaction
> there.
>
Ok, will do that.

Re: [PATCH v2 2/3] sha1_file: implement changes for "cat-file --literally -t"

[Junio C Hamano]

karthik nayak <karthik.188@gmail.com> writes:

>> Looks suspiciously familiar...
> Yes, you suggested it.
> It has a similar structure to unpack_sha1_header().
> Anything wrong with it?

I don't know if there is something wrong with the code, or not, but
it wasn't mentioned in the log message at all that it is not your
code, and I was mostly reacting to that.

It is fairly common around here to take somebody else's code and run
with it, but people say things like "This is based on suggestion
from X", "This function was stolen from Y", etc. and then conclude
with "but I adjusted it to match the caller I wrote, so bugs are
mine." when they do so.

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

[Karthik Nayak]

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 <karthik.188@gmail.com>
---
 builtin/cat-file.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index df99df4..2af9f40 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,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 	buf = NULL;
 	switch (opt) {
 	case 't':
+		if (literally)
+			return sha1_object_info_literally(sha1);
 		type = sha1_object_info(sha1, NULL);
 		if (type > 0) {
 			printf("%s\n", typename(type));
@@ -323,7 +326,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 +362,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 +373,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 +386,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 +411,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.169.ga243085.dirty