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

Eric Sunshine [off-list ref] writes:

quoted

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

quoted

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