Re: [PATCH 0/6] strbuf cleanups
- From
Elijah Newren <newren@gmail.com>
- Date
- May 3, 2023, 02:37 UTC
- Message-ID
- <CABPp-BFagTvBTe0XbEOM_hO-vDFwVvSSmGOii1q2=7amqerwtQ@mail.gmail.com>
- In-Reply-To
- <20230502211454.1673000-1-calvinwan@google.com>
On Tue, May 2, 2023 at 2:15 PM Calvin Wan <calvinwan@google.com> wrote:
Show 48 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. > > 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@evledraar.gmail.com/, 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.