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

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

[karthik nayak]

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.

[PATCH v3 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.167.g7f4ba4b.dirty

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

[Eric Sunshine]

On Thu, Mar 5, 2015 at 1:18 PM, Karthik Nayak <karthik.188@gmail.com> 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.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> 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

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

[karthik nayak]

On 03/09/2015 03:55 AM, Eric Sunshine wrote:
> On Thu, Mar 5, 2015 at 1:18 PM, Karthik Nayak <karthik.188@gmail.com> 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.
>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> 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

[PATCH v3 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 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 <karthik.188@gmail.com>
---
 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

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

[Junio C Hamano]

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

> +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;
> +}

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.

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

[karthik nayak]

On 03/06/2015 05:15 AM, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +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;
>> +}
>
> 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);
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?
>
> 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.
>
>
>
 >
I also missed a part where the object given was a packed object.
eg : git cat-file -t --literally HEAD~2
And since I missed that out, it wasnt copying the type to oi.typename,
and oi.typename would end up being empty, I found this while i was
using gdb.

Didn't CC the mailing list the first time, sorry.

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

[Junio C Hamano]

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

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

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

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

[karthik nayak]

On 03/07/2015 12:58 AM, Junio C Hamano wrote:
> karthik nayak <karthik.188@gmail.com> writes:
>
>>> ... 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?
>
> 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").
>
I got confused with $gmane/264420 thanks for clearing that up, also I
tried implementing it as follows  :

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, which I will fix now.
Also when trying to get the type "literally" it does not call 
parse_sha1_header() hence we don't need to worry about it handling 
unexpected typenames.

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

[Junio C Hamano]

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

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

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.

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

[karthik nayak]

On 03/08/2015 01:39 PM, Junio C Hamano wrote:
> karthik nayak <karthik.188@gmail.com> writes:
>
>> 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,...
>
> 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.
When getting the type "literally" we could just find the first 
occurrence of a " " using strcspn() copy the type to oi->typename and 
avoid calling parse_sha1_header(). Even your code went along these lines.
If "literally" is not set we could call parse_sha1_header() like we 
regularly would.

Thanks
Karthik

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

[Junio C Hamano]

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

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

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

[karthik nayak]

On 03/08/2015 02:33 PM, Junio C Hamano wrote:
> karthik nayak <karthik.188@gmail.com> writes:
>
> > 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.

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

[Junio C Hamano]

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

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

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

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.

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

[karthik nayak]

On 03/09/2015 12:39 AM, Junio C Hamano wrote:
> karthik nayak <karthik.188@gmail.com> writes:
>
>> 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.
>
>> 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.
>
> 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.
>

Hey Junio!
You are indeed right! I was only thinking about "--literally -t" and 
totally ignored the possibility of a future implementation of 
"--literally -s" and maybe a "--literally -e" and a "--literally -p". 
Now I understand why you were saying i need to change
parse_sha1_header().
I will have to look into this, will get back to you when I come up with 
something tangible.
Thanks for the detailed explanation and possible ways of coming across 
this problem.
Regards
-Karthik

[PATCH v3 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 | 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

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

[Eric Sunshine]

On Thu, Mar 5, 2015 at 1:19 PM, Karthik Nayak <karthik.188@gmail.com> 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.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> 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.

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

>                 { 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)"?

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

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

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

[karthik nayak]

On 03/09/2015 04:20 AM, Eric Sunshine wrote:
> On Thu, Mar 5, 2015 at 1:19 PM, Karthik Nayak <karthik.188@gmail.com> 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.
>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> 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.
>
>>          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.
>
>>                  { 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)"?
>
>>                  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.)
>
>> +
>> +       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.