From: Elijah Newren <newren@gmail.com>
To: Calvin Wan <calvinwan@google.com>
Cc: git@vger.kernel.org, peff@peff.net
Subject: Re: [PATCH v2 0/7] strbuf cleanups
Date: Sat, 6 May 2023 17:40:47 -0700 [thread overview]
Message-ID: <CABPp-BGk-s+4b+OHi9FDXjwz5BeZPS8qb4XiJ8CVwkt8Siaq8A@mail.gmail.com> (raw)
In-Reply-To: <20230503184849.1809304-1-calvinwan@google.com>
On Wed, May 3, 2023 at 11:48 AM Calvin Wan <calvinwan@google.com> wrote:
>
> Strbuf is a widely used basic structure that should only interact with
> other primitives in strbuf.[ch].
"should"? I agree with moving things in this direction, but the
wording here suggests there's a pre-existing directive or guideline;
but if so, I'm unfamiliar with it. Maybe this should be reworded to
"This series modifies strbuf to focus on string manipulation with
minimal other dependencies"?
> Over time certain functions inside of
> strbuf.[ch] have been added to interact with higher level objects and
> functions. This series cleans up some of those higher level interactions
> by moving the offending functions to the files they interact with and
> adding documentation to strbuf.h. 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.
As Junio pointed out, "environment variables" is misleading; see below.
> Range-diff against v1:
> -: ---------- > 1: e0dd3f5295 strbuf: clarify API boundary
I read this separately; this patch looks good to me.
> 1: 283771c088 ! 2: ec1ea6ae4f abspath: move related functions to abspath
> @@ Commit message
> abspath: move related functions to abspath
>
> Move abspath-related functions from strbuf.[ch] to abspath.[ch] since
> - they do not belong in a low-level library.
> + paths are not primitive objects and therefore strbuf should not interact
> + with them.
This applies to patches 4 & 5 as well:
Would this perhaps be better worded as:
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: 58f78b8ae0 = 3: 2d74561b91 credential-store: move related functions to credential-store file
> 3: 88ab90c079 ! 4: 30b5e635cb 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 they do not belong in a low-level library.
> + since paths are not a primitive object that strbuf should directly
> + interact with.
>
> ## object-name.c ##
> @@ object-name.c: static void find_abbrev_len_packed(struct min_abbrev_data *mad)
> 4: 30b7de5a81 ! 5: 6905618470 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 it does
> - not belong in a low-level library.
> + 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.
>
> ## path.c ##
> @@ path.c: int normalize_path_copy(char *dst, const char *src)
> 5: 7b6d6353de ! 6: caf3482bf7 strbuf: clarify dependency
> @@ Metadata
> ## Commit message ##
> strbuf: clarify dependency
>
> + refs.h was once needed but is no longer so as of 6bab74e7fb8 ("strbuf:
> + move strbuf_branchname to sha1_name.c", 2010-11-06). strbuf.h was
> + included thru refs.h, so removing refs.h requires strbuf.h to be added
> + back.
> +
Thanks.
> 6: ffacd1cbe5 ! 7: a7f23488f8 strbuf: remove enviroment variables
> @@ Metadata
> Author: Calvin Wan <calvinwan@google.com>
>
> ## Commit message ##
> - strbuf: remove enviroment variables
> + strbuf: remove environment variables
>
> - As a lower level library, strbuf should not directly access environment
> - variables within its functions. Therefore, add an additional variable to
> - function signatures for functions that use an environment variable and
> - refactor callers to pass in the environment variable.
> + As a library that only interacts with other primitives, strbuf should
> + not directly access environment variables within its
> + functions. Therefore, add an additional variable to function signatures
> + for functions that use an environment variable and refactor callers to
> + pass in the environment variable.
"environment variable" makes one think not of "variable from
environment.c [that happens to be a global]" but of
https://en.wikipedia.org/wiki/Environment_variable. The fact that the
variable is defined in environment.c isn't even relevant here, though,
while the fact that it is a global is relevant. I'd just
s/environment variable/global variable/ in all 4 places (er, 5 if you
include the cover letter).
Your v3 7/7 has an important correction, but you didn't send out a
range diff for v3 so I'm putting my comments on your v2 range-diff.
:-)
> + /**
> * Strip whitespace from a buffer. The second parameter controls if
> - * comments are considered contents to be removed or not.
> +- * comments are considered contents to be removed or not.
> ++ * comments are considered contents to be removed or not. The third parameter
> ++ * is the comment character that determines whether a line is a comment or not.
> */
Thanks.
This series fixes two of my comments on v1, and the other was just
"here's another cleanup that could be added" -- it's fine to punt on.
I only had a couple minor comments on this v2/v3 that should be easy
to fix up, all included in this response to the cover letter.
next prev parent reply other threads:[~2023-05-07 0:56 UTC|newest]
Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-02 21:14 [PATCH 0/6] strbuf cleanups Calvin Wan
2023-05-02 21:14 ` [PATCH 1/6] abspath: move related functions to abspath Calvin Wan
2023-05-02 21:42 ` Junio C Hamano
2023-05-02 21:14 ` [PATCH 2/6] credential-store: move related functions to credential-store file Calvin Wan
2023-05-02 21:52 ` Junio C Hamano
2023-05-03 16:28 ` Jeff King
2023-05-03 16:34 ` Jeff King
2023-05-03 18:38 ` Calvin Wan
2023-05-02 21:14 ` [PATCH 3/6] object-name: move related functions to object-name Calvin Wan
2023-05-02 21:14 ` [PATCH 4/6] path: move related function to path Calvin Wan
2023-05-02 21:14 ` [PATCH 5/6] strbuf: clarify dependency Calvin Wan
2023-05-03 1:56 ` Elijah Newren
2023-05-02 21:14 ` [PATCH 6/6] strbuf: remove environment variables Calvin Wan
2023-05-03 2:15 ` Elijah Newren
2023-05-02 22:20 ` [PATCH 0/6] strbuf cleanups Junio C Hamano
2023-05-02 22:31 ` Junio C Hamano
2023-05-02 23:51 ` Felipe Contreras
2023-05-02 22:36 ` Calvin Wan
2023-05-03 2:37 ` Elijah Newren
2023-05-03 18:00 ` Calvin Wan
2023-05-07 0:14 ` Elijah Newren
2023-05-07 13:14 ` Jeff King
2023-05-03 18:48 ` [PATCH v2 0/7] " Calvin Wan
2023-05-03 18:50 ` [PATCH v2 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-03 18:50 ` [PATCH v2 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-03 18:50 ` [PATCH v2 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-03 18:50 ` [PATCH v2 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-03 18:50 ` [PATCH v2 5/7] path: move related function to path Calvin Wan
2023-05-03 18:50 ` [PATCH v2 6/7] strbuf: clarify dependency Calvin Wan
2023-05-03 19:26 ` Junio C Hamano
2023-05-03 18:50 ` [PATCH v2 7/7] strbuf: remove environment variables Calvin Wan
2023-05-03 19:24 ` Junio C Hamano
2023-05-03 19:41 ` Calvin Wan
2023-05-03 19:45 ` Junio C Hamano
2023-05-03 19:42 ` [PATCH v3 7/7] strbuf: remove environment variable Calvin Wan
2023-05-05 22:33 ` [PATCH v2 0/7] strbuf cleanups Junio C Hamano
2023-05-08 16:38 ` Calvin Wan
2023-05-07 0:40 ` Elijah Newren [this message]
2023-05-07 21:47 ` Felipe Contreras
2023-05-08 16:57 ` [PATCH v4 " Calvin Wan
2023-05-08 16:59 ` [PATCH v4 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-08 17:22 ` Eric Sunshine
2023-05-10 22:51 ` Junio C Hamano
2023-05-08 16:59 ` [PATCH v4 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-08 16:59 ` [PATCH v4 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-08 16:59 ` [PATCH v4 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-08 16:59 ` [PATCH v4 5/7] path: move related function to path Calvin Wan
2023-05-08 16:59 ` [PATCH v4 6/7] strbuf: clarify dependency Calvin Wan
2023-05-08 16:59 ` [PATCH v4 7/7] strbuf: remove global variable Calvin Wan
2023-05-10 8:12 ` Phillip Wood
2023-05-09 1:57 ` [PATCH v4 0/7] strbuf cleanups Elijah Newren
2023-05-09 2:13 ` Felipe Contreras
2023-05-11 19:44 ` [PATCH v5 " Calvin Wan
2023-05-11 19:48 ` [PATCH v5 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-11 19:57 ` Eric Sunshine
2023-05-11 20:03 ` Calvin Wan
2023-05-11 19:48 ` [PATCH v5 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-11 19:48 ` [PATCH v5 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-11 19:48 ` [PATCH v5 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-11 19:48 ` [PATCH v5 5/7] path: move related function to path Calvin Wan
2023-05-11 19:48 ` [PATCH v5 6/7] strbuf: clarify dependency Calvin Wan
2023-05-11 19:48 ` [PATCH v5 7/7] strbuf: remove global variable Calvin Wan
2023-05-11 20:24 ` Eric Sunshine
2023-05-11 21:42 ` Junio C Hamano
2023-05-12 14:54 ` Phillip Wood
2023-05-12 14:53 ` Phillip Wood
2023-05-12 19:31 ` Junio C Hamano
2023-05-12 17:14 ` [PATCH v6 0/7] strbuf cleanups Calvin Wan
2023-05-12 17:15 ` [PATCH v6 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-12 17:15 ` [PATCH v6 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-12 17:15 ` [PATCH v6 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-12 17:15 ` [PATCH v6 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-12 17:15 ` [PATCH v6 5/7] path: move related function to path Calvin Wan
2023-05-12 17:15 ` [PATCH v6 6/7] strbuf: clarify dependency Calvin Wan
2023-05-12 17:15 ` [PATCH v6 7/7] strbuf: remove global variable Calvin Wan
2023-05-12 20:24 ` [PATCH v6 0/7] strbuf cleanups Junio C Hamano
2023-05-13 5:54 ` Eric Sunshine
2023-06-06 19:47 ` [PATCH v7 " Calvin Wan
2023-06-06 19:48 ` [PATCH v7 1/7] strbuf: clarify API boundary Calvin Wan
2023-06-06 19:48 ` [PATCH v7 2/7] strbuf: clarify dependency Calvin Wan
2023-06-06 19:48 ` [PATCH v7 3/7] abspath: move related functions to abspath Calvin Wan
2023-06-06 19:48 ` [PATCH v7 4/7] credential-store: move related functions to credential-store file Calvin Wan
2023-06-06 19:48 ` [PATCH v7 5/7] object-name: move related functions to object-name Calvin Wan
2023-06-06 19:48 ` [PATCH v7 6/7] path: move related function to path Calvin Wan
2023-06-06 19:48 ` [PATCH v7 7/7] strbuf: remove global variable Calvin Wan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CABPp-BGk-s+4b+OHi9FDXjwz5BeZPS8qb4XiJ8CVwkt8Siaq8A@mail.gmail.com \
--to=newren@gmail.com \
--cc=calvinwan@google.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).