| Message ID | 20230508165728.525603-1-calvinwan@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | strbuf cleanups | expand |
On Mon, May 8, 2023 at 9:57 AM Calvin Wan <calvinwan@google.com> 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@google.com> > > ## 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@gmail.com>
Elijah Newren wrote: > On Mon, May 8, 2023 at 9:57 AM Calvin Wan <calvinwan@google.com> wrote: > > 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? I guess I can plug git-send-series [1] as well. > * handles the range-diff consistently > * sets up reply-to nicely Same with git-send-email. > * 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 This is orthogonal to sending a patch series, but anyone pushing to GitHub can do that. Advantages of git-send-email that gitgitgadget doesn't have: * Everything about it can be handled locally * It's a standalone script fully in your control * It's not tied to any organization or corporation [1] https://github.com/felipec/git-send-series
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 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@google.com> ## 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.