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

[karthik nayak <karthik.188@gmail.com>]

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.