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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2015, 23:45 UTC
Message-ID: <xmqq61af100p.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1425579560-18898-1-git-send-email-karthik.188@gmail.com>

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

Show 16 quoted lines

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