Re: [PATCH v3] utf8: make utf8_strwidth() and utf8_strnwidth() return size_t
Phillip Wood <[email protected]>
| Newsgroups | org.kernel.vger.git |
|---|---|
| Message-ID | <[email protected]> |
Hi Hardik
On 27/07/2026 07:59, Hardik Kumar wrote:
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 48d5251..83e4dd6 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -564,7 +564,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent,
> name = ci.author_mail.buf;
> else
> name = ci.author.buf;
> - pad = longest_author - utf8_strwidth(name);
> + pad = longest_author - cast_size_t_to_int(utf8_strwidth(name));
> printf(" (%s%*s %10s",
> name, pad, "",
> format_time(ci.author_time,
To me this example perfectly illustrates why changing the return value
of utf8_strwidth() is a bad idea. The return value is pretty much always
used to calculate a padding to pass to printf() which expects an int. By
changing the return value you're forcing all the callers to do the
conversion themselves which is a bug waiting to happen. I'm also far
from convinced that the conversions in this patch are complete: grepping
for 'utf8_strn\{0,1\}width' turns up several calls which do not appear
to be correctly converted here. For example:
builtin/worktree.c: display[i].width = utf8_strwidth(buf.buf);
where "width" is an int.
I think it would be much better to remove the TODO comment as Junio
previously suggested and instead add some documentation to the function
explaining (a) why it is appropriate for it to return an int; (b) why we
must use the cast_size_t_to_int() helper to prevent overflows (see the
commit that added that comment).
Thanks
Phillip
> @@ -685,9 +685,9 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)
> suspect->commit->object.flags |= METAINFO_SHOWN;
> get_commit_info(suspect->commit, &ci);
> if (*option & OUTPUT_SHOW_EMAIL)
> - num = utf8_strwidth(ci.author_mail.buf);
> + num = cast_size_t_to_int(utf8_strwidth(ci.author_mail.buf));
> else
> - num = utf8_strwidth(ci.author.buf);
> + num = cast_size_t_to_int(utf8_strwidth(ci.author.buf));
> if (longest_author < num)
> longest_author = num;
> commit_info_destroy(&ci);
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 775a797..c4ae95f 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -850,7 +850,7 @@ static void display_ref_update(struct display_state *display_state, char code,
> display_state->shown_url = 1;
> }
>
> - width = (summary_width + strlen(summary) - gettext_width(summary));
> + width = (summary_width + strlen(summary) - cast_size_t_to_int(gettext_width(summary)));
> remote = prettify_refname(remote);
> local = prettify_refname(local);
>
> diff --git a/builtin/repo.c b/builtin/repo.c
> index 84e012f..9c7ad8c 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -371,7 +371,7 @@ static void stats_table_vaddf(struct stats_table *table,
>
> strbuf_vaddf(&buf, format, ap);
> formatted_name = strbuf_detach(&buf, NULL);
> - name_width = utf8_strwidth(formatted_name);
> + name_width = cast_size_t_to_int(utf8_strwidth(formatted_name));
>
> item = string_list_append_nodup(&table->rows, formatted_name);
> item->util = entry;
> @@ -387,12 +387,12 @@ static void stats_table_vaddf(struct stats_table *table,
> string_list_append_nodup(&table->annotations, strbuf_detach(&buf, NULL));
> }
> if (entry->value) {
> - int value_width = utf8_strwidth(entry->value);
> + int value_width = cast_size_t_to_int(utf8_strwidth(entry->value));
> if (value_width > table->value_col_width)
> table->value_col_width = value_width;
> }
> if (entry->unit) {
> - int unit_width = utf8_strwidth(entry->unit);
> + int unit_width = cast_size_t_to_int(utf8_strwidth(entry->unit));
> if (unit_width > table->unit_col_width)
> table->unit_col_width = unit_width;
> }
> @@ -582,8 +582,8 @@ static void stats_table_print_structure(const struct stats_table *table)
> {
> const char *name_col_title = _("Repository structure");
> const char *value_col_title = _("Value");
> - int title_name_width = utf8_strwidth(name_col_title);
> - int title_value_width = utf8_strwidth(value_col_title);
> + int title_name_width = cast_size_t_to_int(utf8_strwidth(name_col_title));
> + int title_value_width = cast_size_t_to_int(utf8_strwidth(value_col_title));
> int name_col_width = table->name_col_width;
> int value_col_width = table->value_col_width;
> int unit_col_width = table->unit_col_width;
> diff --git a/column.c b/column.c
> index 93fae31..a63d040 100644
> --- a/column.c
> +++ b/column.c
> @@ -26,7 +26,7 @@ struct column_data {
> /* return length of 's' in letters, ANSI escapes stripped */
> static int item_length(const char *s)
> {
> - return utf8_strnwidth(s, strlen(s), 1);
> + return cast_size_t_to_int(utf8_strnwidth(s, strlen(s), 1));
> }
>
> /*
> diff --git a/diff.c b/diff.c
> index 589c196..205fedf 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2982,7 +2982,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> continue;
> }
> fill_print_name(file);
> - len = utf8_strwidth(file->print_name);
> + len = cast_size_t_to_int(utf8_strwidth(file->print_name));
> if (max_len < len)
> max_len = len;
>
> @@ -3037,7 +3037,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> * making the line longer than the maximum width.
> */
> if (options->stat_width == -1)
> - width = term_columns() - utf8_strnwidth(line_prefix, strlen(line_prefix), 1);
> + width = term_columns() - cast_size_t_to_int(utf8_strnwidth(line_prefix, strlen(line_prefix), 1));
> else
> width = options->stat_width ? options->stat_width : 80;
> number_width = decimal_width(max_change) > number_width ?
> @@ -3108,7 +3108,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> * "scale" the filename
> */
> len = name_width;
> - name_len = utf8_strwidth(name);
> + name_len = cast_size_t_to_int(utf8_strwidth(name));
> if (name_width < name_len) {
> char *slash;
> prefix = "...";
> @@ -3123,7 +3123,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> if (slash)
> name = slash;
> }
> - padding = len - utf8_strwidth(name);
> + padding = len - cast_size_t_to_int(utf8_strwidth(name));
> if (padding < 0)
> padding = 0;
>
> diff --git a/gettext.c b/gettext.c
> index 8d08a61..4d5d05e 100644
> --- a/gettext.c
> +++ b/gettext.c
> @@ -129,7 +129,7 @@ void git_setup_gettext(void)
> }
>
> /* return the number of columns of string 's' in current locale */
> -int gettext_width(const char *s)
> +size_t gettext_width(const char *s)
> {
> static int is_utf8 = -1;
> if (is_utf8 == -1)
> diff --git a/gettext.h b/gettext.h
> index 484cafa..f161a21 100644
> --- a/gettext.h
> +++ b/gettext.h
> @@ -31,7 +31,7 @@
> #ifndef NO_GETTEXT
> extern int git_gettext_enabled;
> void git_setup_gettext(void);
> -int gettext_width(const char *s);
> +size_t gettext_width(const char *s);
> #else
> #define git_gettext_enabled (0)
> static inline void git_setup_gettext(void)
> diff --git a/pretty.c b/pretty.c
> index d8a9f37..83d4e86 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1809,7 +1809,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
>
> if (padding < 0) {
> const char *start = strrchr(sb->buf, '\n');
> - int occupied;
> + size_t occupied;
> if (!start)
> start = sb->buf;
> occupied = utf8_strnwidth(start, strlen(start), 1);
> @@ -1830,7 +1830,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
> placeholder++;
> total_consumed++;
> }
> - len = utf8_strnwidth(local_sb.buf, local_sb.len, 1);
> + len = cast_size_t_to_int(utf8_strnwidth(local_sb.buf, local_sb.len, 1));
>
> if (c->flush_type == flush_left_and_steal) {
> const char *ch = sb->buf + sb->len - 1;
> diff --git a/utf8.c b/utf8.c
> index 96460cc..cefaefe 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -208,7 +208,7 @@ int utf8_width(const char **start, size_t *remainder_p)
> * string, assuming that the string is utf8. Returns strlen() instead
> * if the string does not look like a valid utf8 string.
> */
> -int utf8_strnwidth(const char *string, size_t len, int skip_ansi)
> +size_t utf8_strnwidth(const char *string, size_t len, int skip_ansi)
> {
> const char *orig = string;
> size_t width = 0;
> @@ -225,15 +225,10 @@ int utf8_strnwidth(const char *string, size_t len, int skip_ansi)
> if (glyph_width > 0)
> width += glyph_width;
> }
> -
> - /*
> - * TODO: fix the interface of this function and `utf8_strwidth()` to
> - * return `size_t` instead of `int`.
> - */
> - return cast_size_t_to_int(string ? width : len);
> + return string ? width : len;
> }
>
> -int utf8_strwidth(const char *string)
> +size_t utf8_strwidth(const char *string)
> {
> return utf8_strnwidth(string, strlen(string), 0);
> }
> @@ -821,7 +816,7 @@ void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int wid
> const char *s)
> {
> size_t slen = strlen(s);
> - int display_len = utf8_strnwidth(s, slen, 0);
> + size_t display_len = utf8_strnwidth(s, slen, 0);
> int utf8_compensation = slen - display_len;
>
> if (display_len >= width) {
> diff --git a/utf8.h b/utf8.h
> index cf8ecb0..531e968 100644
> --- a/utf8.h
> +++ b/utf8.h
> @@ -7,8 +7,8 @@ typedef unsigned int ucs_char_t; /* assuming 32bit int */
>
> size_t display_mode_esc_sequence_len(const char *s);
> int utf8_width(const char **start, size_t *remainder_p);
> -int utf8_strnwidth(const char *string, size_t len, int skip_ansi);
> -int utf8_strwidth(const char *string);
> +size_t utf8_strnwidth(const char *string, size_t len, int skip_ansi);
> +size_t utf8_strwidth(const char *string);
> int is_utf8(const char *text);
> int is_encoding_utf8(const char *name);
> int same_encoding(const char *, const char *);
> diff --git a/wt-status.c b/wt-status.c
> index 58461e0..672f83b 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -325,13 +325,13 @@ static const char *wt_status_diff_status_string(int status)
> }
> }
>
> -static int maxwidth(const char *(*label)(int), int minval, int maxval)
> +static size_t maxwidth(const char *(*label)(int), int minval, int maxval)
> {
> int result = 0, i;
>
> for (i = minval; i <= maxval; i++) {
> const char *s = label(i);
> - int len = s ? utf8_strwidth(s) : 0;
> + size_t len = s ? utf8_strwidth(s) : 0;
> if (len > result)
> result = len;
> }
> @@ -345,7 +345,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,
> struct wt_status_change_data *d = it->util;
> struct strbuf onebuf = STRBUF_INIT;
> static char *padding;
> - static int label_width;
> + static size_t label_width;
> const char *one, *how;
> int len;
>
> @@ -360,7 +360,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,
> status_printf(s, color(WT_STATUS_HEADER, s), "\t");
>
> how = wt_status_unmerged_status_string(d->stagemask);
> - len = label_width - utf8_strwidth(how);
> + len = label_width - cast_size_t_to_int(utf8_strwidth(how));
> status_printf_more(s, c, "%s%.*s%s\n", how, len, padding, one);
> strbuf_release(&onebuf);
> }
> @@ -429,7 +429,7 @@ static void wt_longstatus_print_change_data(struct wt_status *s,
> what = wt_status_diff_status_string(status);
> if (!what)
> BUG("unhandled diff status %c", status);
> - len = label_width - utf8_strwidth(what);
> + len = label_width - cast_size_t_to_int(utf8_strwidth(what));
> assert(len >= 0);
> if (one_name != two_name)
> status_printf_more(s, c, "%s%.*s%s -> %s",
>
> base-commit: 9a0c4701dcd5725c4184599322b52933ff5005ca