On Mon, May 8, 2023 at 9:57 AM Calvin Wan <calvinwan@xxxxxxxxxx> wrote: > > This reroll include suggestions made by Elijah and Junio, clarifying > commit messages in patches 2/4/5 and s/environment variable/global > variable in patch 7. > > Calvin Wan (7): > strbuf: clarify API boundary > 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 global variable > > 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 | 53 +++++-------------- > tempfile.c | 1 + > wt-status.c | 6 +-- > 26 files changed, 220 insertions(+), 189 deletions(-) > > Range-diff against v3: > -: ---------- > 1: e0dd3f5295 strbuf: clarify API boundary Huh? I thought v3 had this patch. v2 certainly did, and with the same contents. Did you just give the wrong range for the range-diff? Aside: Can I plug gitgitgadget for a minute? It * handles the range-diff consistently * sets up reply-to nicely * ensures testing on a variety of platforms * makes it _trivial_ to download the series via a simple fetch, with remote/branch/command documented in the cover letter I think the last point is particularly cool. Yes, I know of b4, and it helps a lot, but a simple fetch, especially one where the cover letter contains the command, makes things easier for someone who comes along to review. I know I'm happier as a reviewer when I see a patch series sent by gitgitgadget, because of this last point. (Granted, folks could just copy that really nice usability touch by hand in their cover letters if they like send-email, but I suspect it's enough of a hassle to the sender that people just don't do it.). Anyway, just some food for thought. > 1: ec1ea6ae4f ! 2: 48fb5db28b abspath: move related functions to abspath > @@ Metadata > ## Commit message ## > abspath: move related functions to abspath > > - Move abspath-related functions from strbuf.[ch] to abspath.[ch] since > - paths are not primitive objects and therefore strbuf should not interact > - with them. > + Move abspath-related functions from strbuf.[ch] to abspath.[ch] so that > + strbuf is focused on string manipulation routines with minimal > + dependencies. > > ## abspath.c ## > @@ abspath.c: char *prefix_filename_except_for_dash(const char *pfx, const char *arg) > 2: 2d74561b91 = 3: a663f91819 credential-store: move related functions to credential-store file > 3: 30b5e635cb ! 4: ccef9dd5f2 object-name: move related functions to object-name > @@ Commit message > object-name: move related functions to object-name > > Move object-name-related functions from strbuf.[ch] to object-name.[ch] > - since paths are not a primitive object that strbuf should directly > - interact with. > + so that strbuf is focused on string manipulation routines with minimal > + dependencies. > > ## object-name.c ## > @@ object-name.c: static void find_abbrev_len_packed(struct min_abbrev_data *mad) > 4: 6905618470 ! 5: 0d6b9cf0f7 path: move related function to path > @@ Metadata > ## Commit message ## > path: move related function to path > > - Move path-related function from strbuf.[ch] to path.[ch] since path is > - not a primitive object and therefore strbuf should not directly interact > - with it. > + Move path-related function from strbuf.[ch] to path.[ch] so that strbuf > + is focused on string manipulation routines with minimal dependencies. > > ## path.c ## > @@ path.c: int normalize_path_copy(char *dst, const char *src) > 5: caf3482bf7 = 6: 5655c56a6d strbuf: clarify dependency > 6: 3bbaebf292 ! 7: 874d0efac3 strbuf: remove environment variable > @@ Metadata > Author: Calvin Wan <calvinwan@xxxxxxxxxx> > > ## Commit message ## > - strbuf: remove environment variable > + strbuf: remove global variable > > As a library that only interacts with other primitives, strbuf should > - not utilize the comment_line_char environment variable within its > + not utilize the comment_line_char global variable within its > functions. Therefore, add an additional parameter for functions that use > comment_line_char and refactor callers to pass it in instead. > > -- > 2.40.1.521.gf1e218fcd8-goog Other than the off-by-one in the range-diff of the cover letter, this all looks good to me: Reviewed-by: Elijah Newren <newren@xxxxxxxxx>
