From: Junio C Hamano <gitster@pobox.com>
To: "Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Chandra Pratap <chandrapratap376@gmail.com>,
Chandra Pratap <chandrapratap3519@gmail.com>
Subject: Re: [PATCH] sideband.c: replace int with size_t for clarity
Date: Tue, 26 Dec 2023 09:14:26 -0800 [thread overview]
Message-ID: <xmqqo7ecj37x.fsf@gitster.g> (raw)
In-Reply-To: <pull.1625.git.1703264469238.gitgitgadget@gmail.com> (Chandra Pratap via GitGitGadget's message of "Fri, 22 Dec 2023 17:01:09 +0000")
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> -static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)
Changing the type of the paramter alone might be a good start, but
does not really help anybody, as (1) the callers are not taught to
handle wider integral types for the values they pass and (2) the
function uses "int len" it computes internally to compare with "n".
There are three callers in demultiplex_sideband(), one of whose
paramters is "int len" and is passed to this function in one of
these calls. Among the other two, one uses "int linelen" derived
from scanning the piece of memory read from sideband via strpbrk(),
and the other uses strlen(b) which is the length of the "remainder"
of the same buffer after the loop that processes one line at a time
using the said strpbrk() is done with the complete lines in the
early part.
The buffer involved in all of the above stores bytes taken from a
packet received via the pkt-line interface, which is capable of
transferring only 64kB at a time.
I _think_ the most productive use of our time is to replace the
NEEDSWORK with a comment saying why it is fine to use "int" here to
avoid tempting the next developer to come up with this patch
again---they will waste their time to do so without thinking it
through if we leave the (incomplete) NEEDSWORK comment, I am afraid.
prev parent reply other threads:[~2023-12-26 17:14 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-22 17:01 [PATCH] sideband.c: replace int with size_t for clarity Chandra Pratap via GitGitGadget
2023-12-22 17:57 ` Torsten Bögershausen
2023-12-22 18:27 ` Chandra Pratap
2023-12-22 18:39 ` Torsten Bögershausen
2023-12-22 19:01 ` Junio C Hamano
2024-01-02 22:31 ` Taylor Blau
2023-12-23 17:03 ` [PATCH v2] " Chandra Pratap via GitGitGadget
2023-12-27 10:20 ` [PATCH v3] sideband.c: remove redundant 'NEEDSWORK' tag Chandra Pratap via GitGitGadget
2023-12-27 22:22 ` Junio C Hamano
2023-12-28 8:01 ` [PATCH v4] " Chandra Pratap via GitGitGadget
2023-12-28 20:33 ` Junio C Hamano
2023-12-26 17:14 ` Junio C Hamano [this message]
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=xmqqo7ecj37x.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=chandrapratap3519@gmail.com \
--cc=chandrapratap376@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
/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 NNTP newsgroup(s).