Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html