Re: [PATCH v4 0/7] strbuf cleanups

From: Elijah Newren <newren@gmail.com>
Date: May 9, 2023, 01:57 UTC
Message-ID: <CABPp-BGTByZg5rJCmBeZNwjNTFwsoQ5HTjEhmhYFFfi-KbfgTw@mail.gmail.com>
In-Reply-To: <20230508165728.525603-1-calvinwan@google.com>

On Mon, May 8, 2023 at 9:57 AM Calvin Wan <calvinwan@google.com> wrote:

Show 44 quoted lines

>
> 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.

Show 57 quoted lines

> 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>