While moving strbuf_add_separated_string_list() to a separate file would mean that strbuf would no longer have a dependency on string-list, I don't think that dependency is problematic to begin with. Widening the boundary for strbuf as a string manipulation library to a string and string list manipulation library seems reasonable to me. On Tue, May 2, 2023 at 7:37 PM Elijah Newren <newren@xxxxxxxxx> wrote: > > On Tue, May 2, 2023 at 2:15 PM Calvin Wan <calvinwan@xxxxxxxxxx> wrote: > > > > 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. > > > > Calvin Wan (6): > > abspath: move related functions to abspath > > credential-store: move related functions to credential-store file > > object-name: move related functions to object-name > > path: move related function to path > > strbuf: clarify dependency > > strbuf: remove environment variables > > > > abspath.c | 36 +++++++++++++ > > abspath.h | 21 ++++++++ > > add-patch.c | 12 +++-- > > builtin/am.c | 2 +- > > builtin/branch.c | 4 +- > > builtin/commit.c | 2 +- > > builtin/credential-store.c | 19 +++++++ > > builtin/merge.c | 10 ++-- > > builtin/notes.c | 16 +++--- > > builtin/rebase.c | 2 +- > > builtin/stripspace.c | 6 ++- > > builtin/tag.c | 9 ++-- > > fmt-merge-msg.c | 9 ++-- > > gpg-interface.c | 5 +- > > hook.c | 1 + > > object-name.c | 15 ++++++ > > object-name.h | 9 ++++ > > path.c | 20 +++++++ > > path.h | 5 ++ > > pretty.c | 1 + > > rebase-interactive.c | 15 +++--- > > sequencer.c | 24 +++++---- > > strbuf.c | 106 +++---------------------------------- > > strbuf.h | 44 ++------------- > > tempfile.c | 1 + > > wt-status.c | 6 +-- > > 26 files changed, 213 insertions(+), 187 deletions(-) > > > > -- > > 2.40.1.495.gc816e09b53d-goog > > The series looks pretty good to me. I left a couple small comments on > 5/6 and 6/6. One other high-level note: > > As Ævar noted over at > https://lore.kernel.org/git/230501.86a5yohsme.gmgdl@xxxxxxxxxxxxxxxxxxx/, > strbuf_add_separated_string_list() is currently only used by > merge-ort.c and merge-recursive.c (both of which include > merge-recursive.h). It may make sense to move that function, and that > would permit you to drop the forward declaration of 'struct > string_list;' from strbuf.c as well.
