Re: [PATCH v4] sideband.c: remove redundant 'NEEDSWORK' tag

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 28, 2023, 20:33 UTC
Message-ID: <xmqq8r5egj85.fsf@gitster.g>
In-Reply-To: <pull.1625.v4.git.1703750460527.gitgitgadget@gmail.com>

"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH v4] sideband.c: remove redundant 'NEEDSWORK' tag

The reason for removal is not that it was redundant and we said the same thing elsewhere. Rather, what it claimed to be necessary has turned to be unwanted. So something like

    Subject: sideband.c: update stale NEEDSWORK comment

    If we really wanted to change the type of the parameter to this
    function to "size_t", we should also update its callers to hold
    the values they use to compute the parameter also in "size_t".

    But in this callchain, "int" is wide enough.  Avoid tempting
    future developers into wasting their time on using "size_t"
    around this function.

or along that line would be more appropriate, perhaps?

Thanks.

Show 47 quoted lines

> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>     sideband.c: replace int with size_t for clarity
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1625
>
> Range-diff vs v3:
>
>  1:  273415aa6a4 ! 1:  8c003256e5b sideband.c: remove redundant 'NEEDSWORK' tag
>      @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons
>         *
>       - * NEEDSWORK: use "size_t n" instead for clarity.
>       + * It is fine to use "int n" here instead of "size_t n" as all calls to this
>      -+ * function pass an 'int' parameter.
>      ++ * function pass an 'int' parameter. Additionally, the buffer involved in
>      ++ * storing these 'int' values takes input from a packet via the pkt-line
>      ++ * interface, which is capable of transferring only 64kB at a time.
>         */
>        static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>        {
>
>
>  sideband.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/sideband.c b/sideband.c
> index 6cbfd391c47..266a67342be 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -69,7 +69,10 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
>   * of the line. This should be called for a single line only, which is
>   * passed as the first N characters of the SRC array.
>   *
> - * NEEDSWORK: use "size_t n" instead for clarity.
> + * It is fine to use "int n" here instead of "size_t n" as all calls to this
> + * function pass an 'int' parameter. Additionally, the buffer involved in
> + * storing these 'int' values takes input from a packet via the pkt-line
> + * interface, which is capable of transferring only 64kB at a time.
>   */
>  static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>  {
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996