| Message ID | 20250126125638.3089-2-soekkle@freenet.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix type conversion Warings from msvc | expand |
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
Patrick Steinhardt <ps@pks.im> writes: > 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. Thanks for a detailed review; I'll omit them as I agree with all you said there. If I pretend for a while that moving from ulong to size_t is a good change for line numbers and line counts in the first place, that is. In other words, I agree with all the improvements your comments suggest to the _implementation_. Thanks.
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) > {
Phillip Wood <phillip.wood123@gmail.com> writes: > 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 Ah, this patch still does that? I was hoping that it got corrected already after it was pointed out in the previous iterations. I agree with you that size_t is a dubious type to use for the line numbers there. diff.c defines "struct emit_callback" with lno_in_{pre,post}image members that are of type "int", which is somewhat dubious, too, but at least we don't run on 16-bit machines, and being limited to 2 billion lines is probably OK. I am OK to upgrade that to long (if we use negative values for some oob signal) or ulong, but that is clearly outside of this topic. >> 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(). Nice advice. > Best Wishes > > Phillip Thanks. By the way, who is <CAPig+cQ49Hdc_8=mRhhJDTny_Kqo6Wg6Nr98rsBN_YXmBrQ6kA@mail.gmail.com> and why is such an apparently bogus e-mail address Cc'ed?
Hi Junio On 29/01/2025 19:52, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > > By the way, who is > <CAPig+cQ49Hdc_8=mRhhJDTny_Kqo6Wg6Nr98rsBN_YXmBrQ6kA@mail.gmail.com> > and why is such an apparently bogus e-mail address Cc'ed? That's the Reply-To address from the mail I was replying to. Unfortunately it does not seem to exist. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > On 29/01/2025 19:52, Junio C Hamano wrote: >> Phillip Wood <phillip.wood123@gmail.com> writes: >> By the way, who is >> <CAPig+cQ49Hdc_8=mRhhJDTny_Kqo6Wg6Nr98rsBN_YXmBrQ6kA@mail.gmail.com> >> and why is such an apparently bogus e-mail address Cc'ed? > > That's the Reply-To address from the mail I was replying > to. Unfortunately it does not seem to exist. It just occured to me that it is probably added by a mistake and the sender really wanted to add it to In-Reply-To: instead of Reply-To: I wonder if this is a mistake we can do something to help users avoid? "git send-email" has the "--reply-to=" option and there is a valid use case for that option, so disabling that option is a non-starter. Of course there are other ways to send e-mailed patches, but I do not think of a way to misuse them with reply-to and in-reply-to mixed up. Thoughts?
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) {
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. Test if convertion fails with over or underflow. 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(-)