On Tue, May 2, 2023 at 3:20 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Calvin Wan <calvinwan@xxxxxxxxxx> writes: > > > 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". Thanks for clarifying the boundary I was attempting to explain in my cover letter, but unsuccessfully did so. > > 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. I agree that with the functions that are moved out from strbuf, they should be refactored in the future to the form, taking absolute path as an example, strbuf_addstr(sb, get_absolute_path(path)) or something similar. > > 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. >
