Re: [PATCHv2 1/4] add-patch: Fix type missmatch rom msvc

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 7, 2025, 00:26 UTC
Message-ID: <xmqqed1fwjt9.fsf@gitster.g>
In-Reply-To: <Z3xxxbKtqyLmDAif@tapette.crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Show 6 quoted lines

> We don't typically put comments about the revisions we've made to a
> patch in the commit message.  We may put them below the --- so that
> they're visible to readers and reviewers, which is helpful, but we
> pretend that our patches were perfect to begin with in terms of the
> commit message, since the future reader of the history only cares about
> the actual end result and not what changes we made along the way.

Thanks, all true. The future readers would only *see* the end results, and we do not want to hear about the previous stumblings the author made before reaching an acceptable version.

Show 5 quoted lines

>> ---
>>  add-patch.c       | 53 +++++++++++++++++++++++++++--------------------
>>  gettext.h         |  2 +-
>>  git-compat-util.h |  6 ++++++
>>  3 files changed, 38 insertions(+), 23 deletions(-)

I already made this comment, but I think the offset/count being ulong is a very sane design decision, and what is causing the compiler warning is some earlier change that introduced size_t variables or parameters in the callchain. As far as I can tell, there is no system functions that yields size_t (hence we must use size_t everywhere) in the code paths that deal with offset and count. I suggested to find these abused size_t and fix them to use the matching type, i.e. "unsigned long", instead, as an alternative fix. I did not get an impression that the author tried the approach and found why we must use size_t for offset/count instead.

And if we go that route, there is *no* need to talk about 64-bit ve 32-bit platforms. ulong used consistently everywhere would let you use offset/count that fits in ulong, and the apply machinery is artificially limited to limit the patch size to a few gigabytes, so 32-bit ulong should be plenty as Phillip pointed out earlier.

If we needed to parse an integer into a large integer, the existing code seem to use strtoumax() into uintmax_t and move it to the target (while checking for truncation). "Ah we are on windows, so use strtoll, otherwise use strtol" is not something we want to see in our codebase.

> If we're using size_t, we can use %zu.  That's specified in C99 as the
> appropriate formatting type for size_t, and we require C99 or C11 for
> all systems.  We don't need to cast to uintmax_t.

You and Documentation/CodingGuidelines contradict with each other here.

Thanks for a review.