Re: [PATCH 02/20] path: expose `do_git_common_path()` as `strbuf_git_common_pathv()`

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

Justin Tobler <jltobler@gmail.com> 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);
>>  }