Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 19, 2022, 14:52 UTC
Message-ID: <nrr2312s-q256-61n7-2843-7r0s817rp432@tzk.qr>
In-Reply-To: <xmqqr11gxm04.fsf@gitster.g>

Hi Junio,

On Tue, 16 Aug 2022, Junio C Hamano wrote:

Show 14 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > The scope of this patch series is to fix running the tests in Visual
> > Studio when building using CMake.
>
> That's only the perspective of who cares about VS+CMake.  From my
> point of view who wants a healthy Git overall, the priority is
> different.  "add -p" fix is wider than the context of VS, and I do
> not discount the need that we need to fix it before we can call
> VS+CMake issue fixed (and I do not mean to say it is unnecessary to
> fix VS+CMake).  It's just this one can proceed with help by those
> who do not care about or have no environment to help with VS+CMake
> because it is more generic, and I do not think you mind to make the
> rest of the narrower VS+CMake topic _depend_ on it.

Sure.

But if I pull out that patch into its code contribution, all of a sudden the scope of _that_ contribution is to address an implicit signed/unsigned cast. What other purpose could it have? And if we're addressing that signed/unsigned cast, we cannot just fix this single one. We need to fix them all, no?

So let's have a look at that project, since you are implicitly volunteering me for it. We do have this in our `config.mak.dev`:

	# These are disabled because we have these all over the place.
	DEVELOPER_CFLAGS += -Wno-empty-body
	DEVELOPER_CFLAGS += -Wno-missing-field-initializers
	DEVELOPER_CFLAGS += -Wno-sign-compare
	DEVELOPER_CFLAGS += -Wno-unused-parameter
	endif

Note the `-Wno-sign-compare` part. If I comment that out, I get these reports:

-- snip -- diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare] diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare] diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare] diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare] add-interactive.c:207:21: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] add-interactive.c:210:17: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] add-interactive.c:238:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] add-patch.c:423:16: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare] add-interactive.c:379:25: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] add-interactive.c:389:11: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] [... 1260 more issues ...] -- snap --

You see how that increases the amount of work you are asking me to do?

The worst part is that gcc (at least of version 9.4.0 which I have available in my Ubuntu) does not even catch the line that is fixed by the patch we are trying to discuss here. It catches 10 issues in `add-patch.c`, but not the `i < file_diff->mode_change` one.

Neither does Visual C 2022, nor the gcc I have in Git for Windows' SDK, which is v12.1.0.

So I would first need to find a tool that identifies all code locations that compare signed with unsigned values.

And that's even before analyzing carefully how to address them (not all instances will be as easy as upcasting from an unsigned bit to `int`, some of those instances are about `size_t` vs `ssize_t`).

Show 9 quoted lines

> > Pulling out this patch would break that patch series because it would
> > leave that breakage in place.
>
> We deal with topic that depends on another topic all the time, and I
> do not think there is anything that makes VS+CMake topic so special
> that it cannot be dependent on another topic.  It's just the matter
> of splitting this out and make it [1/1], and make the remainder to
> [1-4/4] and mark them rely on add-p fix when you send the latter
> out, isn't it?  Puzzled...

Sure, if you think that the signed/unsigned comparison project is more important than fixing running Git's test suite inside Visual Studio, or worse: if you are asking me to do a less than thorough job on those signed/unsigned fixes by fixing only a single instance and leaving all others unaddressed in a patch series that has nothing to do with Visual Studio, then I understand how my stance could confuse you.

But my intention with this patch series is to fix running Git's test suite in Visual Studio. And my intention is to provide a complete solution in my patch series, no half measures.

As such, I would be loathe to have my authorship on a single patch that solves neither the Visual Studio/CTest problem nor the vast majority of the signed/unsigned problems. It would be incomplete in any way you look at it. I would consider such a contribution lackluster, sloppy and definitely not up to my standards.

Ciao, Dscho