Re: [PATCH v3 1/4] add-patch: Fix type conversion warnings from msvc

[Patrick Steinhardt <ps@pks.im>]

Note: the word after the subject's subsystem should start with a
lower-case letter.

On Sun, Jan 26, 2025 at 01:56:35PM +0100, Sören Krecker wrote:
> Fix some compiler warnings from msvc in add-patch.c for value truncation
> form 64 bit to 32 bit integers. Change unsigned long to size_t for
> correct variable size on linux and windows.
> Add macro str_to_size_t for converting a string to size_t.

There shouldn't be a need for this macro, we already have `strtoumax()`.
And in case the platform doesn't provide it we know to provide our own
implementation.

> Test if convertion fails with over or underflow.

s/convertion/conversion/

> diff --git a/add-patch.c b/add-patch.c
> index 95c67d8c80..4fb6ae2c4b 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -322,11 +322,12 @@ static void setup_child_process(struct add_p_state *s,
>  }
>  
>  static int parse_range(const char **p,
> -		       unsigned long *offset, unsigned long *count)
> +		       size_t *offset, size_t *count)
>  {
>  	char *pend;
> -
> -	*offset = strtoul(*p, &pend, 10);
> +	*offset = str_to_size_t(*p, &pend, 10);
> +	if (errno == ERANGE)
> +		return error(_("Number is too large for this field"));

Error messages should start with a lower-case letter.

>  	if (pend == *p)
>  		return -1;
>  	if (*pend != ',') {
> @@ -334,7 +335,9 @@ static int parse_range(const char **p,
>  		*p = pend;
>  		return 0;
>  	}
> -	*count = strtoul(pend + 1, (char **)p, 10);
> +	*count = str_to_size_t(pend + 1, (char **)p, 10);
> +	if (errno == ERANGE)
> +		return error(_("Number is too large for this field"));

Here, too.

> @@ -1066,11 +1071,13 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
>  
>  	/* last hunk simply gets the rest */
>  	if (header->old_offset != remaining.old_offset)
> -		BUG("miscounted old_offset: %lu != %lu",
> -		    header->old_offset, remaining.old_offset);
> +		BUG("miscounted old_offset: %"PRIuMAX" != %"PRIuMAX,
> +		    (uintmax_t)header->old_offset,
> +		    (uintmax_t)remaining.old_offset);
>  	if (header->new_offset != remaining.new_offset)
> -		BUG("miscounted new_offset: %lu != %lu",
> -		    header->new_offset, remaining.new_offset);
> +		BUG("miscounted new_offset: %"PRIuMAX" != %"PRIuMAX,
> +		    (uintmax_t)header->new_offset,
> +		    (uintmax_t)remaining.new_offset);
>  	header->old_count = remaining.old_count;
>  	header->new_count = remaining.new_count;
>  	hunk->end = end;

I feel like most of the changes are adapting formatting directives like
this. Might be worthwhile to separate into a standalone commit. That'd
also allow the commit message to read less like a list of bullet points
and provide more context, explaining the actual change.

> diff --git a/gettext.h b/gettext.h
> index 484cafa562..d36f5a7ade 100644
> --- a/gettext.h
> +++ b/gettext.h
> @@ -53,7 +53,7 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
>  }
>  
>  static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
> -const char *Q_(const char *msgid, const char *plu, unsigned long n)
> +const char *Q_(const char *msgid, const char *plu, size_t n)
>  {
>  	if (!git_gettext_enabled)
>  		return n == 1 ? msgid : plu;

This change feels completely unrelated to all the other changes. It
would probably warrant a new commit.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index e283c46c6f..bb9a6c2bc4 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -292,6 +292,13 @@ static inline int _have_unix_sockets(void)
>  #include <sys/sysctl.h>
>  #endif
>  
> +#if SIZE_MAX == ULONG_MAX
> +#define str_to_size_t strtoul
> +#else
> +#define str_to_size_t strtoull
> +#endif

Hm. A couple of comments:

  - The function name doesn't match the schema of function names we
    already have. I would rather have expected it to be called something
    like `strtouz()` or something like that.

  - We tend to avoid using `strtoul()` and friends directly, as they are
    really hard to get right. See the implementation of `strtoul_ui()`
    for all the checks we do there.

  - The way the macro is implemented feels quite fragile.

So I'd propose to adapt the approach a bit and introduce a new function
`strtoumax_ui()`:

    static inline int strtoumax_ui(char *const *s, int base, unsigned
                                   uintmax_t max, int *result);

The implementation would mostly follow what we have in `strotul_ui()`.
The `max` parameter here could be used to control the maximum that the
caller expects -- if the parsed integer exceeds it, it would return an
error and set `ERANGE`. If we had such a helper, we can then also
reimplement `strtoul_ui()` on top of that function with a simple call to
`strtoumax_ui(s, base, UINT_MAX, result)`.

This would overall be a lot more flexible than what we currently have.

Patrick