{"thread":{"id":"62685","subject":"[PATCH 0/4] Fixes typemissmatch warinigs from msvc","startedAt":"2024-12-23T11:09:38Z","lastAt":"2024-12-28T16:04:15Z","messageCount":17,"participants":["Sören Krecker","Junio C Hamano","Patrick Steinhardt","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"509513","messageId":"20241223110407.3308-1-soekkle@freenet.de","threadId":"62685","inReplyTo":null,"subject":"[PATCH 0/4] Fixes typemissmatch warinigs from msvc","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-12-23T11:04:03Z","receivedAt":"2024-12-23T11:09:38Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"A smale series of patches to fix some typemissmatch warings from msvc 14.30.\nMost of the missmatches a 64 to 32 bit conversion on a 64 bit Windows platform.\n\nI use size_t where the variable values cannot become negative.\n\nSören Krecker (4):\n add-patch: Fix type missmatch rom msvc\n date.c: Fix type missmatch warings from msvc\n apply.c : Fix type missmatch warings from msvc\n commit.c: Fix type missmatch warings from msvc\n\n add-patch.c | 44 +++++++++++++++++++++++++-------------------\n apply.c | 37 +++++++++++++++++++------------------\n apply.h | 6 +++---\n commit.c | 10 +++++-----\n date.c | 6 +++---\n gettext.h | 2 +-\n 6 files changed, 56 insertions(+), 49 deletions(-)\n\n\nbase-commit: ff795a5c5ed2e2d07c688c217a615d89e3f5733b\n-- \n2.39.5\n\nThanks\n\nSören Krecker\n"},{"id":"509514","messageId":"20241223110407.3308-5-soekkle@freenet.de","threadId":"62685","inReplyTo":"20241223110407.3308-1-soekkle@freenet.de","subject":"[PATCH 4/4] commit.c: Fix type missmatch warings from msvc","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-12-23T11:04:07Z","receivedAt":"2024-12-23T11:09:43Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Fix compiler warings from msvc in date.c for value truncation from 64\nbit to 32 bit integers.\n\nAlso switch from int to size_t for all variables with result of strlen()\nwhich cannot become negative.\n\nSigned-off-by: Sören Krecker \n---\n commit.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 35ab9bead5..3d363260f3 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -466,8 +466,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tstruct object_id parent;\n \tstruct commit_list **pptr;\n \tstruct commit_graft *graft;\n-\tconst int tree_entry_len = the_hash_algo->hexsz + 5;\n-\tconst int parent_entry_len = the_hash_algo->hexsz + 7;\n+\tconst size_t tree_entry_len = the_hash_algo->hexsz + 5;\n+\tconst size_t parent_entry_len = the_hash_algo->hexsz + 7;\n \tstruct tree *tree;\n \n \tif (item->object.parsed)\n@@ -1114,10 +1114,10 @@ static const char *gpg_sig_headers[] = {\n \n int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct git_hash_algo *algo)\n {\n-\tint inspos, copypos;\n+\tssize_t inspos, copypos;\n \tconst char *eoh;\n \tconst char *gpg_sig_header = gpg_sig_headers[hash_algo_by_ptr(algo)];\n-\tint gpg_sig_header_len = strlen(gpg_sig_header);\n+\tsize_t gpg_sig_header_len = strlen(gpg_sig_header);\n \n \t/* find the end of the header */\n \teoh = strstr(buf->buf, \"\\n\\n\");\n@@ -1530,7 +1530,7 @@ int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree,\n \treturn result;\n }\n \n-static int find_invalid_utf8(const char *buf, int len)\n+static int find_invalid_utf8(const char *buf, size_t len)\n {\n \tint offset = 0;\n \tstatic const unsigned int max_codepoint[] = {\n-- \n2.39.5\n\n"},{"id":"509515","messageId":"20241223110407.3308-2-soekkle@freenet.de","threadId":"62685","inReplyTo":"20241223110407.3308-1-soekkle@freenet.de","subject":"[PATCH 1/4] add-patch: Fix type missmatch rom msvc","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-12-23T11:04:04Z","receivedAt":"2024-12-23T11:09:46Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Fix some compiler warings from msvw in add-patch.c for value truncation\nform 64 bit to 32 bit integers.Change unsigned long to size_t for\ncorrect variable size on linux and windows\n\nSigned-off-by: Sören Krecker \n---\n add-patch.c | 44 +++++++++++++++++++++++++-------------------\n gettext.h | 2 +-\n 2 files changed, 26 insertions(+), 20 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 557903310d..1ea70ef988 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -241,7 +241,7 @@ static struct patch_mode patch_mode_worktree_nothead = {\n };\n \n struct hunk_header {\n-\tunsigned long old_offset, old_count, new_offset, new_count;\n+\tsize_t old_offset, old_count, new_offset, new_count;\n \t/*\n \t * Start/end offsets to the extra text after the second `@@` in the\n \t * hunk header, e.g. the function signature. This is expected to\n@@ -321,7 +321,7 @@ static void setup_child_process(struct add_p_state *s,\n }\n \n static int parse_range(const char **p,\n-\t\t unsigned long *offset, unsigned long *count)\n+\t\t size_t *offset, size_t *count)\n {\n \tchar *pend;\n \n@@ -672,8 +672,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\t */\n \t\tconst char *p;\n \t\tsize_t len;\n-\t\tunsigned long old_offset = header->old_offset;\n-\t\tunsigned long new_offset = header->new_offset;\n+\t\tsize_t old_offset = header->old_offset;\n+\t\tsize_t new_offset = header->new_offset;\n \n \t\tif (!colored) {\n \t\t\tp = s->plain.buf + header->extra_start;\n@@ -699,12 +699,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\telse\n \t\t\tnew_offset += delta;\n \n-\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n+\t\tstrbuf_addf(out, \"@@ -%\" PRIuMAX, (uintmax_t)old_offset);\n \t\tif (header->old_count != 1)\n-\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n-\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n+\t\t\tstrbuf_addf(out, \",%\" PRIuMAX,\n+\t\t\t\t (uintmax_t)header->old_count);\n+\t\tstrbuf_addf(out, \" +%\" PRIuMAX, (uintmax_t)new_offset);\n \t\tif (header->new_count != 1)\n-\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n+\t\t\tstrbuf_addf(out, \",%\" PRIuMAX,\n+\t\t\t\t (uintmax_t)header->new_count);\n \t\tstrbuf_addstr(out, \" @@\");\n \n \t\tif (len)\n@@ -1065,11 +1067,13 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n \n \t/* last hunk simply gets the rest */\n \tif (header->old_offset != remaining.old_offset)\n-\t\tBUG(\"miscounted old_offset: %lu != %lu\",\n-\t\t header->old_offset, remaining.old_offset);\n+\t\tBUG(\"miscounted old_offset: %\"PRIuMAX\" != %\"PRIuMAX,\n+\t\t (uintmax_t)header->old_offset,\n+\t\t (uintmax_t)remaining.old_offset);\n \tif (header->new_offset != remaining.new_offset)\n-\t\tBUG(\"miscounted new_offset: %lu != %lu\",\n-\t\t header->new_offset, remaining.new_offset);\n+\t\tBUG(\"miscounted new_offset: %\"PRIuMAX\" != %\"PRIuMAX,\n+\t\t (uintmax_t)header->new_offset,\n+\t\t (uintmax_t)remaining.new_offset);\n \theader->old_count = remaining.old_count;\n \theader->new_count = remaining.new_count;\n \thunk->end = end;\n@@ -1353,9 +1357,10 @@ static void summarize_hunk(struct add_p_state *s, struct hunk *hunk,\n \tstruct strbuf *plain = &s->plain;\n \tsize_t len = out->len, i;\n \n-\tstrbuf_addf(out, \" -%lu,%lu +%lu,%lu \",\n-\t\t header->old_offset, header->old_count,\n-\t\t header->new_offset, header->new_count);\n+\tstrbuf_addf(out,\n+\t\t \" -%\"PRIuMAX\",%\"PRIuMAX\" +%\"PRIuMAX\",%\"PRIuMAX\" \",\n+\t\t (uintmax_t)header->old_offset, (uintmax_t)header->old_count,\n+\t\t (uintmax_t)header->new_offset, (uintmax_t)header->new_count);\n \tif (out->len - len < SUMMARY_HEADER_WIDTH)\n \t\tstrbuf_addchars(out, ' ',\n \t\t\t\tSUMMARY_HEADER_WIDTH + len - out->len);\n@@ -1624,10 +1629,11 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\telse if (0 < response && response <= file_diff->hunk_nr)\n \t\t\t\thunk_index = response - 1;\n \t\t\telse\n-\t\t\t\terr(s, Q_(\"Sorry, only %d hunk available.\",\n-\t\t\t\t\t \"Sorry, only %d hunks available.\",\n-\t\t\t\t\t file_diff->hunk_nr),\n-\t\t\t\t (int)file_diff->hunk_nr);\n+\t\t\t\terr(s,\n+\t\t\t\t Q_(\"Sorry, only %\"PRIuMAX\" hunk available.\",\n+\t\t\t\t \"Sorry, only %\"PRIuMAX\" hunks available.\",\n+\t\t\t\t (uintmax_t)file_diff->hunk_nr),\n+\t\t\t\t (uintmax_t)file_diff->hunk_nr);\n \t\t} else if (s->answer.buf[0] == '/') {\n \t\t\tregex_t regex;\n \t\t\tint ret;\ndiff --git a/gettext.h b/gettext.h\nindex 484cafa562..d36f5a7ade 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -53,7 +53,7 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n }\n \n static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)\n-const char *Q_(const char *msgid, const char *plu, unsigned long n)\n+const char *Q_(const char *msgid, const char *plu, size_t n)\n {\n \tif (!git_gettext_enabled)\n \t\treturn n == 1 ? msgid : plu;\n-- \n2.39.5\n\n"},{"id":"509516","messageId":"20241223110407.3308-3-soekkle@freenet.de","threadId":"62685","inReplyTo":"20241223110407.3308-1-soekkle@freenet.de","subject":"[PATCH 2/4] date.c: Fix type missmatch warings from msvc","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-12-23T11:04:05Z","receivedAt":"2024-12-23T11:09:48Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Fix compiler warings from msvc in date.c for value truncation from 64\nbit to 32 bit integers.\n\nAlso switch from int to size_t for all variables with result of strlen()\nwhich cannot become negative.\n\nSigned-off-by: Sören Krecker \n---\n date.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex bee9fe8f10..8ae19f9ecc 100644\n--- a/date.c\n+++ b/date.c\n@@ -1242,7 +1242,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \t}\n \n \tfor (s = special; s->name; s++) {\n-\t\tint len = strlen(s->name);\n+\t\tsize_t len = strlen(s->name);\n \t\tif (match_string(date, s->name) == len) {\n \t\t\ts->fn(tm, now, num);\n \t\t\t*touched = 1;\n@@ -1252,7 +1252,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \n \tif (!*num) {\n \t\tfor (i = 1; i < 11; i++) {\n-\t\t\tint len = strlen(number_name[i]);\n+\t\t\tsize_t len = strlen(number_name[i]);\n \t\t\tif (match_string(date, number_name[i]) == len) {\n \t\t\t\t*num = i;\n \t\t\t\t*touched = 1;\n@@ -1268,7 +1268,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \n \ttl = typelen;\n \twhile (tl->type) {\n-\t\tint len = strlen(tl->type);\n+\t\tsize_t len = strlen(tl->type);\n \t\tif (match_string(date, tl->type) >= len-1) {\n \t\t\tupdate_tm(tm, now, tl->length * *num);\n \t\t\t*num = 0;\n-- \n2.39.5\n\n"},{"id":"509517","messageId":"20241223110407.3308-4-soekkle@freenet.de","threadId":"62685","inReplyTo":"20241223110407.3308-1-soekkle@freenet.de","subject":"[PATCH 3/4] apply.c : Fix type missmatch warings from msvc","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-12-23T11:04:06Z","receivedAt":"2024-12-23T11:09:51Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Fix compiler warings from msvc in date.c for value truncation from 64\nbit to 32 bit integers.\n\nAlso switch from int to size_t for all variables with result of strlen()\nwhich cannot become negative.\n\nSigned-off-by: Sören Krecker \n---\n apply.c | 37 +++++++++++++++++++------------------\n apply.h | 6 +++---\n 2 files changed, 22 insertions(+), 21 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex a3fc2d5330..5bb0b0e78e 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -413,9 +413,9 @@ static int read_patch_file(struct strbuf *sb, int fd)\n \treturn 0;\n }\n \n-static unsigned long linelen(const char *buffer, unsigned long size)\n+static size_t linelen(const char *buffer, size_t size)\n {\n-\tunsigned long len = 0;\n+\tsize_t len = 0;\n \twhile (size--) {\n \t\tlen++;\n \t\tif (*buffer++ == '\\n')\n@@ -687,7 +687,7 @@ static char *find_name_common(struct strbuf *root,\n \t * or \"file~\").\n \t */\n \tif (def) {\n-\t\tint deflen = strlen(def);\n+\t\tsize_t deflen = strlen(def);\n \t\tif (deflen < len && !strncmp(start, def, deflen))\n \t\t\treturn squash_slash(xstrdup(def));\n \t}\n@@ -1087,7 +1087,7 @@ static int gitdiff_index(struct gitdiff_data *state,\n \t */\n \tconst char *ptr, *eol;\n \tint len;\n-\tconst unsigned hexsz = the_hash_algo->hexsz;\n+\tconst size_t hexsz = the_hash_algo->hexsz;\n \n \tptr = strchr(line, '.');\n \tif (!ptr || ptr[1] != '.' || hexsz < ptr - line)\n@@ -1130,7 +1130,7 @@ static int gitdiff_unrecognized(struct gitdiff_data *state UNUSED,\n */\n static const char *skip_tree_prefix(int p_value,\n \t\t\t\t const char *line,\n-\t\t\t\t int llen)\n+\t\t\t\t size_t llen)\n {\n \tint nslash;\n \tint i;\n@@ -1157,7 +1157,7 @@ static const char *skip_tree_prefix(int p_value,\n */\n static char *git_header_name(int p_value,\n \t\t\t const char *line,\n-\t\t\t int llen)\n+\t\t\t ssize_t llen)\n {\n \tconst char *name;\n \tconst char *second = NULL;\n@@ -1312,15 +1312,15 @@ static int check_header_line(int linenr, struct patch *patch)\n \treturn 0;\n }\n \n-int parse_git_diff_header(struct strbuf *root,\n+size_t parse_git_diff_header(struct strbuf *root,\n \t\t\t int *linenr,\n \t\t\t int p_value,\n \t\t\t const char *line,\n-\t\t\t int len,\n-\t\t\t unsigned int size,\n+\t\t\t size_t len,\n+\t\t\t size_t size,\n \t\t\t struct patch *patch)\n {\n-\tunsigned long offset;\n+\tsize_t offset;\n \tstruct gitdiff_data parse_hdr_state;\n \n \t/* A git diff has explicit new/delete information, so we don't guess */\n@@ -1377,7 +1377,7 @@ int parse_git_diff_header(struct strbuf *root,\n \t\t\tbreak;\n \t\tfor (i = 0; i < ARRAY_SIZE(optable); i++) {\n \t\t\tconst struct opentry *p = optable + i;\n-\t\t\tint oplen = strlen(p->str);\n+\t\t\tsize_t oplen = strlen(p->str);\n \t\t\tint res;\n \t\t\tif (len < oplen || memcmp(p->str, line, oplen))\n \t\t\t\tcontinue;\n@@ -1429,7 +1429,8 @@ static int parse_num(const char *line, unsigned long *p)\n static int parse_range(const char *line, int len, int offset, const char *expect,\n \t\t unsigned long *p1, unsigned long *p2)\n {\n-\tint digits, ex;\n+\tint digits;\n+\tsize_t ex;\n \n \tif (offset < 0 || offset >= len)\n \t\treturn -1;\n@@ -1464,7 +1465,7 @@ static int parse_range(const char *line, int len, int offset, const char *expect\n \treturn offset + ex;\n }\n \n-static void recount_diff(const char *line, int size, struct fragment *fragment)\n+static void recount_diff(const char *line, size_t size, struct fragment *fragment)\n {\n \tint oldlines = 0, newlines = 0, ret = 0;\n \n@@ -1474,7 +1475,7 @@ static void recount_diff(const char *line, int size, struct fragment *fragment)\n \t}\n \n \tfor (;;) {\n-\t\tint len = linelen(line, size);\n+\t\tsize_t len = linelen(line, size);\n \t\tsize -= len;\n \t\tline += len;\n \n@@ -1542,11 +1543,11 @@ static int parse_fragment_header(const char *line, int len, struct fragment *fra\n */\n static int find_header(struct apply_state *state,\n \t\t const char *line,\n-\t\t unsigned long size,\n+\t\t size_t size,\n \t\t int *hdrsize,\n \t\t struct patch *patch)\n {\n-\tunsigned long offset, len;\n+\tsize_t offset, len;\n \n \tpatch->is_toplevel_relative = 0;\n \tpatch->is_rename = patch->is_copy = 0;\n@@ -2131,7 +2132,7 @@ static int use_patch(struct apply_state *state, struct patch *p)\n * the number of bytes consumed otherwise,\n * so that the caller can call us again for the next patch.\n */\n-static int parse_chunk(struct apply_state *state, char *buffer, unsigned long size, struct patch *patch)\n+static int parse_chunk(struct apply_state *state, char *buffer, size_t size, struct patch *patch)\n {\n \tint hdrsize, patchsize;\n \tint offset = find_header(state, buffer, size, &hdrsize, patch);\n@@ -2490,7 +2491,7 @@ static int match_fragment(struct apply_state *state,\n \tstruct strbuf fixed = STRBUF_INIT;\n \tchar *fixed_buf;\n \tsize_t fixed_len;\n-\tint preimage_limit;\n+\tssize_t preimage_limit;\n \tint ret;\n \n \tif (preimage->line_nr + current_lno <= img->line_nr) {\ndiff --git a/apply.h b/apply.h\nindex 90e887ec0e..bb01ce7dbc 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -166,12 +166,12 @@ int check_apply_state(struct apply_state *state, int force_apply);\n *\n * Returns -1 on failure, the length of the parsed header otherwise.\n */\n-int parse_git_diff_header(struct strbuf *root,\n+size_t parse_git_diff_header(struct strbuf *root,\n \t\t\t int *linenr,\n \t\t\t int p_value,\n \t\t\t const char *line,\n-\t\t\t int len,\n-\t\t\t unsigned int size,\n+\t\t\t size_t len,\n+\t\t\t size_t size,\n \t\t\t struct patch *patch);\n \n void release_patch(struct patch *patch);\n-- \n2.39.5\n\n"},{"id":"509524","messageId":"xmqqmsgm1hku.fsf@gitster.g","threadId":"62685","inReplyTo":"20241223110407.3308-1-soekkle@freenet.de","subject":"Re: [PATCH 0/4] Fixes typemissmatch warinigs from msvc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-23T16:37:21Z","receivedAt":"2024-12-23T16:37:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sören Krecker writes:\n\n> A smale series of patches to fix some typemissmatch warings from msvc 14.30.\n> Most of the missmatches a 64 to 32 bit conversion on a 64 bit Windows platform.\n\nThanks for the patches.\n\nI'll welcome other people to take a look, if they are inclined, but\nit is not something I'd want to look at during a pre-release freeze.\nNobody sane would be running \"git add -p\" on a patch that exceeds\n2GB, for example, so the only practical thing they fix are compiler\nwarnings. They are worth fixing eventually, but not all that\nurgent.\n\nThanks, again.\n"},{"id":"509525","messageId":"xmqqikra1gux.fsf@gitster.g","threadId":"62685","inReplyTo":"xmqqmsgm1hku.fsf@gitster.g","subject":"Re: [PATCH 0/4] Fixes typemissmatch warinigs from msvc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-23T16:52:54Z","receivedAt":"2024-12-23T16:52:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Sören Krecker writes:\n>\n>> A smale series of patches to fix some typemissmatch warings from msvc 14.30.\n>> Most of the missmatches a 64 to 32 bit conversion on a 64 bit Windows platform.\n>\n> Thanks for the patches.\n>\n> I'll welcome other people to take a look, if they are inclined, but\n> it is not something I'd want to look at during a pre-release freeze.\n> Nobody sane would be running \"git add -p\" on a patch that exceeds\n> 2GB, for example, so the only practical thing they fix are compiler\n> warnings. They are worth fixing eventually, but not all that\n> urgent.\n>\n> Thanks, again.\n\nOops, sorry, this didn't come out quite right. I didn't mean to say\nthat this contribution is unwelcome. I'll get to it eventually\n(like, after the upcoming release), but please do not expect them to\nbe merged before the upcoming release.\n\n"},{"id":"509563","messageId":"c2381e2e-ff13-4549-ba42-75f77775c99f@freenet.de","threadId":"62685","inReplyTo":"xmqqikra1gux.fsf@gitster.g","subject":"Re: [PATCH 0/4] Fixes typemissmatch warinigs from msvc","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-12-26T08:59:02Z","receivedAt":"2024-12-26T09:04:21Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Hi Junio,\n\nI think I undersand your first mail correct, that this is only a code \nquality change and not a bug in the code. And so it isn't critical for \ngit 2.48.\nIn the future I will send some more commits for warings from msvc in \ngit. MSVC print more the 1000 warings.\n\nBest regards,\n\nSören Krecker\n\n\nAm 23.12.24 um 17:52 schrieb Junio C Hamano:\n> Junio C Hamano writes:\n> \n>> Sören Krecker writes:\n>>\n>>> A smale series of patches to fix some typemissmatch warings from msvc 14.30.\n>>> Most of the missmatches a 64 to 32 bit conversion on a 64 bit Windows platform.\n>>\n>> Thanks for the patches.\n>>\n>> I'll welcome other people to take a look, if they are inclined, but\n>> it is not something I'd want to look at during a pre-release freeze.\n>> Nobody sane would be running \"git add -p\" on a patch that exceeds\n>> 2GB, for example, so the only practical thing they fix are compiler\n>> warnings. They are worth fixing eventually, but not all that\n>> urgent.\n>>\n>> Thanks, again.\n> \n> Oops, sorry, this didn't come out quite right. I didn't mean to say\n> that this contribution is unwelcome. I'll get to it eventually\n> (like, after the upcoming release), but please do not expect them to\n> be merged before the upcoming release.\n> \n\n"},{"id":"509576","messageId":"xmqq34iaxh7r.fsf@gitster.g","threadId":"62685","inReplyTo":"20241223110407.3308-2-soekkle@freenet.de","subject":"Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-26T21:33:12Z","receivedAt":"2024-12-26T21:33:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sören Krecker writes:\n\n> Fix some compiler warings from msvw in add-patch.c for value truncation\n> form 64 bit to 32 bit integers.Change unsigned long to size_t for\n> correct variable size on linux and windows\n>\n> Signed-off-by: Sören Krecker \n> ---\n> add-patch.c | 44 +++++++++++++++++++++++++-------------------\n> gettext.h | 2 +-\n> 2 files changed, 26 insertions(+), 20 deletions(-)\n\n\n\n> struct hunk_header {\n> -\tunsigned long old_offset, old_count, new_offset, new_count;\n> +\tsize_t old_offset, old_count, new_offset, new_count;\n\nThese are not \"size\"s in the traditional sense of what size_t is\n(i.e. the number of bytes in a region of memory), but are more or\nless proportional to that in that they count in number of lines.\n\nIf ulong is sufficient to count number of lines in an incoming\npatch, then turning size_t may be excessive---are we sure that we\nare not unnecessarily using wider-than-necessary size_t in some\nplaces to hold these values for which ulong is sufficient, causing\ncompilers to emit unnecessary warning?\n\nIOW, if we have variables of unsigned integer of various sizes, we\n_could_ rewrite all of them to use uintmax_t and there won't be\ntruncation-upon-assignment warnings from a compiler, but such a\nrewrite can be pointless. We'd need to find where we stuff these\nvalues, which are originally ulong, to size_t, and see if the use of\nsize_t is really sensible.\n\n> @@ -321,7 +321,7 @@ static void setup_child_process(struct add_p_state *s,\n> }\n> \n> static int parse_range(const char **p,\n> -\t\t unsigned long *offset, unsigned long *count)\n> +\t\t size_t *offset, size_t *count)\n> {\n> \tchar *pend;\n> \n> @@ -672,8 +672,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\t */\n> \t\tconst char *p;\n> \t\tsize_t len;\n> -\t\tunsigned long old_offset = header->old_offset;\n> -\t\tunsigned long new_offset = header->new_offset;\n> +\t\tsize_t old_offset = header->old_offset;\n> +\t\tsize_t new_offset = header->new_offset;\n> \n> \t\tif (!colored) {\n> \t\t\tp = s->plain.buf + header->extra_start;\n> @@ -699,12 +699,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\telse\n> \t\t\tnew_offset += delta;\n> \n> -\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n> +\t\tstrbuf_addf(out, \"@@ -%\" PRIuMAX, (uintmax_t)old_offset);\n> \t\tif (header->old_count != 1)\n> -\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n> -\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n> +\t\t\tstrbuf_addf(out, \",%\" PRIuMAX,\n> +\t\t\t\t (uintmax_t)header->old_count);\n> +\t\tstrbuf_addf(out, \" +%\" PRIuMAX, (uintmax_t)new_offset);\n> \t\tif (header->new_count != 1)\n> -\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> +\t\t\tstrbuf_addf(out, \",%\" PRIuMAX,\n> +\t\t\t\t (uintmax_t)header->new_count);\n> \t\tstrbuf_addstr(out, \" @@\");\n> \n> \t\tif (len)\n\nSo far if we left the types of *_offset and count all ulong, I do\nnot see anything that causes trunation-upon-assignment.\n\n> @@ -1065,11 +1067,13 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n> \n> \t/* last hunk simply gets the rest */\n> \tif (header->old_offset != remaining.old_offset)\n> -\t\tBUG(\"miscounted old_offset: %lu != %lu\",\n> -\t\t header->old_offset, remaining.old_offset);\n> +\t\tBUG(\"miscounted old_offset: %\"PRIuMAX\" != %\"PRIuMAX,\n> +\t\t (uintmax_t)header->old_offset,\n> +\t\t (uintmax_t)remaining.old_offset);\n> \tif (header->new_offset != remaining.new_offset)\n> -\t\tBUG(\"miscounted new_offset: %lu != %lu\",\n> -\t\t header->new_offset, remaining.new_offset);\n> +\t\tBUG(\"miscounted new_offset: %\"PRIuMAX\" != %\"PRIuMAX,\n> +\t\t (uintmax_t)header->new_offset,\n> +\t\t (uintmax_t)remaining.new_offset);\n> \theader->old_count = remaining.old_count;\n> \theader->new_count = remaining.new_count;\n> \thunk->end = end;\n\nThis hunk unfortunately may be needed because this function somehow\ndecided to use size_t for things like \"hunk_index\" and \"end\" when it\nwas added, even though the surrounding functions it interacts with\nall used ulong.\n\n> @@ -1353,9 +1357,10 @@ static void summarize_hunk(struct add_p_state *s, struct hunk *hunk,\n> \tstruct strbuf *plain = &s->plain;\n> \tsize_t len = out->len, i;\n> \n> -\tstrbuf_addf(out, \" -%lu,%lu +%lu,%lu \",\n> -\t\t header->old_offset, header->old_count,\n> -\t\t header->new_offset, header->new_count);\n> +\tstrbuf_addf(out,\n> +\t\t \" -%\"PRIuMAX\",%\"PRIuMAX\" +%\"PRIuMAX\",%\"PRIuMAX\" \",\n> +\t\t (uintmax_t)header->old_offset, (uintmax_t)header->old_count,\n> +\t\t (uintmax_t)header->new_offset, (uintmax_t)header->new_count);\n> \tif (out->len - len < SUMMARY_HEADER_WIDTH)\n> \t\tstrbuf_addchars(out, ' ',\n> \t\t\t\tSUMMARY_HEADER_WIDTH + len - out->len);\n\nAgain, if we left the types of *_offset and count all ulong, I do\nnot see anything that causes trunation-upon-assignment.\n\n> @@ -1624,10 +1629,11 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\telse if (0 < response && response <= file_diff->hunk_nr)\n> \t\t\t\thunk_index = response - 1;\n> \t\t\telse\n> -\t\t\t\terr(s, Q_(\"Sorry, only %d hunk available.\",\n> -\t\t\t\t\t \"Sorry, only %d hunks available.\",\n> -\t\t\t\t\t file_diff->hunk_nr),\n> -\t\t\t\t (int)file_diff->hunk_nr);\n> +\t\t\t\terr(s,\n> +\t\t\t\t Q_(\"Sorry, only %\"PRIuMAX\" hunk available.\",\n> +\t\t\t\t \"Sorry, only %\"PRIuMAX\" hunks available.\",\n> +\t\t\t\t (uintmax_t)file_diff->hunk_nr),\n> +\t\t\t\t (uintmax_t)file_diff->hunk_nr);\n\nAgain, this hunk may be needed, as the \"hunk_nr\" uses size_t, which\nprobably is overly wide.\n\nSo, I do not mind too much to adjust the code around hunk_nr,\nhunk_alloc and other things that are already size_t (but before\ndoing so, we probably should see if it makes more sense to use ulong\nfor these members instead of size_t), hbut I am not sure if it is a\nsensible move to change old_offset, count, etc. that count in number\nof lines and use ulong (not bytes) to use size_t instead.\n\nsize_t _might_ be wider than some other forms of unsigned integers,\nbut it is not necessarily the widest, so \"because on msvc ulong is\nmerely 32-bit and I want wider integer like everybody else!\" is not\na good excuse for such a change (if it were, we'd all coding with\nnothing but uintmax_t).\n\nSo I dunno.\n\n> diff --git a/gettext.h b/gettext.h\n> index 484cafa562..d36f5a7ade 100644\n> --- a/gettext.h\n> +++ b/gettext.h\n> @@ -53,7 +53,7 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n> }\n> \n> static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)\n> -const char *Q_(const char *msgid, const char *plu, unsigned long n)\n> +const char *Q_(const char *msgid, const char *plu, size_t n)\n> {\n> \tif (!git_gettext_enabled)\n> \t\treturn n == 1 ? msgid : plu;\n"},{"id":"509577","messageId":"xmqqy102w2li.fsf@gitster.g","threadId":"62685","inReplyTo":"20241223110407.3308-3-soekkle@freenet.de","subject":"Re: [PATCH 2/4] date.c: Fix type missmatch warings from msvc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-26T21:34:17Z","receivedAt":"2024-12-26T21:34:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sören Krecker writes:\n\n> Fix compiler warings from msvc in date.c for value truncation from 64\n> bit to 32 bit integers.\n>\n> Also switch from int to size_t for all variables with result of strlen()\n> which cannot become negative.\n>\n> Signed-off-by: Sören Krecker \n> ---\n> date.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/date.c b/date.c\n> index bee9fe8f10..8ae19f9ecc 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -1242,7 +1242,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n> \t}\n> \n> \tfor (s = special; s->name; s++) {\n> -\t\tint len = strlen(s->name);\n> +\t\tsize_t len = strlen(s->name);\n> \t\tif (match_string(date, s->name) == len) {\n> \t\t\ts->fn(tm, now, num);\n> \t\t\t*touched = 1;\n> @@ -1252,7 +1252,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n> \n> \tif (!*num) {\n> \t\tfor (i = 1; i < 11; i++) {\n> -\t\t\tint len = strlen(number_name[i]);\n> +\t\t\tsize_t len = strlen(number_name[i]);\n> \t\t\tif (match_string(date, number_name[i]) == len) {\n> \t\t\t\t*num = i;\n> \t\t\t\t*touched = 1;\n> @@ -1268,7 +1268,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n> \n> \ttl = typelen;\n> \twhile (tl->type) {\n> -\t\tint len = strlen(tl->type);\n> +\t\tsize_t len = strlen(tl->type);\n> \t\tif (match_string(date, tl->type) >= len-1) {\n> \t\t\tupdate_tm(tm, now, tl->length * *num);\n> \t\t\t*num = 0;\n\nThese are all good changes, unquestionably. strlen() counts in\nbytes and returns size_t; we should recieve the returned value in a\nvariable of type size_t.\n\nWill queue. Thanks.\n"},{"id":"509578","messageId":"xmqqttaqw2eb.fsf@gitster.g","threadId":"62685","inReplyTo":"20241223110407.3308-5-soekkle@freenet.de","subject":"Re: [PATCH 4/4] commit.c: Fix type missmatch warings from msvc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-26T21:38:36Z","receivedAt":"2024-12-26T21:38:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sören Krecker writes:\n\n> Fix compiler warings from msvc in date.c for value truncation from 64\n> bit to 32 bit integers.\n>\n> Also switch from int to size_t for all variables with result of strlen()\n> which cannot become negative.\n>\n> Signed-off-by: Sören Krecker \n> ---\n> commit.c | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/commit.c b/commit.c\n> index 35ab9bead5..3d363260f3 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -466,8 +466,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n> \tstruct object_id parent;\n> \tstruct commit_list **pptr;\n> \tstruct commit_graft *graft;\n> -\tconst int tree_entry_len = the_hash_algo->hexsz + 5;\n> -\tconst int parent_entry_len = the_hash_algo->hexsz + 7;\n> +\tconst size_t tree_entry_len = the_hash_algo->hexsz + 5;\n> +\tconst size_t parent_entry_len = the_hash_algo->hexsz + 7;\n\nWe saw another change around hexsz in this series, but I seriously\ndoubt that it is sensible to define .hexsz member of git_hash_algo\nas type size_t. The whole _point_ of hash function is so that it\ncan be represented by a handful of bytes, so insisting size_t and\nforcing us to suffer code churning like we see here is simply crazy.\n\nWould it work equally well, if not better, if you instead fixed the\ntype of the .hexsz member (and its friends) to something more\nreasonable, like \"int\"?\n"},{"id":"509586","messageId":"Z25-aaS7s3baU7ly@pks.im","threadId":"62685","inReplyTo":"xmqq34iaxh7r.fsf@gitster.g","subject":"Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:16:09Z","receivedAt":"2024-12-27T10:16:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 26, 2024 at 01:33:12PM -0800, Junio C Hamano wrote:\n> Sören Krecker writes:\n> > @@ -1624,10 +1629,11 @@ static int patch_update_file(struct add_p_state *s,\n> > \t\t\telse if (0 < response && response <= file_diff->hunk_nr)\n> > \t\t\t\thunk_index = response - 1;\n> > \t\t\telse\n> > -\t\t\t\terr(s, Q_(\"Sorry, only %d hunk available.\",\n> > -\t\t\t\t\t \"Sorry, only %d hunks available.\",\n> > -\t\t\t\t\t file_diff->hunk_nr),\n> > -\t\t\t\t (int)file_diff->hunk_nr);\n> > +\t\t\t\terr(s,\n> > +\t\t\t\t Q_(\"Sorry, only %\"PRIuMAX\" hunk available.\",\n> > +\t\t\t\t \"Sorry, only %\"PRIuMAX\" hunks available.\",\n> > +\t\t\t\t (uintmax_t)file_diff->hunk_nr),\n> > +\t\t\t\t (uintmax_t)file_diff->hunk_nr);\n> \n> Again, this hunk may be needed, as the \"hunk_nr\" uses size_t, which\n> probably is overly wide.\n> \n> So, I do not mind too much to adjust the code around hunk_nr,\n> hunk_alloc and other things that are already size_t (but before\n> doing so, we probably should see if it makes more sense to use ulong\n> for these members instead of size_t), hbut I am not sure if it is a\n> sensible move to change old_offset, count, etc. that count in number\n> of lines and use ulong (not bytes) to use size_t instead.\n> \n> size_t _might_ be wider than some other forms of unsigned integers,\n> but it is not necessarily the widest, so \"because on msvc ulong is\n> merely 32-bit and I want wider integer like everybody else!\" is not\n> a good excuse for such a change (if it were, we'd all coding with\n> nothing but uintmax_t).\n> \n> So I dunno.\n\nIn practice, the number of bytes and number of lines _can_ be the same\nwhen the file consists of newlines, only. That is of course unlikely to\nbe the case in any sane input, but may be the case when processing input\nthat was specifically crafted to trigger such an edge case. So using a\ntype that can store the maximum size of a theoretically possible object\nfeels like a sensible safeguard to me and requires us to worry less\nabout such weird edge cases.\n\nI also doubt that widening the type to `size_t` would have a meaningful\nimpact on performance, so I don't see a strong reason not to go there.\nI tend to think that using `size_t` in such size-like-fields should be\nour default unless there is a good reason not to pick it.\n\nPatrick\n"},{"id":"509591","messageId":"e396131c-1bd3-46d0-bae6-cd97ca9710d8@gmail.com","threadId":"62685","inReplyTo":"xmqq34iaxh7r.fsf@gitster.g","subject":"Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-12-27T10:38:33Z","receivedAt":"2024-12-27T10:38:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/12/2024 21:33, Junio C Hamano wrote:\n> Sören Krecker writes:\n> \n>> Fix some compiler warings from msvw in add-patch.c for value truncation\n>> form 64 bit to 32 bit integers.Change unsigned long to size_t for\n>> correct variable size on linux and windows\n>>\n>> Signed-off-by: Sören Krecker \n>> ---\n>> add-patch.c | 44 +++++++++++++++++++++++++-------------------\n>> gettext.h | 2 +-\n>> 2 files changed, 26 insertions(+), 20 deletions(-)\n> \n> \n> \n>> struct hunk_header {\n>> -\tunsigned long old_offset, old_count, new_offset, new_count;\n>> +\tsize_t old_offset, old_count, new_offset, new_count;\n> \n> These are not \"size\"s in the traditional sense of what size_t is\n> (i.e. the number of bytes in a region of memory), but are more or\n> less proportional to that in that they count in number of lines.\n> \n> If ulong is sufficient to count number of lines in an incoming\n> patch, then turning size_t may be excessive---are we sure that we\n> are not unnecessarily using wider-than-necessary size_t in some\n> places to hold these values for which ulong is sufficient, causing\n> compilers to emit unnecessary warning?\n\nThat's my thought too - I think something like the diff below should\nfix the warnings by using more appropriate types in expressions\ninvolving the hunk header offset and count. Our internal diff\nimplementation will not generate diffs for blobs greater than ~1GB\nand I don't think \"git apply\" can handle diff headers that contain\nnumbers greater that ULONG_MAX so switching to size_t here seems\nunnecessary.\n\nBest Wishes\n\nPhillip\n\n---- >8 ----\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 557903310de..2c439b83665 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -253,7 +253,7 @@ struct hunk_header {\n \n struct hunk {\n \tsize_t start, end, colored_start, colored_end, splittable_into;\n-\tssize_t delta;\n+\tlong delta;\n \tenum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK } use;\n \tstruct hunk_header header;\n };\n@@ -760,7 +760,8 @@ static void render_diff_header(struct add_p_state *s,\n static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,\n \t\t size_t *hunk_index, int use_all, struct hunk *merged)\n {\n-\tsize_t i = *hunk_index, delta;\n+\tsize_t i = *hunk_index;\n+\tlong delta;\n \tstruct hunk *hunk = file_diff->hunk + i;\n \t/* `header` corresponds to the merged hunk */\n \tstruct hunk_header *header = &merged->header, *next;\n@@ -890,7 +891,7 @@ static void reassemble_patch(struct add_p_state *s,\n {\n \tstruct hunk *hunk;\n \tsize_t save_len = s->plain.len, i;\n-\tssize_t delta = 0;\n+\tlong delta = 0;\n \n \trender_diff_header(s, file_diff, 0, out);\n \n@@ -926,7 +927,8 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n \tint colored = !!s->colored.len, first = 1;\n \tstruct hunk *hunk = file_diff->hunk + hunk_index;\n \tsize_t splittable_into;\n-\tsize_t end, colored_end, current, colored_current = 0, context_line_count;\n+\tsize_t end, colored_end, current, colored_current = 0;\n+\tunsigned long context_line_count;\n \tstruct hunk_header remaining, *header;\n \tchar marker, ch;\n \n@@ -1175,8 +1177,8 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n \treturn 1;\n }\n \n-static ssize_t recount_edited_hunk(struct add_p_state *s, struct hunk *hunk,\n-\t\t\t\t size_t orig_old_count, size_t orig_new_count)\n+static long recount_edited_hunk(struct add_p_state *s, struct hunk *hunk,\n+\t\t\t\t unsigned long orig_old_count, unsigned long orig_new_count)\n {\n \tstruct hunk_header *header = &hunk->header;\n \tsize_t i;\n@@ -1626,7 +1628,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\telse\n \t\t\t\terr(s, Q_(\"Sorry, only %d hunk available.\",\n \t\t\t\t\t \"Sorry, only %d hunks available.\",\n-\t\t\t\t\t file_diff->hunk_nr),\n+\t\t\t\t\t (int)file_diff->hunk_nr),\n \t\t\t\t (int)file_diff->hunk_nr);\n \t\t} else if (s->answer.buf[0] == '/') {\n \t\t\tregex_t regex;\n\n\n"},{"id":"509633","messageId":"xmqq5xn5urhv.fsf@gitster.g","threadId":"62685","inReplyTo":"e396131c-1bd3-46d0-bae6-cd97ca9710d8@gmail.com","subject":"Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-27T14:31:40Z","receivedAt":"2024-12-27T14:31:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>>> struct hunk_header {\n>>> -\tunsigned long old_offset, old_count, new_offset, new_count;\n>>> +\tsize_t old_offset, old_count, new_offset, new_count;\n>> These are not \"size\"s in the traditional sense of what size_t is\n>> (i.e. the number of bytes in a region of memory), but are more or\n>> less proportional to that in that they count in number of lines.\n>> If ulong is sufficient to count number of lines in an incoming\n>> patch, then turning size_t may be excessive---are we sure that we\n>> are not unnecessarily using wider-than-necessary size_t in some\n>> places to hold these values for which ulong is sufficient, causing\n>> compilers to emit unnecessary warning?\n>\n> That's my thought too - I think something like the diff below should\n> fix the warnings by using more appropriate types in expressions\n> involving the hunk header offset and count. Our internal diff\n> implementation will not generate diffs for blobs greater than ~1GB\n> and I don't think \"git apply\" can handle diff headers that contain\n> numbers greater that ULONG_MAX so switching to size_t here seems\n> unnecessary.\n\nYes, exactly.\n\nOf course, when filling old_offset and friends by parsing an input\nline like this:\n\n @@ -253,7 +253,7 @@ struct hunk_header {\n\nit would be a bug if we did not check if \"253\" overflows the type of\nold_offset, etc. And I would very much welcome patches to fix such\na careless input validation routine. But replacing ulong with size_t\nwould not make such a problem go away.\n\nNow, I would be a bit more sympathetic if the patch were to use\nintegers of exact sizes, in the name of \"let's make sure that\nregardless of the platforms we handle patches up to the same limit\".\nBut size_t is not a type that is appropriate for that (and of course\nulong is not, either---but the original did not aim for such a uniform\nlimit to begin with).\n\n> @@ -1626,7 +1628,7 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\telse\n> \t\t\t\terr(s, Q_(\"Sorry, only %d hunk available.\",\n> \t\t\t\t\t \"Sorry, only %d hunks available.\",\n> -\t\t\t\t\t file_diff->hunk_nr),\n> +\t\t\t\t\t (int)file_diff->hunk_nr),\n> \t\t\t\t (int)file_diff->hunk_nr);\n> \t\t} else if (s->answer.buf[0] == '/') {\n> \t\t\tregex_t regex;\n\nI skimmed your \"how about going this way\" illustration patch and\nfound all the hunks reasonable, but this one I am not sure. Is\nthere a reason why hunk_nr has to be of type size_t? \n\nWhen queuing a hunk (and performing an operation that changes the\nnumber of hunks, like splitting an existing one), the code should be\ncareful not to make too many hunks to overflow \"int\" (if that is the\nmore natural type to count them---and \"int\" being the most natural\ninteger type for the platform, I tend to think it should be fine),\nagain, that applies equally if the type of hunk_nr is \"size_t\".\n\nThanks.\n"},{"id":"509642","messageId":"965ac9bd-7340-4dbd-88da-2daa88c126c4@freenet.de","threadId":"62685","inReplyTo":"xmqq5xn5urhv.fsf@gitster.g","subject":"Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-12-27T16:35:56Z","receivedAt":"2024-12-27T16:36:10Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Hi everyone,\n\nIf I understand your comments correctly, it would be preferably to \nswitch to a data type like uint32_t or uint64_t so that the behavior is \nconsisted on all platforms?\nAlso add a test if the input overflows the data type.\n\nBest regards,\n\nSören Krecker\n\nJunio C Hamano writes:\n> Phillip Wood writes:\n> \n>>>> struct hunk_header {\n>>>> -\tunsigned long old_offset, old_count, new_offset, new_count;\n>>>> +\tsize_t old_offset, old_count, new_offset, new_count;\n>>> These are not \"size\"s in the traditional sense of what size_t is\n>>> (i.e. the number of bytes in a region of memory), but are more or\n>>> less proportional to that in that they count in number of lines.\n>>> If ulong is sufficient to count number of lines in an incoming\n>>> patch, then turning size_t may be excessive---are we sure that we\n>>> are not unnecessarily using wider-than-necessary size_t in some\n>>> places to hold these values for which ulong is sufficient, causing\n>>> compilers to emit unnecessary warning?\n>>\n>> That's my thought too - I think something like the diff below should\n>> fix the warnings by using more appropriate types in expressions\n>> involving the hunk header offset and count. Our internal diff\n>> implementation will not generate diffs for blobs greater than ~1GB\n>> and I don't think \"git apply\" can handle diff headers that contain\n>> numbers greater that ULONG_MAX so switching to size_t here seems\n>> unnecessary.\n> \n> Yes, exactly.\n> \n> Of course, when filling old_offset and friends by parsing an input\n> line like this:\n> \n> @@ -253,7 +253,7 @@ struct hunk_header {\n> \n> it would be a bug if we did not check if \"253\" overflows the type of\n> old_offset, etc. And I would very much welcome patches to fix such\n> a careless input validation routine. But replacing ulong with size_t\n> would not make such a problem go away.\n> \n> Now, I would be a bit more sympathetic if the patch were to use\n> integers of exact sizes, in the name of \"let's make sure that\n> regardless of the platforms we handle patches up to the same limit\".\n> But size_t is not a type that is appropriate for that (and of course\n> ulong is not, either---but the original did not aim for such a uniform\n> limit to begin with).\n> \n>> @@ -1626,7 +1628,7 @@ static int patch_update_file(struct add_p_state *s,\n>> \t\t\telse\n>> \t\t\t\terr(s, Q_(\"Sorry, only %d hunk available.\",\n>> \t\t\t\t\t \"Sorry, only %d hunks available.\",\n>> -\t\t\t\t\t file_diff->hunk_nr),\n>> +\t\t\t\t\t (int)file_diff->hunk_nr),\n>> \t\t\t\t (int)file_diff->hunk_nr);\n>> \t\t} else if (s->answer.buf[0] == '/') {\n>> \t\t\tregex_t regex;\n> \n> I skimmed your \"how about going this way\" illustration patch and\n> found all the hunks reasonable, but this one I am not sure. Is\n> there a reason why hunk_nr has to be of type size_t?\n> \n> When queuing a hunk (and performing an operation that changes the\n> number of hunks, like splitting an existing one), the code should be\n> careful not to make too many hunks to overflow \"int\" (if that is the\n> more natural type to count them---and \"int\" being the most natural\n> integer type for the platform, I tend to think it should be fine),\n> again, that applies equally if the type of hunk_nr is \"size_t\".\n> \n> Thanks.\n\n"},{"id":"509643","messageId":"xmqqfrm9t6up.fsf@gitster.g","threadId":"62685","inReplyTo":"965ac9bd-7340-4dbd-88da-2daa88c126c4@freenet.de","subject":"Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-27T16:42:54Z","receivedAt":"2024-12-27T16:42:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sören Krecker writes:\n\n> If I understand your comments correctly, it would be preferably to\n> switch to a data type like uint32_t or uint64_t so that the behavior\n> is consisted on all platforms?\n\nI personally wouldn't prefer that.\n\nI'd rather stick to some \"natural\" platform type like ulong. I see\nno strong need to say \"we must behave identically on all platforms\"\nin this area. It is preferrable to have every platform use the most\nnatural type on it, and make sure that we validate input that is too\nlarge to fit on each platform correctly (i.e. it is OK to diagnose\n\"too big a line number\" and die on 32-bit platform with much smaller\nline number than on 64-bit platform).\n\nThanks.\n\n"},{"id":"509683","messageId":"11a36c3d-d42c-45c5-bed7-0e40205d66ea@gmail.com","threadId":"62685","inReplyTo":"xmqq5xn5urhv.fsf@gitster.g","subject":"Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-12-28T16:04:11Z","receivedAt":"2024-12-28T16:04:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 27/12/2024 14:31, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> @@ -1626,7 +1628,7 @@ static int patch_update_file(struct add_p_state *s,\n>> \t\t\telse\n>> \t\t\t\terr(s, Q_(\"Sorry, only %d hunk available.\",\n>> \t\t\t\t\t \"Sorry, only %d hunks available.\",\n>> -\t\t\t\t\t file_diff->hunk_nr),\n>> +\t\t\t\t\t (int)file_diff->hunk_nr),\n>> \t\t\t\t (int)file_diff->hunk_nr);\n>> \t\t} else if (s->answer.buf[0] == '/') {\n>> \t\t\tregex_t regex;\n> \n> I skimmed your \"how about going this way\" illustration patch and\n> found all the hunks reasonable, but this one I am not sure. Is\n> there a reason why hunk_nr has to be of type size_t?\n\nWe certainly don't need to be able to hold that many hunks but changing \nit to a narrower type generates a truncation warning in ALLOC_GROW_BY() \nthat macro declares a local size_t variable to hold the new element \ncount and then assigns that to hunk_nr\n\n> When queuing a hunk (and performing an operation that changes the\n> number of hunks, like splitting an existing one), the code should be\n> careful not to make too many hunks to overflow \"int\" (if that is the\n> more natural type to count them---and \"int\" being the most natural\n> integer type for the platform, I tend to think it should be fine),\n\nYes it's hard to see anyone wanting to use \"git add -p\" on INT_MAX hunks\n\n> again, that applies equally if the type of hunk_nr is \"size_t\".\n\nIf we cast to \"unsigned long\" rather than \"int\" here then we'd be sure \nthat there was no overflow as we only support files with up to ULONG_MAX \nlines so there cannot be more than that number of hunks. \"unsigned long\" \nwould also match the prototype of ngettext().\n\nBest Wishes\n\nPhillip\n\n"}]}