Re: [PATCH 0/6] strbuf cleanups

From: Junio C Hamano <gitster@pobox.com>
Date: May 2, 2023, 22:20 UTC
Message-ID: <xmqqttwuxtnz.fsf@gitster.g>
In-Reply-To: <20230502211454.1673000-1-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

Show 8 quoted lines

> Strbuf is a basic structure that should function as a low-level library
> due to how generic it is. Over time certain functions inside of
> strbuf.[ch] have been added with dependencies to higher level objects
> and functions. This series cleans up some of those higher level
> dependencies by moving the offending functions to the files they
> interact with. With the goal of eventually being able to stand up strbuf
> as a libary, this series also removes the use of environment variables
> from strbuf.

I touched a bit of the same in my review for 1/6, but the main thrust of the series is that you want to make strbuf.[ch] mostly about "string processing primitives".

Any meaningful features relate to more than one conceptual things, e.g. pathname functions work on "paths" (that is one) and it may store computed result in a "strbuf" (that is another). We have historically called them strbuf_do_something() and gave the pointer to a strbuf that receives the result. And this series wants to reverse the course and want to see strbuf that is more pure.

That's fine. The strbuf has become so successful a data structure, its use has become as ubiquitous as a plain string or an integer in our codebase.

But if we were moving in that direction, I have to wonder if some of these functions also need to be renamed to lose their strbuf_ prefix. A function that takes a pathname and creates a directory there is not called string_mkdir() just because it takes a pathname as a string. A function that takes a string buffer and a path, and reads the symbolic link content is not called string_readlink() just because it learns the symlink target in a string buffer. What their parameters mean matters a lot more than what type these parameters are represented as.

With some other topics in flight, merging this to 'seen' and merging this to 'next' may require different merge fixes, but I'll manage.

Thanks.