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

[Junio C Hamano <gitster@pobox.com>]

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.