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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.

>> ---
>>  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.