{"thread":{"id":"61140","subject":"[PATCH 0/2] utf8: change return type of some helpers from int to size_t","startedAt":"2024-03-17T08:27:54Z","lastAt":"2024-03-21T10:30:29Z","messageCount":6,"participants":["Mohit Marathe via GitGitGadget","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"490819","messageId":"pull.1690.git.1710664071.gitgitgadget@gmail.com","threadId":"61140","inReplyTo":null,"subject":"[PATCH 0/2] utf8: change return type of some helpers from int to size_t","fromName":"Mohit Marathe via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-17T08:27:49Z","receivedAt":"2024-03-17T08:27:54Z","isPatch":true,"sender":{"key":"mohitmarathe@proton.me","avatar":"https://gravatar.com/avatar/3bf18f96e6d36cef960077143b9880e8b22468d960da18bf0b34c4e87d4df7e9?d=mp&s=160"},"body":"Hello,\n\nThere are lot of places in the codebase where int is used to store size of\nan object instead of size_t (which is better alternative due to reasons like\nportability, readability, etc). This patch series accomplishes one such\n#TODO comment, which addresses this issue.\n\nI appreciate your review and feedback on this patch series.\n\nBest regards, Mohit Marathe\n\nMohit Marathe (2):\n  utf8: change type from int to size_t\n  utf8: make utf8_strnwidth & utf8_strwidth return size_t instead of int\n\n builtin/blame.c    |  6 +++---\n builtin/branch.c   |  2 +-\n builtin/fetch.c    |  2 +-\n builtin/worktree.c |  4 ++--\n column.c           |  2 +-\n diff.c             |  8 +++++---\n gettext.c          |  2 +-\n gettext.h          |  2 +-\n pretty.c           |  4 ++--\n utf8.c             | 14 +++++---------\n utf8.h             |  4 ++--\n wt-status.c        |  4 ++--\n 12 files changed, 26 insertions(+), 28 deletions(-)\n\n\nbase-commit: 2953d95d402b6bff1a59c4712f4d46f1b9ea137f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1690%2Fmohit-marathe%2Fint-to-size_t-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1690/mohit-marathe/int-to-size_t-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1690\n-- \ngitgitgadget\n"},{"id":"490820","messageId":"2e89da2e72b460228b3f77b1a5168f0a1fe0adcf.1710664071.git.gitgitgadget@gmail.com","threadId":"61140","inReplyTo":"pull.1690.git.1710664071.gitgitgadget@gmail.com","subject":"[PATCH 1/2] utf8: change type from int to size_t","fromName":"Mohit Marathe via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-17T08:27:50Z","receivedAt":"2024-03-17T08:27:56Z","isPatch":true,"sender":{"key":"mohitmarathe@proton.me","avatar":"https://gravatar.com/avatar/3bf18f96e6d36cef960077143b9880e8b22468d960da18bf0b34c4e87d4df7e9?d=mp&s=160"},"body":"From: Mohit Marathe <mohitmarathe@proton.me>\n\nThis update modifies the variable types that are used in calls to the\n`utf8_strnwidth` and `utf8_strwidth` functions. This modification is\na proactive measure in anticipation of an upcoming interface change\nto these functions in the subsequent patch.\n\nSigned-off-by: Mohit Marathe <mohitmarathe@proton.me>\n---\n builtin/blame.c    | 6 +++---\n builtin/branch.c   | 2 +-\n builtin/fetch.c    | 2 +-\n builtin/worktree.c | 4 ++--\n column.c           | 2 +-\n diff.c             | 8 +++++---\n gettext.c          | 2 +-\n gettext.h          | 2 +-\n pretty.c           | 4 ++--\n utf8.c             | 4 ++--\n wt-status.c        | 4 ++--\n 11 files changed, 21 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex db1f56de61a..a72e2552305 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -53,7 +53,7 @@ static const char *annotate_opt_usage[] = {\n };\n \n static int longest_file;\n-static int longest_author;\n+static size_t longest_author;\n static int max_orig_digits;\n static int max_digits;\n static int max_score_digits;\n@@ -529,7 +529,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n \t\t\t\t\tname = ci.author_mail.buf;\n \t\t\t\telse\n \t\t\t\t\tname = ci.author.buf;\n-\t\t\t\tpad = longest_author - utf8_strwidth(name);\n+\t\t\t\tpad = cast_size_t_to_int(longest_author - utf8_strwidth(name));\n \t\t\t\tprintf(\" (%s%*s %10s\",\n \t\t\t\t       name, pad, \"\",\n \t\t\t\t       format_time(ci.author_time,\n@@ -631,7 +631,7 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)\n \n \tfor (e = sb->ent; e; e = e->next) {\n \t\tstruct blame_origin *suspect = e->suspect;\n-\t\tint num;\n+\t\tsize_t num;\n \n \t\tif (compute_auto_abbrev)\n \t\t\tauto_abbrev = update_auto_abbrev(auto_abbrev, suspect);\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 8c2305ad2c8..321c3558f2d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -349,7 +349,7 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)\n \tfor (i = 0; i < refs->nr; i++) {\n \t\tstruct ref_array_item *it = refs->items[i];\n \t\tconst char *desc = it->refname;\n-\t\tint w;\n+\t\tsize_t w;\n \n \t\tskip_prefix(it->refname, \"refs/heads/\", &desc);\n \t\tskip_prefix(it->refname, \"refs/remotes/\", &desc);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 46a793411a4..fee992c3c14 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -705,7 +705,7 @@ static int refcol_width(const struct ref *ref_map, int compact_format)\n \t\tmax = max * 2 / 3;\n \n \tfor (ref = ref_map; ref; ref = ref->next) {\n-\t\tint rlen, llen = 0, len;\n+\t\tsize_t rlen, llen = 0, len;\n \n \t\tif (ref->status == REF_STATUS_REJECT_SHALLOW ||\n \t\t    !ref->peer_ref ||\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 9c76b62b02d..bdbf46fb658 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -961,8 +961,8 @@ static void show_worktree_porcelain(struct worktree *wt, int line_terminator)\n static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint cur_path_len = strlen(wt->path);\n-\tint path_adj = cur_path_len - utf8_strwidth(wt->path);\n+\tsize_t cur_path_len = strlen(wt->path);\n+\tint path_adj = cast_size_t_to_int(cur_path_len - utf8_strwidth(wt->path));\n \tconst char *reason;\n \n \tstrbuf_addf(&sb, \"%-*s \", 1 + path_maxlen + path_adj, wt->path);\ndiff --git a/column.c b/column.c\nindex 50bbccc92ee..ec874036de6 100644\n--- a/column.c\n+++ b/column.c\n@@ -22,7 +22,7 @@ struct column_data {\n };\n \n /* return length of 's' in letters, ANSI escapes stripped */\n-static int item_length(const char *s)\n+static size_t item_length(const char *s)\n {\n \treturn utf8_strnwidth(s, strlen(s), 1);\n }\ndiff --git a/diff.c b/diff.c\nindex e50def45383..4faf151345a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2629,7 +2629,8 @@ void print_stat_summary(FILE *fp, int files,\n \n static void show_stats(struct diffstat_t *data, struct diff_options *options)\n {\n-\tint i, len, add, del, adds = 0, dels = 0;\n+\tint i, add, del, adds = 0, dels = 0;\n+\tsize_t len = 0;\n \tuintmax_t max_change = 0, max_len = 0;\n \tint total_files = data->nr, count;\n \tint width, name_width, graph_width, number_width = 0, bin_width = 0;\n@@ -2780,7 +2781,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tchar *name = file->print_name;\n \t\tuintmax_t added = file->added;\n \t\tuintmax_t deleted = file->deleted;\n-\t\tint name_len, padding;\n+\t\tsize_t name_len;\n+\t\tint padding;\n \n \t\tif (!file->is_interesting && (added + deleted == 0))\n \t\t\tcontinue;\n@@ -2809,7 +2811,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tif (slash)\n \t\t\t\tname = slash;\n \t\t}\n-\t\tpadding = len - utf8_strwidth(name);\n+\t\tpadding = cast_size_t_to_int(len - utf8_strwidth(name));\n \t\tif (padding < 0)\n \t\t\tpadding = 0;\n \ndiff --git a/gettext.c b/gettext.c\nindex 57facbc21ec..5a77b4f7202 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -127,7 +127,7 @@ void git_setup_gettext(void)\n }\n \n /* return the number of columns of string 's' in current locale */\n-int gettext_width(const char *s)\n+size_t gettext_width(const char *s)\n {\n \tstatic int is_utf8 = -1;\n \tif (is_utf8 == -1)\ndiff --git a/gettext.h b/gettext.h\nindex 484cafa5628..f161a21b45c 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -31,7 +31,7 @@\n #ifndef NO_GETTEXT\n extern int git_gettext_enabled;\n void git_setup_gettext(void);\n-int gettext_width(const char *s);\n+size_t gettext_width(const char *s);\n #else\n #define git_gettext_enabled (0)\n static inline void git_setup_gettext(void)\ndiff --git a/pretty.c b/pretty.c\nindex bdbed4295aa..f03493c74b1 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1781,7 +1781,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n \n \tif (padding < 0) {\n \t\tconst char *start = strrchr(sb->buf, '\\n');\n-\t\tint occupied;\n+\t\tsize_t occupied;\n \t\tif (!start)\n \t\t\tstart = sb->buf;\n \t\toccupied = utf8_strnwidth(start, strlen(start), 1);\n@@ -1802,7 +1802,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n \t\tplaceholder++;\n \t\ttotal_consumed++;\n \t}\n-\tlen = utf8_strnwidth(local_sb.buf, local_sb.len, 1);\n+\tlen = cast_size_t_to_int(utf8_strnwidth(local_sb.buf, local_sb.len, 1));\n \n \tif (c->flush_type == flush_left_and_steal) {\n \t\tconst char *ch = sb->buf + sb->len - 1;\ndiff --git a/utf8.c b/utf8.c\nindex 6bfaefa28eb..8ccdf684e07 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -466,7 +466,7 @@ int utf8_fprintf(FILE *stream, const char *format, ...)\n \n \tcolumns = fputs(buf.buf, stream);\n \tif (0 <= columns) /* keep the error from the I/O */\n-\t\tcolumns = utf8_strwidth(buf.buf);\n+\t\tcolumns = cast_size_t_to_int(utf8_strwidth(buf.buf));\n \tstrbuf_release(&buf);\n \treturn columns;\n }\n@@ -806,7 +806,7 @@ void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int wid\n \t\t       const char *s)\n {\n \tsize_t slen = strlen(s);\n-\tint display_len = utf8_strnwidth(s, slen, 0);\n+\tsize_t display_len = utf8_strnwidth(s, slen, 0);\n \tint utf8_compensation = slen - display_len;\n \n \tif (display_len >= width) {\ndiff --git a/wt-status.c b/wt-status.c\nindex 7108a92b52c..c847b4bb5ed 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -325,7 +325,7 @@ static int maxwidth(const char *(*label)(int), int minval, int maxval)\n \n \tfor (i = minval; i <= maxval; i++) {\n \t\tconst char *s = label(i);\n-\t\tint len = s ? utf8_strwidth(s) : 0;\n+\t\tsize_t len = s ? utf8_strwidth(s) : 0;\n \t\tif (len > result)\n \t\t\tresult = len;\n \t}\n@@ -341,7 +341,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,\n \tstatic char *padding;\n \tstatic int label_width;\n \tconst char *one, *how;\n-\tint len;\n+\tsize_t len;\n \n \tif (!padding) {\n \t\tlabel_width = maxwidth(wt_status_unmerged_status_string, 1, 7);\n-- \ngitgitgadget\n\n"},{"id":"490821","messageId":"615481efd70fbd7e8950bea3edca12e43a024cd8.1710664071.git.gitgitgadget@gmail.com","threadId":"61140","inReplyTo":"pull.1690.git.1710664071.gitgitgadget@gmail.com","subject":"[PATCH 2/2] utf8: make utf8_strnwidth & utf8_strwidth return size_t instead of int","fromName":"Mohit Marathe via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-17T08:27:51Z","receivedAt":"2024-03-17T08:27:56Z","isPatch":true,"sender":{"key":"mohitmarathe@proton.me","avatar":"https://gravatar.com/avatar/3bf18f96e6d36cef960077143b9880e8b22468d960da18bf0b34c4e87d4df7e9?d=mp&s=160"},"body":"From: Mohit Marathe <mohitmarathe@proton.me>\n\nThis patch addresses the TODO comment of changing the return types\nof these functions from int to size_t.\n\nSigned-off-by: Mohit Marathe <mohitmarathe@proton.me>\n---\n utf8.c | 10 +++-------\n utf8.h |  4 ++--\n 2 files changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex 8ccdf684e07..050fc8b3cdf 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -206,7 +206,7 @@ int utf8_width(const char **start, size_t *remainder_p)\n  * string, assuming that the string is utf8.  Returns strlen() instead\n  * if the string does not look like a valid utf8 string.\n  */\n-int utf8_strnwidth(const char *string, size_t len, int skip_ansi)\n+size_t utf8_strnwidth(const char *string, size_t len, int skip_ansi)\n {\n \tconst char *orig = string;\n \tsize_t width = 0;\n@@ -224,14 +224,10 @@ int utf8_strnwidth(const char *string, size_t len, int skip_ansi)\n \t\t\twidth += glyph_width;\n \t}\n \n-\t/*\n-\t * TODO: fix the interface of this function and `utf8_strwidth()` to\n-\t * return `size_t` instead of `int`.\n-\t */\n-\treturn cast_size_t_to_int(string ? width : len);\n+\treturn string ? width : len;\n }\n \n-int utf8_strwidth(const char *string)\n+size_t utf8_strwidth(const char *string)\n {\n \treturn utf8_strnwidth(string, strlen(string), 0);\n }\ndiff --git a/utf8.h b/utf8.h\nindex 35df76086a6..cae10d5ac1f 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -7,8 +7,8 @@ typedef unsigned int ucs_char_t;  /* assuming 32bit int */\n \n size_t display_mode_esc_sequence_len(const char *s);\n int utf8_width(const char **start, size_t *remainder_p);\n-int utf8_strnwidth(const char *string, size_t len, int skip_ansi);\n-int utf8_strwidth(const char *string);\n+size_t utf8_strnwidth(const char *string, size_t len, int skip_ansi);\n+size_t utf8_strwidth(const char *string);\n int is_utf8(const char *text);\n int is_encoding_utf8(const char *name);\n int same_encoding(const char *, const char *);\n-- \ngitgitgadget\n"},{"id":"490822","messageId":"xmqq7ci0n6mx.fsf@gitster.g","threadId":"61140","inReplyTo":"pull.1690.git.1710664071.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] utf8: change return type of some helpers from int to size_t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-17T16:51:50Z","receivedAt":"2024-03-17T16:51:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Mohit Marathe via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> There are lot of places in the codebase where int is used to store size of\n> an object instead of size_t (which is better alternative due to reasons like\n> portability, readability, etc). This patch series accomplishes one such\n> #TODO comment, which addresses this issue.\n\nNice. In principle this is the right thing to do.\n\nTo reviewers, if I were reviewing this series (which I will not in\nthe next 24 hours), I'd pay particular attention to the callers that\ncurrently accept \"int\" and use negative return value as an sign of a\nfailure.  Moving to \"size_t\" from \"int\" will break such callers.\n\nThanks.\n"},{"id":"491117","messageId":"ZfwMO-6IEXDm3z1b@tanuki","threadId":"61140","inReplyTo":"2e89da2e72b460228b3f77b1a5168f0a1fe0adcf.1710664071.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] utf8: change type from int to size_t","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T10:30:19Z","receivedAt":"2024-03-21T10:30:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Mar 17, 2024 at 08:27:50AM +0000, Mohit Marathe via GitGitGadget wrote:\n> From: Mohit Marathe <mohitmarathe@proton.me>\n> \n> This update modifies the variable types that are used in calls to the\n> `utf8_strnwidth` and `utf8_strwidth` functions. This modification is\n> a proactive measure in anticipation of an upcoming interface change\n> to these functions in the subsequent patch.\n\nIt would help to mention `utf8_str{,n}width()` in the commit subject so\nthat it's immediately obvious which functions get touched by this\ncommit.\n\n> Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>\n> ---\n>  builtin/blame.c    | 6 +++---\n>  builtin/branch.c   | 2 +-\n>  builtin/fetch.c    | 2 +-\n>  builtin/worktree.c | 4 ++--\n>  column.c           | 2 +-\n>  diff.c             | 8 +++++---\n>  gettext.c          | 2 +-\n>  gettext.h          | 2 +-\n>  pretty.c           | 4 ++--\n>  utf8.c             | 4 ++--\n>  wt-status.c        | 4 ++--\n>  11 files changed, 21 insertions(+), 19 deletions(-)\n> \n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index db1f56de61a..a72e2552305 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -53,7 +53,7 @@ static const char *annotate_opt_usage[] = {\n>  };\n>  \n>  static int longest_file;\n\nWe also end up assigning `longest_file = num`, where `num` became a\n`size_t` now. We should likely change this variable's type to `size_t`\nas well.\n\n> -static int longest_author;\n> +static size_t longest_author;\n>  static int max_orig_digits;\n>  static int max_digits;\n>  static int max_score_digits;\n> @@ -529,7 +529,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n>  \t\t\t\t\tname = ci.author_mail.buf;\n>  \t\t\t\telse\n>  \t\t\t\t\tname = ci.author.buf;\n> -\t\t\t\tpad = longest_author - utf8_strwidth(name);\n> +\t\t\t\tpad = cast_size_t_to_int(longest_author - utf8_strwidth(name));\n\nI feel like this cast here is somewhat dubious. Both `longest_author`\nand the return value of `utf8_strwidth()` are `size_t` now. So an\nalternative way to handle this would be to assign the return value to a\ntemporary variable and then `if (longest_author > name_len) BUG(...)`.\n\n>  \t\t\t\tprintf(\" (%s%*s %10s\",\n>  \t\t\t\t       name, pad, \"\",\n>  \t\t\t\t       format_time(ci.author_time,\n> @@ -631,7 +631,7 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)\n\nA few lines before this hunk we declare `longest_src_lines` and\n`longest_dst_lines` as `int`, but we assign `num` to them. We should fix\nthose to become `size_t`, as well. This will bubble up so that we also\nhave to fix `max_orig_digits` and `max_digits`.\n\n>  \tfor (e = sb->ent; e; e = e->next) {\n>  \t\tstruct blame_origin *suspect = e->suspect;\n> -\t\tint num;\n> +\t\tsize_t num;\n>  \n>  \t\tif (compute_auto_abbrev)\n>  \t\t\tauto_abbrev = update_auto_abbrev(auto_abbrev, suspect);\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 8c2305ad2c8..321c3558f2d 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -349,7 +349,7 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)\n>  \tfor (i = 0; i < refs->nr; i++) {\n>  \t\tstruct ref_array_item *it = refs->items[i];\n>  \t\tconst char *desc = it->refname;\n> -\t\tint w;\n> +\t\tsize_t w;\n\nThis isn't sufficient as we end up assignign `max = w`, where `max` is\ndeclared as `int`. We would thus also have to change that variable and\nthen change the return value of `calc_maxwidth()` itself to be `size_t`.\n\nFurthermore, we do `w += remote_bonus` in this function, where\n`remote_bonus` is of type `int`. Is it guaranteed that this variable is\nalways positive? If so, it should likely become a `size_t`, too.\nOtherwise we'll need to have some safety checks that the result does not\nwrap.\n\n>  \t\tskip_prefix(it->refname, \"refs/heads/\", &desc);\n>  \t\tskip_prefix(it->refname, \"refs/remotes/\", &desc);\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 46a793411a4..fee992c3c14 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -705,7 +705,7 @@ static int refcol_width(const struct ref *ref_map, int compact_format)\n>  \t\tmax = max * 2 / 3;\n>  \n>  \tfor (ref = ref_map; ref; ref = ref->next) {\n> -\t\tint rlen, llen = 0, len;\n> +\t\tsize_t rlen, llen = 0, len;\n\nWe assign `width = rlen` further down, but `width` is an `int`. It\nshould likely be changed to become a `size_t` as well, which will\nrequire us to change the return value of this function.\n\n>  \t\tif (ref->status == REF_STATUS_REJECT_SHALLOW ||\n>  \t\t    !ref->peer_ref ||\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 9c76b62b02d..bdbf46fb658 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -961,8 +961,8 @@ static void show_worktree_porcelain(struct worktree *wt, int line_terminator)\n>  static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n>  {\n>  \tstruct strbuf sb = STRBUF_INIT;\n> -\tint cur_path_len = strlen(wt->path);\n> -\tint path_adj = cur_path_len - utf8_strwidth(wt->path);\n> +\tsize_t cur_path_len = strlen(wt->path);\n> +\tint path_adj = cast_size_t_to_int(cur_path_len - utf8_strwidth(wt->path));\n>  \tconst char *reason;\n\nI was wondering whether there should be a sanity check here for whether\n`cur_path_len >= path_adj`. But given that both measure the length of\n`wt->path`, and given that `utf8_strwidth()` should only ever return a\nvalue smaller than the original length, I think this should be fine.\n\n>  \tstrbuf_addf(&sb, \"%-*s \", 1 + path_maxlen + path_adj, wt->path);\n> diff --git a/column.c b/column.c\n> index 50bbccc92ee..ec874036de6 100644\n> --- a/column.c\n> +++ b/column.c\n> @@ -22,7 +22,7 @@ struct column_data {\n>  };\n>  \n>  /* return length of 's' in letters, ANSI escapes stripped */\n> -static int item_length(const char *s)\n> +static size_t item_length(const char *s)\n>  {\n>  \treturn utf8_strnwidth(s, strlen(s), 1);\n>  }\n\nThe value of this function is assigned to `struct column_data::len`,\nwhich is an array of `int`. That type needs to change.\n\n> diff --git a/diff.c b/diff.c\n> index e50def45383..4faf151345a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2629,7 +2629,8 @@ void print_stat_summary(FILE *fp, int files,\n>  \n>  static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>  {\n> -\tint i, len, add, del, adds = 0, dels = 0;\n> +\tint i, add, del, adds = 0, dels = 0;\n> +\tsize_t len = 0;\n>  \tuintmax_t max_change = 0, max_len = 0;\n\nIt's somewhat weird that `max_len` is declared as `uintmax_t`. Should it\nbecome a `size_t`, as well?\n\n>  \tint total_files = data->nr, count;\n>  \tint width, name_width, graph_width, number_width = 0, bin_width = 0;\n> @@ -2780,7 +2781,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>  \t\tchar *name = file->print_name;\n>  \t\tuintmax_t added = file->added;\n>  \t\tuintmax_t deleted = file->deleted;\n> -\t\tint name_len, padding;\n> +\t\tsize_t name_len;\n\nWe compare `name_len` with `name_width`. The latter should likely become\na `size_t`, as well.\n\n> +\t\tint padding;\n>  \n>  \t\tif (!file->is_interesting && (added + deleted == 0))\n>  \t\t\tcontinue;\n> @@ -2809,7 +2811,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>  \t\t\tif (slash)\n>  \t\t\t\tname = slash;\n>  \t\t}\n> -\t\tpadding = len - utf8_strwidth(name);\n> +\t\tpadding = cast_size_t_to_int(len - utf8_strwidth(name));\n\nI think we shouldn't abuse `cast_size_t_to_int()` like this but instead\nexplicitly check whether `len >= len(name)`.\n\n>  \t\tif (padding < 0)\n>  \t\t\tpadding = 0;\n>  \n> diff --git a/gettext.c b/gettext.c\n> index 57facbc21ec..5a77b4f7202 100644\n> --- a/gettext.c\n> +++ b/gettext.c\n> @@ -127,7 +127,7 @@ void git_setup_gettext(void)\n>  }\n>  \n>  /* return the number of columns of string 's' in current locale */\n> -int gettext_width(const char *s)\n> +size_t gettext_width(const char *s)\n\nWe also need to adjust the callsite of this function. There's only a\nsingle one in \"builtin/fetch.c::display_ref_update()\".\n\n>  {\n>  \tstatic int is_utf8 = -1;\n>  \tif (is_utf8 == -1)\n> diff --git a/gettext.h b/gettext.h\n> index 484cafa5628..f161a21b45c 100644\n> --- a/gettext.h\n> +++ b/gettext.h\n> @@ -31,7 +31,7 @@\n>  #ifndef NO_GETTEXT\n>  extern int git_gettext_enabled;\n>  void git_setup_gettext(void);\n> -int gettext_width(const char *s);\n> +size_t gettext_width(const char *s);\n>  #else\n>  #define git_gettext_enabled (0)\n>  static inline void git_setup_gettext(void)\n> diff --git a/pretty.c b/pretty.c\n> index bdbed4295aa..f03493c74b1 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1781,7 +1781,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n>  \n>  \tif (padding < 0) {\n>  \t\tconst char *start = strrchr(sb->buf, '\\n');\n> -\t\tint occupied;\n> +\t\tsize_t occupied;\n>  \t\tif (!start)\n>  \t\t\tstart = sb->buf;\n>  \t\toccupied = utf8_strnwidth(start, strlen(start), 1);\n\nWe're assigning `padding = (-padding) - occupied` next, where `padding`\nis of type `int`. We should add some safety checks here.\n\n> @@ -1802,7 +1802,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n>  \t\tplaceholder++;\n>  \t\ttotal_consumed++;\n>  \t}\n> -\tlen = utf8_strnwidth(local_sb.buf, local_sb.len, 1);\n> +\tlen = cast_size_t_to_int(utf8_strnwidth(local_sb.buf, local_sb.len, 1));\n\nWhy did you decide to use `cast_size_t_to_int()` instead of converting\n`len` to be of type `size_t`?\n\n>  \tif (c->flush_type == flush_left_and_steal) {\n>  \t\tconst char *ch = sb->buf + sb->len - 1;\n> diff --git a/utf8.c b/utf8.c\n> index 6bfaefa28eb..8ccdf684e07 100644\n> --- a/utf8.c\n> +++ b/utf8.c\n> @@ -466,7 +466,7 @@ int utf8_fprintf(FILE *stream, const char *format, ...)\n>  \n>  \tcolumns = fputs(buf.buf, stream);\n>  \tif (0 <= columns) /* keep the error from the I/O */\n> -\t\tcolumns = utf8_strwidth(buf.buf);\n> +\t\tcolumns = cast_size_t_to_int(utf8_strwidth(buf.buf));\n>  \tstrbuf_release(&buf);\n>  \treturn columns;\n>  }\n> @@ -806,7 +806,7 @@ void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int wid\n>  \t\t       const char *s)\n>  {\n>  \tsize_t slen = strlen(s);\n> -\tint display_len = utf8_strnwidth(s, slen, 0);\n> +\tsize_t display_len = utf8_strnwidth(s, slen, 0);\n>  \tint utf8_compensation = slen - display_len;\n>  \n>  \tif (display_len >= width) {\n\nA few lines further down we have:\n\n    int left = (width - dispay_len) / 2;\n\nThis should be changed to become a `size_t`.\n\n> diff --git a/wt-status.c b/wt-status.c\n> index 7108a92b52c..c847b4bb5ed 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -325,7 +325,7 @@ static int maxwidth(const char *(*label)(int), int minval, int maxval)\n>  \n>  \tfor (i = minval; i <= maxval; i++) {\n>  \t\tconst char *s = label(i);\n> -\t\tint len = s ? utf8_strwidth(s) : 0;\n> +\t\tsize_t len = s ? utf8_strwidth(s) : 0;\n>  \t\tif (len > result)\n>  \t\t\tresult = len;\n\n`result` is of type `int`, so we'd have to change both its type as well\nas the return type of `maxwidth()`.\n\n>  \t}\n> @@ -341,7 +341,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,\n>  \tstatic char *padding;\n>  \tstatic int label_width;\n>  \tconst char *one, *how;\n> -\tint len;\n> +\tsize_t len;\n>  \n>  \tif (!padding) {\n>  \t\tlabel_width = maxwidth(wt_status_unmerged_status_string, 1, 7);\n\n`len` gets passed to the formatter \"%.*s\". Quoting fprintf(3p):\n\n    A field width, or precision, or both, may be indicated by an\n    <asterisk> ('*'). In this case an argument of type int supplies the\n    field width or precision.\n\nSo this must be an `int` or otherwise bad things will happen depending\non the size of those types on your platform.\n\nPatrick\n"},{"id":"491118","messageId":"ZfwMQdzvyRMAIvtB@tanuki","threadId":"61140","inReplyTo":"pull.1690.git.1710664071.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] utf8: change return type of some helpers from int to size_t","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T10:30:25Z","receivedAt":"2024-03-21T10:30:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Mar 17, 2024 at 08:27:49AM +0000, Mohit Marathe via GitGitGadget wrote:\n> Hello,\n> \n> There are lot of places in the codebase where int is used to store size of\n> an object instead of size_t (which is better alternative due to reasons like\n> portability, readability, etc). This patch series accomplishes one such\n> #TODO comment, which addresses this issue.\n> \n> I appreciate your review and feedback on this patch series.\n\nI really appreciate you working on this -- our mismatch of types really\nirks me a ton, so I'm very happy to see some fixes. I've provided a\nbunch of feedback for your patch 1/2, where we need to be quite a bit\nmore careful about the blast radius of each of the changes.\n\nI'd recommend to split that patch up into multiple parts. While each of\nthe changes is basically only changing some types around, it's rather\ninvolved to review as you have to also investigate the vicinity of each\nchange quite vigorously. So at the very least I would recommend to split\nout patches for every change that requires further changes, e.g. to the\nreturn types of additional functions.\n\nThanks!\n\nPatrick\n"}]}