From: Phillip Wood <phillip.wood123@gmail•com>
To: CAPig+cQ49Hdc_8=mRhhJDTny_Kqo6Wg6Nr98rsBN_YXmBrQ6kA@mail•gmail.com,
git@vger•kernel.org
Cc: gitster@pobox•com, ps@pks•im, sunshine@sunshineco•com,
"Sören Krecker" <soekkle@freenet•de>
Subject: Re: [PATCH v3 1/4] add-patch: Fix type conversion warnings from msvc
Date: Wed, 29 Jan 2025 16:51:42 +0000 [thread overview]
Message-ID: <6a251603-25bc-415d-ab8c-ae698bd7977a@gmail.com> (raw)
In-Reply-To: <20250126125638.3089-2-soekkle@freenet.de>
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@gmail.com
> 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@freenet•de>
> ---
> 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)
> {
next prev parent reply other threads:[~2025-01-29 16:51 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-26 12:56 [PATCH v3 0/4] Fix type conversion Warings from msvc Sören Krecker
2025-01-26 12:56 ` [PATCH v3 1/4] add-patch: Fix type conversion warnings " Sören Krecker
2025-01-27 7:26 ` Patrick Steinhardt
2025-01-27 16:10 ` Junio C Hamano
2025-01-29 16:51 ` Phillip Wood [this message]
2025-01-29 19:52 ` Junio C Hamano
2025-01-30 10:47 ` Phillip Wood
2025-01-30 19:24 ` Junio C Hamano
2025-01-27 7:26 ` [PATCH v3 0/4] Fix type conversion Warings " Patrick Steinhardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6a251603-25bc-415d-ab8c-ae698bd7977a@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=CAPig+cQ49Hdc_8=mRhhJDTny_Kqo6Wg6Nr98rsBN_YXmBrQ6kA@mail$(echo .)gmail.com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=ps@pks$(echo .)im \
--cc=soekkle@freenet$(echo .)de \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox