Subject: Re: [PATCH v3 1/4] add-patch: Fix type conversion warnings from msvc

From: Junio C Hamano <[email-masked]>
Date: Wed, 29 Jan 2025 11:52:49 -0800

Phillip Wood <[email-masked]> writes:

> I'm afraid I'm still not convinced this is a good idea for the reasons
> I explained previously [1] together with an alternative approach to
> silencing these warnings. What makes "unsigned long" an incorrect
> choice when that's what "git diff" and "git apply" use?
>
> [1]
> https://lore.kernel.org/git/[email-masked]

Ah, this patch still does that?  I was hoping that it got corrected
already after it was pointed out in the previous iterations.  I
agree with you that size_t is a dubious type to use for the line
numbers there.

diff.c defines "struct emit_callback" with lno_in_{pre,post}image
members that are of type "int", which is somewhat dubious, too, but
at least we don't run on 16-bit machines, and being limited to 2
billion lines is probably OK.  I am OK to upgrade that to long (if
we use negative values for some oob signal) or ulong, but that is
clearly outside of this topic.

>> Add macro str_to_size_t for converting a string to size_t.
>> Test if convertion fails with over or underflow.
>
> That is welcome, but the implementation needs tweaking. If you look at
> other uses of strtoul() in our code you'll see that (somewhat
> unusually) one needs to set errno to zero before calling strtoul() as
> one cannot tell from the return value whether there was an error or
> not. As errno may have been set by a previous function call it needs
> to be cleared before calling strtoul() so we can be sure the error
> came from strtoul().

Nice advice.

> Best Wishes
> Phillip

Thanks.

By the way, who is
<CAPig+cQ49Hdc_8=[email-masked]>
and why is such an apparently bogus e-mail address Cc'ed?