On Fri, Aug 09, 2024 at 10:32:54AM -0700, Junio C Hamano wrote: > 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. Agreed. I was doing it for consistency's sake in this case, but let's rather not make the overall interface any weirder than it already is. Patrick
