Hi Junio, On Wed, 10 Aug 2022, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx> > writes: > > > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > > > In the interactive `add` operation, users can choose to jump to specific > > hunks, and Git will present the hunk list in that case. To avoid showing > > too many lines at once, only a maximum of 21 hunks are shown, skipping > > the "mode change" pseudo hunk. > > > > The comparison performed to skip the "mode change" pseudo hunk (if any) > > compares a signed integer `i` to the unsigned value `mode_change` (which > > can be 0 or 1 because it is a 1-bit type). > > > > According to section 6.3.1.8 of the C99 standard (see e.g. > > https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should > > happen is an automatic conversion of the "lesser" type to the "greater" > > type, but since the types differ in signedness, it is ill-defined what > > is the correct "usual arithmetic conversion". > > > > Which means that Visual C's behavior can (and does) differ from GCC's: > > When compiling Git using the latter, `add -p`'s `goto` command shows no > > hunks by default because it casts a negative start offset to a pretty > > large unsigned value, breaking the "goto hunk" test case in > > `t3701-add-interactive.sh`. > > > > Let's avoid that by converting the unsigned bit explicitly to a signed > > integer. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > > --- > > This looks more like a fix to a general problem, not limited to > windows or cmake, that we had since 9254bdfb (built-in add -p: > implement the 'g' ("goto") command, 2019-12-13). > > Please pull this out of the series and let's have it reviewed > separately. The scope of this patch series is to fix running the tests in Visual Studio when building using CMake. Pulling out this patch would break that patch series because it would leave that breakage in place. Except if you are asking to put this patch series on the back burner and prioritize the patch that fixes an ambiguous implicit cast between signed and unsigned data types? However, that would mean that I'd now have to address all of those implicit casts, which is unfortunately a larger amount of work than I can justify to take on. Therefore I move that in this instance, the perfect is the enemy of the good, and that the patch should remain within this patch series, even if the larger-scoped project to avoid any implicit signed/unsigned casts remains unaddressed. BTW I would have expected your review to ask the (in hindsight, obvious) question why the test suite still passes even with `vs-test` exercising the code that is compiled using Visual C? The answer to that would have been that the `vs-test` job of our CI runs defines `NO_PERL`, and t3701 is skipped completely if that is the case. Ciao, Dscho
