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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2024, 17:32 UTC
Message-ID: <xmqq5xs9eh4p.fsf@gitster.g>
In-Reply-To: <ytv7y3jlbaru5o3ehotanqoqvjnwaomzsyj746k3hbb3htymts@m4qct6cvebiz>

Justin Tobler <jltobler@gmail.com> writes:

Show 12 quoted lines

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

Show 12 quoted lines

>>  {
>> -	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);
>>  }