Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type

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

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +extern int hash_sha1_file_literally(struct strbuf *buf, const char
>> *type, unsigned char *return_sha1, unsigned flags);
>
> A few questions:
>
> What's the value of making the first argument of
> hash_sha1_file_literally() a strbuf rather than the two-argument
> buf & len accepted by hash_sha1_file() and write_sha1_file()? Is
> the inconsistency warranted?

I do not care either way, as this is really meant to be a
single-purpose wrapper for a single caller.

> Would it make sense to name the third argument "sha1" instead of
> "return_sha1" to match the argument name of hash_sha1_file()?
>
> And, as an aside, should your new patch 4/4 rename "return_sha1" to
> "sha1" in the write_sha1_file() prototype also?

Because most of the read-cache.c functions take object name and do
something about it, but these small number of functions compute and
return object name, I actually think it is a good way to keep
"return" in the name to remind those who are writing or reading
callers.

>> +       /* type string, SP, %lu of the length plus NUL must fit this */
>> +       strbuf_grow(&header, strlen(type) + 20);
>
> A couple comments:
>
> First, given that the largest 64-bit unsigned long value
> (18,446,744,073,709,551,615) is 20 characters, do we want to be really
> pedantic and add 22 instead of 20?

32 is fine ;-)

> Second, is strbuf overkill in this situation when a simple
> xmalloc()/free() would do?

I think underneath the number of xmalloc()/free() calls are the
same.  The code that uses strbuf abstraction is easier to read, I
would think.