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

[Phillip Wood <phillip.wood123@xxxxxxxxx>]

Hi Sören

On 26/01/2025 12:56, 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.

I'm afraid I'm still not convinced this is a good idea for the reasons I 
explained previously [1] together with an alternative approach to 
silencing these warnings. What makes "unsigned long" an incorrect choice 
when that's what "git diff" and "git apply" use?

[1] 
https://lore.kernel.org/git/e396131c-1bd3-46d0-bae6-cd97ca9710d8@xxxxxxxxx

> Add macro str_to_size_t for converting a string to size_t.
> Test if convertion fails with over or underflow.

That is welcome, but the implementation needs tweaking. If you look at 
other uses of strtoul() in our code you'll see that (somewhat unusually) 
one needs to set errno to zero before calling strtoul() as one cannot 
tell from the return value whether there was an error or not. As errno 
may have been set by a previous function call it needs to be cleared 
before calling strtoul() so we can be sure the error came from strtoul().

Best Wishes

Phillip

> Signed-off-by: Sören Krecker <soekkle@xxxxxxxxxx>
> ---
>   add-patch.c       | 53 +++++++++++++++++++++++++++--------------------
>   gettext.h         |  2 +-
>   git-compat-util.h |  7 +++++++
>   3 files changed, 39 insertions(+), 23 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 95c67d8c80..4fb6ae2c4b 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -242,7 +242,7 @@ static struct patch_mode patch_mode_worktree_nothead = {
>   };
>   
>   struct hunk_header {
> -	unsigned long old_offset, old_count, new_offset, new_count;
> +	size_t old_offset, old_count, new_offset, new_count;
>   	/*
>   	 * Start/end offsets to the extra text after the second `@@` in the
>   	 * hunk header, e.g. the function signature. This is expected to
> @@ -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"));
>   	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"));
>   	return *p == pend + 1 ? -1 : 0;
>   }
>   
> @@ -673,8 +676,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
>   		 */
>   		const char *p;
>   		size_t len;
> -		unsigned long old_offset = header->old_offset;
> -		unsigned long new_offset = header->new_offset;
> +		size_t old_offset = header->old_offset;
> +		size_t new_offset = header->new_offset;
>   
>   		if (!colored) {
>   			p = s->plain.buf + header->extra_start;
> @@ -700,12 +703,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
>   		else
>   			new_offset += delta;
>   
> -		strbuf_addf(out, "@@ -%lu", old_offset);
> +		strbuf_addf(out, "@@ -%" PRIuMAX, (uintmax_t)old_offset);
>   		if (header->old_count != 1)
> -			strbuf_addf(out, ",%lu", header->old_count);
> -		strbuf_addf(out, " +%lu", new_offset);
> +			strbuf_addf(out, ",%" PRIuMAX,
> +				    (uintmax_t)header->old_count);
> +		strbuf_addf(out, " +%" PRIuMAX, (uintmax_t)new_offset);
>   		if (header->new_count != 1)
> -			strbuf_addf(out, ",%lu", header->new_count);
> +			strbuf_addf(out, ",%" PRIuMAX,
> +				    (uintmax_t)header->new_count);
>   		strbuf_addstr(out, " @@");
>   
>   		if (len)
> @@ -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;
> @@ -1354,9 +1361,10 @@ static void summarize_hunk(struct add_p_state *s, struct hunk *hunk,
>   	struct strbuf *plain = &s->plain;
>   	size_t len = out->len, i;
>   
> -	strbuf_addf(out, " -%lu,%lu +%lu,%lu ",
> -		    header->old_offset, header->old_count,
> -		    header->new_offset, header->new_count);
> +	strbuf_addf(out,
> +		    " -%"PRIuMAX",%"PRIuMAX" +%"PRIuMAX",%"PRIuMAX" ",
> +		    (uintmax_t)header->old_offset, (uintmax_t)header->old_count,
> +		    (uintmax_t)header->new_offset, (uintmax_t)header->new_count);
>   	if (out->len - len < SUMMARY_HEADER_WIDTH)
>   		strbuf_addchars(out, ' ',
>   				SUMMARY_HEADER_WIDTH + len - out->len);
> @@ -1625,10 +1633,11 @@ static int patch_update_file(struct add_p_state *s,
>   			else if (0 < response && response <= file_diff->hunk_nr)
>   				hunk_index = response - 1;
>   			else
> -				err(s, Q_("Sorry, only %d hunk available.",
> -					  "Sorry, only %d hunks available.",
> -					  file_diff->hunk_nr),
> -				    (int)file_diff->hunk_nr);
> +				err(s,
> +				    Q_("Sorry, only %"PRIuMAX" hunk available.",
> +				       "Sorry, only %"PRIuMAX" hunks available.",
> +				       (uintmax_t)file_diff->hunk_nr),
> +				    (uintmax_t)file_diff->hunk_nr);
>   		} else if (s->answer.buf[0] == '/') {
>   			regex_t regex;
>   			int ret;
> 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;
> 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
> +
> +
>   /* Used by compat/win32/path-utils.h, and more */
>   static inline int is_xplatform_dir_sep(int c)
>   {