Re: [PATCH v2 12/14] builtin/patch-id: fix type of `get_one_patchid()`

[shejialuo <shejialuo@gmail.com>]

On Mon, Dec 02, 2024 at 01:04:44PM +0100, Patrick Steinhardt wrote:
> In `get_one_patchid()` we assign either the result of `strlen()` or
> `remove_space()` to `len`. But while the former correctly returns a
> `size_t`, the latter returns an `int` to indicate the length of the

It is a little misleading about the statement "the latte returns an
`int` to indicate the length of the string".

Should "without spaces" be appended into the last? However, don't worth
a reroll.

> string even though it cannot ever return a negative value. This causes a
> warning with "-Wsign-conversion".
> 
> In fact, even `get_one_patchid()` itself is also using an integer as
> return value even though it always returns the length of the patch, and
> this bubbles up to other callers.
> 
> Adapt the function and its helpers to use `size_t` for string lengths
> consistently.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/patch-id.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/patch-id.c b/builtin/patch-id.c
> index 56585757477911c96bbb9ef2cf3710691b8e744e..f540d8daa736b027649c8c64ffe5100cf4044037 100644
> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -1,5 +1,4 @@
>  #define USE_THE_REPOSITORY_VARIABLE
> -#define DISABLE_SIGN_COMPARE_WARNINGS
>  
>  #include "builtin.h"
>  #include "config.h"
> @@ -10,13 +9,13 @@
>  #include "parse-options.h"
>  #include "setup.h"
>  
> -static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)
> +static void flush_current_id(size_t patchlen, struct object_id *id, struct object_id *result)
>  {
>  	if (patchlen)
>  		printf("%s %s\n", oid_to_hex(result), oid_to_hex(id));
>  }
>  
> -static int remove_space(char *line)
> +static size_t remove_space(char *line)

So, eventually, we have decided to change the return type from "int" to
"size_t". This avoids casting "int" to "size_t", which is good. And it
would be more natural, because "remove_space" never returns negative
result.

>  {
>  	char *src = line;
>  	char *dst = line;

Thanks,
Jialuo