Hi Junio,
On Tue, 16 Aug 2022, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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`).
> > 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
