Justin Tobler <jltobler@xxxxxxxxx> writes: >> -static void do_git_common_path(const struct repository *repo, >> - struct strbuf *buf, >> - const char *fmt, >> - va_list args) >> +void strbuf_git_common_pathv(struct strbuf *sb, >> + const struct repository *repo, >> + const char *fmt, >> + va_list args) > > Here we reorder the arguments to make `strbuf` first. I assume we are do > this to align with the preexisting `strbuf_git_common_path()` and use > the "strbuf_" prefix in the function name. I thought that we already established as a general guideline that "strbuf_" should be cleaned up so that functions that happen to use strbuf merely as a way to carry parameters into or results out of them but are not primarily about string manipulation are renamed out of the "strbuf_" namespace. https://lore.kernel.org/git/ZqiLA0bGYZfH1OWD@tanuki/ And this is about getting a path, which is communicated via a "struct strbuf", and not the standard "char *". That is a prime example of a function that we do *not* want to stress strbuf-ness of the function. > In the previous commit we used the "repo_" prefix for > `repo_git_pathv()`. Would it make sense to be consistent here? All these > functions are operating on the provided buffer, but for a given > repository. Not sure what would be most appropriate here. Yes, if the function is about obtaining the path for a file in a given repository's metadata directory, and its association with "strbuf" is that it merely happens to use it instead of "char *", it should not be named as if "strbuf_" ness is the primary characteristics of the function. strbuf_cleanup_path() should also be renamed for the same reason. >> { >> - strbuf_addstr(buf, repo->commondir); >> - if (buf->len && !is_dir_sep(buf->buf[buf->len - 1])) >> - strbuf_addch(buf, '/'); >> - strbuf_vaddf(buf, fmt, args); >> - strbuf_cleanup_path(buf); >> + strbuf_addstr(sb, repo->commondir); >> + if (sb->len && !is_dir_sep(sb->buf[sb->len - 1])) >> + strbuf_addch(sb, '/'); >> + strbuf_vaddf(sb, fmt, args); >> + strbuf_cleanup_path(sb); >> }
