| Message ID | 4d24a4345ba66031d2ccf7ce472ed93ace82e9d6.1660143750.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Some fixes and an improvement for using CTest on Windows | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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@gmx.de> > --- 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. Thanks. > add-patch.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/add-patch.c b/add-patch.c > index 509ca04456b..3524555e2b0 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -1547,7 +1547,7 @@ soft_increment: > strbuf_remove(&s->answer, 0, 1); > strbuf_trim(&s->answer); > i = hunk_index - DISPLAY_HUNKS_LINES / 2; > - if (i < file_diff->mode_change) > + if (i < (int)file_diff->mode_change) > i = file_diff->mode_change; > while (s->answer.len == 0) { > i = display_hunks(s, file_diff, i);
Hi Dscho On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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. Oh the joys of bit-fields! > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > add-patch.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/add-patch.c b/add-patch.c > index 509ca04456b..3524555e2b0 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -1547,7 +1547,7 @@ soft_increment: > strbuf_remove(&s->answer, 0, 1); > strbuf_trim(&s->answer); Unrelated to this change but why don't we just call strbuf_reset() here? > i = hunk_index - DISPLAY_HUNKS_LINES / 2; > - if (i < file_diff->mode_change) > + if (i < (int)file_diff->mode_change) The change itself looks good Best Wishes Phillip > i = file_diff->mode_change; > while (s->answer.len == 0) { > i = display_hunks(s, file_diff, i);
Hi Junio, On Wed, 10 Aug 2022, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > 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@gmx.de> > > --- > > 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
Hi Phillip, On Thu, 11 Aug 2022, Phillip Wood wrote: > On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote: > > > diff --git a/add-patch.c b/add-patch.c > > index 509ca04456b..3524555e2b0 100644 > > --- a/add-patch.c > > +++ b/add-patch.c > > @@ -1547,7 +1547,7 @@ soft_increment: > > strbuf_remove(&s->answer, 0, 1); > > strbuf_trim(&s->answer); > > Unrelated to this change but why don't we just call strbuf_reset() here? This part of the code is used when the `g` command (to "go to hunk") was issued by the user. And that `g` command allows for a number to be specified, e.g. `g1` to go to the first hunk. The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command. The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and the number, e.g. `g 1` should also go to the first hunk. If we called `strbuf_reset()` here, we would remove the number completely. Ciao, Dscho
Hi Dscho On 16/08/2022 11:00, Johannes Schindelin wrote: > Hi Phillip, > > On Thu, 11 Aug 2022, Phillip Wood wrote: > >> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote: >> >>> diff --git a/add-patch.c b/add-patch.c >>> index 509ca04456b..3524555e2b0 100644 >>> --- a/add-patch.c >>> +++ b/add-patch.c >>> @@ -1547,7 +1547,7 @@ soft_increment: >>> strbuf_remove(&s->answer, 0, 1); >>> strbuf_trim(&s->answer); >> >> Unrelated to this change but why don't we just call strbuf_reset() here? > > This part of the code is used when the `g` command (to "go to hunk") was > issued by the user. And that `g` command allows for a number to be > specified, e.g. `g1` to go to the first hunk. > > The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command. > > The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and > the number, e.g. `g 1` should also go to the first hunk. > > If we called `strbuf_reset()` here, we would remove the number completely. Oh so if the user is not using interactive.singleKey then they can skip the prompt which displays the hunks and asks which one they want to jump to by guessing the number of the hunk they want and typing "g <n>". Thanks Phillip > Ciao, > Dscho
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. > 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... Thanks.
Hi Phillip, On Tue, 16 Aug 2022, Phillip Wood wrote: > On 16/08/2022 11:00, Johannes Schindelin wrote: > > > On Thu, 11 Aug 2022, Phillip Wood wrote: > > > > > On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote: > > > > > > > diff --git a/add-patch.c b/add-patch.c > > > > index 509ca04456b..3524555e2b0 100644 > > > > --- a/add-patch.c > > > > +++ b/add-patch.c > > > > @@ -1547,7 +1547,7 @@ soft_increment: > > > > strbuf_remove(&s->answer, 0, 1); > > > > strbuf_trim(&s->answer); > > > > > > Unrelated to this change but why don't we just call strbuf_reset() here? > > > > This part of the code is used when the `g` command (to "go to hunk") was > > issued by the user. And that `g` command allows for a number to be > > specified, e.g. `g1` to go to the first hunk. > > > > The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command. > > > > The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and > > the number, e.g. `g 1` should also go to the first hunk. > > > > If we called `strbuf_reset()` here, we would remove the number completely. > > Oh so if the user is not using interactive.singleKey then they can skip the > prompt which displays the hunks and asks which one they want to jump to by > guessing the number of the hunk they want and typing "g <n>". Yes, precisely. I forgot that you are using `interactive.singleKey` (I planned on opting into that mode, but for some reason I never get around to flip the switch). The default mode is not the single key mode, though. Ciao, Dscho
Hi Junio, On Tue, 16 Aug 2022, Junio C Hamano wrote: > 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`). > > 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
diff --git a/add-patch.c b/add-patch.c index 509ca04456b..3524555e2b0 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1547,7 +1547,7 @@ soft_increment: strbuf_remove(&s->answer, 0, 1); strbuf_trim(&s->answer); i = hunk_index - DISPLAY_HUNKS_LINES / 2; - if (i < file_diff->mode_change) + if (i < (int)file_diff->mode_change) i = file_diff->mode_change; while (s->answer.len == 0) { i = display_hunks(s, file_diff, i);