From: Hardik Kumar <hardikxk@gmail.com>
Date: Mon, 27 Jul 2026 16:13:28 GMT
Subject: Re: [PATCH v3] utf8: make utf8_strwidth() and utf8_strnwidth() return size_t
Message-ID: <DK9HJ1A58HMD.2CDVOK50X2UMH@gmail.com>
In-Reply-To: <e971400e-6d23-463f-ae9c-a21d3c5a3563@gmail.com>

On Mon Jul 27, 2026 at 6:21 PM IST, Phillip Wood 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 had intentionally left out some sites which did not seem could have
any impact by implicit conversions as there are other examples of such
cases where the return value of `strlen` is being assigned to an int
variable. Example:

in combine-diff.c (where len is an int):
	if (len < 0)
		len = strlen(line);

in builtin/update-index.c:
	int namelen = strlen(path);

and other such examples.
>
> 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).
This can result in issues down the line and I had mentioned so in a
previous mail but wanted to try it with v3 since I had already been
working on it. I'll send a new patch to remove the TODO. This change
might just not be worth after all.

Thanks,
Hardik