From: Elijah Newren <newren@gmail.com>
To: Calvin Wan <calvinwan@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 6/6] strbuf: remove environment variables
Date: Tue, 2 May 2023 19:15:00 -0700 [thread overview]
Message-ID: <CABPp-BGgP50TV7Gc8bpu1vvR-6-EKKC8eM_j=o4imz7Y-55pMw@mail.gmail.com> (raw)
In-Reply-To: <20230502211454.1673000-7-calvinwan@google.com>
On Tue, May 2, 2023 at 2:15 PM Calvin Wan <calvinwan@google.com> wrote:
>
> 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.
Yaay! I hate the pervasive implicit use of globals throughout the
codebase, and like seeing changes that at least make them less hidden.
I might have split this patch into three, one for each of the
functions you are changing. The reason is just that it makes review
so much easier; a simple --color-words view then becomes easy
to rapidly scan through on each patch. When one patch includes all
three, reviewers have to double check the position of the added
parameter in the argument list against the name of the function. But
that's a really minor point, and some might prefer as you have it here
with the three squashed together.
[...]
> diff --git a/strbuf.h b/strbuf.h
> index b6d53c1cbe..f90da8859f 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -283,7 +283,8 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
> * by a comment character and a blank.
> */
> void strbuf_add_commented_lines(struct strbuf *out,
> - const char *buf, size_t size);
> + const char *buf, size_t size,
> + char comment_line_char);
>
>
> /**
> @@ -412,8 +413,8 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
> * Add a formatted string prepended by a comment character and a
> * blank to the buffer.
> */
> -__attribute__((format (printf, 2, 3)))
> -void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);
> +__attribute__((format (printf, 3, 4)))
> +void strbuf_commented_addf(struct strbuf *sb, char comment_line_char, const char *fmt, ...);
>
> __attribute__((format (printf,2,0)))
> void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
> @@ -538,7 +539,7 @@ int strbuf_normalize_path(struct strbuf *sb);
> * Strip whitespace from a buffer. The second parameter controls if
> * comments are considered contents to be removed or not.
Should this documentation string be updated, given that it previously
documented all arguments to the function?
> */
> -void strbuf_stripspace(struct strbuf *buf, int skip_comments);
> +void strbuf_stripspace(struct strbuf *buf, int skip_comments, char comment_line_char);
>
> static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
> {
[...]
The rest looks good to me.
next prev parent reply other threads:[~2023-05-03 2:15 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 [this message]
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
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-BGgP50TV7Gc8bpu1vvR-6-EKKC8eM_j=o4imz7Y-55pMw@mail.gmail.com' \
--to=newren@gmail.com \
--cc=calvinwan@google.com \
--cc=git@vger.kernel.org \
/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).