From: Junio C Hamano <gitster@pobox•com>
To: Jonathan Tan <jonathantanmy@google•com>
Cc: git@vger•kernel.org, Dragan Simic <dsimic@manjaro•org>,
Phillip Wood <phillip.wood123@gmail•com>
Subject: Re: [PATCH v2 3/4] strbuf: make add_lines() public
Date: Wed, 01 Nov 2023 13:14:29 +0900 [thread overview]
Message-ID: <xmqq5y2mun2y.fsf@gitster.g> (raw)
In-Reply-To: <283f502acb68910cb43d6077eef99d6345aaea4b.1698791220.git.jonathantanmy@google.com> (Jonathan Tan's message of "Tue, 31 Oct 2023 15:28:32 -0700")
Jonathan Tan <jonathantanmy@google•com> writes:
> -static void add_lines(struct strbuf *out,
> - const char *prefix1,
> - const char *prefix2,
> - const char *buf, size_t size)
> +void strbuf_add_lines_varied_prefix(struct strbuf *sb,
> + const char *default_prefix,
> + const char *tab_nl_prefix,
> + const char *buf, size_t size)
> {
> while (size) {
> const char *prefix;
> const char *next = memchr(buf, '\n', size);
> next = next ? (next + 1) : (buf + size);
>
> - prefix = ((prefix2 && (buf[0] == '\n' || buf[0] == '\t'))
> - ? prefix2 : prefix1);
> - strbuf_addstr(out, prefix);
> - strbuf_add(out, buf, next - buf);
> + prefix = (buf[0] == '\n' || buf[0] == '\t')
> + ? tab_nl_prefix : default_prefix;
> + strbuf_addstr(sb, prefix);
> + strbuf_add(sb, buf, next - buf);
The original allowed callers to pass NULL for the second prefix when
they want to use the same prefix, even for commenting out an empty
line or a line that begins with a tab. The new one does not allow
the callers to do so. As long as updating the existing callers are
done carefully, the difference would not matter, but would it help
new callers in the future to rid the usability feature like this
patch does while performing a refactoring? The loss of feature is
not even documented, by the way.
While "tab_nl" sound a bit more specific than "2", I am not sure if
we made it better. It does not make it clear why it makes sense to
(and it is necessary to) special case HT and LF. A developer who is
writing a new caller would not know why there are two prefixes
supported, or why the function is named "varied prefix", with these
names.
Giving a name that explains the reason might help the readability.
I've been thinking what the best name for this function would be but
not successfully.
It may be that we shouldn't take two prefixes in the first place.
The ONLY case callers want to pass prefix2 that is different from
prefix1 is when prefix1 ends with a space, and prefix2 is identical
to prefix1 without the trailing space. The reason they use such a
pair of prefixes is to avoid leaving a trailing whitespace (when
buf[0] == '\n') or having a space before tab (when buf[0] == '\t')
on the generated lines.
So eventually we may want to have something like this as the final
interface given to the public callers, simply because ...
strbuf_add_lines_as_comments(struct strbuf *sb,
const char *comment_prefix,
const char *buf, size_t size)
{
while (size) {
const char *next = memchr(buf, '\n', size);
next = next ? (next + 1) : (buf + size);
strbuf_addstr(sb, comment_prefix);
/* avoid trailing-whitespace and space-before-tab */
if (buf[0] != '\n' && buf[0] != '\t')
strbuf_addch(sb, ' ');
strbuf_add(sb, buf, next - buf);
... loop control ...
}
... strbuf completion ...
}
... there is no need for totally unrelated two prefix variants. And
both the function name and the parameter name would be a bit easier
to understand than your version (and far easier than the original).
The function is about commenting out all the lines in buf with the
comment prefix, and most of the time we add a space between the
comment character and the commented out text, but in some cases we
do not want to add the space.
But as I said already, I'd prefer to see a patch that claims to be a
refactoring to do as little as necessary. Giving it a name better
than add_lines() is inevitable, because you are making it extern.
But I'd prefer to see the parameter naems and the function body left
untouched and kept the same as the original. It should be left to a
separate step to improve the interface and the implementation.
Thanks.
next prev parent reply other threads:[~2023-11-01 4:14 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-30 5:10 [PATCH 0/2] Avoid passing global comment_line_char repeatedly Junio C Hamano
2023-10-30 5:10 ` [PATCH 1/2] strbuf_commented_addf(): drop the comment_line_char parameter Junio C Hamano
2023-10-30 5:10 ` [PATCH 2/2] strbuf_add_commented_lines(): " Junio C Hamano
2023-10-30 5:36 ` [PATCH 0/2] Avoid passing global comment_line_char repeatedly Dragan Simic
2023-10-30 9:59 ` Phillip Wood
2023-10-30 20:22 ` [RFC PATCH 0/3] " Jonathan Tan
2023-10-30 20:22 ` [RFC PATCH 1/3] strbuf: make add_lines() public Jonathan Tan
2023-10-30 23:53 ` Junio C Hamano
2023-10-31 6:01 ` Junio C Hamano
2023-10-30 20:22 ` [RFC PATCH 2/3] strbuf_commented_addf(): drop the comment_line_char parameter Jonathan Tan
2023-10-31 5:19 ` Junio C Hamano
2023-10-31 22:24 ` Jonathan Tan
2023-10-31 23:54 ` Junio C Hamano
2023-10-30 20:22 ` [RFC PATCH 3/3] strbuf_add_commented_lines(): " Jonathan Tan
2023-10-31 22:28 ` [PATCH v2 0/4] Avoid passing global comment_line_char repeatedly Jonathan Tan
2023-10-31 22:28 ` [PATCH v2 1/4] strbuf_commented_addf(): drop the comment_line_char parameter Jonathan Tan
2023-10-31 22:28 ` [PATCH v2 2/4] strbuf_add_commented_lines(): " Jonathan Tan
2023-10-31 22:28 ` [PATCH v2 3/4] strbuf: make add_lines() public Jonathan Tan
2023-11-01 4:14 ` Junio C Hamano [this message]
2023-10-31 22:28 ` [PATCH v2 4/4] strbuf: move env-using functions to environment.c Jonathan Tan
2023-11-01 4:37 ` Junio C Hamano
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=xmqq5y2mun2y.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=dsimic@manjaro$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=jonathantanmy@google$(echo .)com \
--cc=phillip.wood123@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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