Re: [PATCH] remote: align --verbose output with spaces

From: Wang Bing-hua <louiswpf@gmail.com>
Date: Dec 17, 2024, 15:24 UTC
Message-ID: <1812003825ec9aea.c3843a5641fc0dd3.ac43970f069a1446@host>
In-Reply-To: <Z2F7VAsTjfBCVCrG@ArchLinux>

On 17/12/2024 21:23, shejialuo wrote:

> 
> Good enhancement.
>

Thank you.

Show 10 quoted lines

> 
> Nit: we should use "size_t" to declare/define loop variable `i`
> because the type of `list-nr` is `size_t`.
> 
> Recently, Patrick has provided a patch to start warn unsigned value
> compared with signed value in [1] which has not been merged into the
> master.
> 
> [1] https://lore.kernel.org/git/20241206-pks-sign-compare-v4-0-0344c6dfb219@pks.im
>

Show 10 quoted lines

> 
> So, here we traverse the list to find the max "utf8_strwidth". However,
> we should not EXPLICITLY traverse the string list. There are two ways
> you could do:
> 
> 1. Use the helper macro "for_each_string_list_item" in "string-list.h"
> to do above.
> 2. Use the helper function "for_each_string_list" in "string-list.c" to
> do above.
>

Show 5 quoted lines

> 
> So, here we call `calc_maxwidth` in the loop. That does not make sense.
> We should not call this function when we are traversing the string list.
> I think we should firstly calculate the max width outside of the loop.
>

Thank you for the detailed comments. It really helps. I will address these issues in v2.