| Message ID | pull.1340.git.1661867664.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | A couple of CI fixes regarding the built-in add --patch | expand |
Hi, On Tue, 30 Aug 2022, Johannes Schindelin via GitGitGadget wrote: > Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but > probably applies cleanly even on maint) because I tried to develop a > semantic patch to fix similar issues in the code base. However, I've since > run into what looks like a bug in Coccinelle > [https://github.com/coccinelle/coccinelle/issues/284]. My latest version of > that semantic patch looks like this, but I stopped when running it on Git's > source code triggered the bug for 66 of Git's .c files: > > @@ > type T = { unsigned int }; > T:n b; > type S != { unsigned int, size_t }; > S s; > binary operator o != { &&, || }; > @@ > -s o b > +s o (S)b > > @@ > type T = { unsigned int }; > T:n b; > type S != { unsigned int, size_t }; > S s; > binary operator o != { &&, || }; > @@ > -b o s > +(S)b o s The bug in Coccinelle is already fixed (are you impressed? I certainly am at the incredible speed and at the wonderful conversation I had!), and I verified with this semantic patch that our code is clean: -- snip -- @@ type T = { unsigned int }; T:n b; type S != { unsigned int, size_t, float, double }; S s; binary operator o != { &&, || }; @@ s o -b +(S)b @@ type T = { unsigned int }; T:n b; type S != { unsigned int, size_t, float, double }; S s; binary operator o != { &&, || }; @@ -b +(S)b o s -- snap -- I do not currently plan on integrating this into `contrib/coccinelle/`, though, because it will take a while until we can benefit from the fix in Git's CI/PR runs. Ciao, Dscho
Hi Dscho All three patches look good to me, thanks for working on them. Phillip On 30/08/2022 14:54, Johannes Schindelin via GitGitGadget wrote: > This is an attempt to address the concern Junio raised in > https://lore.kernel.org/git/xmqq7d3gm1bl.fsf@gitster.g/: originally > motivated by a test suite failure when running Git's test suite in Visual > Studio, this pulls out the signed vs unsigned fix whose implications are > potentially much wider than Visual C. > > While in that space, I spent the time (which took almost as long as I > expected > [https://lore.kernel.org/git/nrr2312s-q256-61n7-2843-7r0s817rp432@tzk.qr/]) > to craft a semantic patch to scrutinize Git's source code for similar issues > (narrator's voice: there were no other instances, what did ya expect?). > > To verify the fix, I then worked on a patch to exercise the built-in git add > -p in the test suite even when NO_PERL is set, and while developing this > patch and validating it, I got really puzzled that the add -p test case in > t6132 did not need to be guarded behind a PERL prereq. So this patch series > also includes a fix for that. > > The story arc that binds all of these patches together is that they all > revolve around NO_PERL and CI issues that involve git add --patch. > > Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but > probably applies cleanly even on maint) because I tried to develop a > semantic patch to fix similar issues in the code base. However, I've since > run into what looks like a bug in Coccinelle > [https://github.com/coccinelle/coccinelle/issues/284]. My latest version of > that semantic patch looks like this, but I stopped when running it on Git's > source code triggered the bug for 66 of Git's .c files: > > @@ > type T = { unsigned int }; > T:n b; > type S != { unsigned int, size_t }; > S s; > binary operator o != { &&, || }; > @@ > -s o b > +s o (S)b > > @@ > type T = { unsigned int }; > T:n b; > type S != { unsigned int, size_t }; > S s; > binary operator o != { &&, || }; > @@ > -b o s > +(S)b o s > > > Johannes Schindelin (3): > add -p: avoid ambiguous signed/unsigned comparison > t3701: test the built-in `add -i` regardless of NO_PERL > t6132(NO_PERL): do not run the scripted `add -p` > > add-patch.c | 2 +- > t/t3701-add-interactive.sh | 4 ++-- > t/t6132-pathspec-exclude.sh | 6 +++++- > 3 files changed, 8 insertions(+), 4 deletions(-) > > > base-commit: ef46584831268a83591412a33783caf866867482 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1340%2Fdscho%2Fbuilt-in-add-i-does-not-need-perl-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1340/dscho/built-in-add-i-does-not-need-perl-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1340
Hi Dscho On 30/08/2022 15:19, Johannes Schindelin wrote: > Hi, > > On Tue, 30 Aug 2022, Johannes Schindelin via GitGitGadget wrote: > >> Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but >> probably applies cleanly even on maint) because I tried to develop a >> semantic patch to fix similar issues in the code base. However, I've since >> run into what looks like a bug in Coccinelle >> [https://github.com/coccinelle/coccinelle/issues/284]. My latest version of >> that semantic patch looks like this, but I stopped when running it on Git's >> source code triggered the bug for 66 of Git's .c files: >> >> @@ >> type T = { unsigned int }; >> T:n b; >> type S != { unsigned int, size_t }; >> S s; >> binary operator o != { &&, || }; >> @@ >> -s o b >> +s o (S)b >> >> @@ >> type T = { unsigned int }; >> T:n b; >> type S != { unsigned int, size_t }; >> S s; >> binary operator o != { &&, || }; >> @@ >> -b o s >> +(S)b o s > > The bug in Coccinelle is already fixed (are you impressed? I certainly > am at the incredible speed and at the wonderful conversation I had!), and > I verified with this semantic patch that our code is clean: Wow that's fast, I wonder if they would be interested in fixing the parsing bug we found with Peff's UNUSED() series. It's good news that the patch does not find any other problems. Best Wishes Phillip > > -- snip -- > @@ > type T = { unsigned int }; > T:n b; > type S != { unsigned int, size_t, float, double }; > S s; > binary operator o != { &&, || }; > @@ > s o > -b > +(S)b > > @@ > type T = { unsigned int }; > T:n b; > type S != { unsigned int, size_t, float, double }; > S s; > binary operator o != { &&, || }; > @@ > -b > +(S)b > o s > -- snap -- > > I do not currently plan on integrating this into `contrib/coccinelle/`, > though, because it will take a while until we can benefit from the fix in > Git's CI/PR runs. > > Ciao, > Dscho
Hi Phillip, On Tue, 30 Aug 2022, Phillip Wood wrote: > I wonder if they would be interested in fixing the parsing bug we found > with Peff's UNUSED() series. Could you point me to the relevant mail? I am sure that I can come up with an MCVE that will help them pinpoint the bug and fix it as quickly as they did with this here bug. Still amazed at the turn-around time, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Phillip, > > On Tue, 30 Aug 2022, Phillip Wood wrote: > >> I wonder if they would be interested in fixing the parsing bug we found >> with Peff's UNUSED() series. > > Could you point me to the relevant mail? I am sure that I can come up with > an MCVE that will help them pinpoint the bug and fix it as quickly as they > did with this here bug. > > Still amazed at the turn-around time, > Dscho https://lore.kernel.org/git/xmqqwnax438x.fsf@gitster.g
On Tue, Aug 30, 2022 at 11:12:24PM +0200, Johannes Schindelin wrote: > On Tue, 30 Aug 2022, Phillip Wood wrote: > > > I wonder if they would be interested in fixing the parsing bug we found > > with Peff's UNUSED() series. > > Could you point me to the relevant mail? I am sure that I can come up with > an MCVE that will help them pinpoint the bug and fix it as quickly as they > did with this here bug. It's the sub-thread starting here: https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/ I tried to put together a minimal example, which would be something like: $ cat foo.c void old_function(int); void new_function(int); #if defined(__GNUC__) #define UNUSED(var) UNUSED_##var __attribute__((unused)) #else #define UNUSED(var) UNUSED_##var #endif void foo(int UNUSED(bar), int x) { old_function(x); } $ cat foo.cocci @@ expression E; @@ -old_function(E) +new_function(E) but to my frustration, it actually works! If you add --verbose-parsing to the spatch command, you can see it complaining about some of the parsing. And it produces wrong outputs in the real-world with our actual unused.cocci file. I suspect the example semantic patch might just need to be a little more complicated. -Peff
Junio C Hamano <gitster@pobox.com> writes: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> Hi Phillip, >> >> On Tue, 30 Aug 2022, Phillip Wood wrote: >> >>> I wonder if they would be interested in fixing the parsing bug we found >>> with Peff's UNUSED() series. >> >> Could you point me to the relevant mail? I am sure that I can come up with >> an MCVE that will help them pinpoint the bug and fix it as quickly as they >> did with this here bug. I think the reference Peff posted is much closer to the description of the "parsing bug". Sorry for the noise. >> Still amazed at the turn-around time, >> Dscho Yes, indeed. Thanks.
This is an attempt to address the concern Junio raised in https://lore.kernel.org/git/xmqq7d3gm1bl.fsf@gitster.g/: originally motivated by a test suite failure when running Git's test suite in Visual Studio, this pulls out the signed vs unsigned fix whose implications are potentially much wider than Visual C. While in that space, I spent the time (which took almost as long as I expected [https://lore.kernel.org/git/nrr2312s-q256-61n7-2843-7r0s817rp432@tzk.qr/]) to craft a semantic patch to scrutinize Git's source code for similar issues (narrator's voice: there were no other instances, what did ya expect?). To verify the fix, I then worked on a patch to exercise the built-in git add -p in the test suite even when NO_PERL is set, and while developing this patch and validating it, I got really puzzled that the add -p test case in t6132 did not need to be guarded behind a PERL prereq. So this patch series also includes a fix for that. The story arc that binds all of these patches together is that they all revolve around NO_PERL and CI issues that involve git add --patch. Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but probably applies cleanly even on maint) because I tried to develop a semantic patch to fix similar issues in the code base. However, I've since run into what looks like a bug in Coccinelle [https://github.com/coccinelle/coccinelle/issues/284]. My latest version of that semantic patch looks like this, but I stopped when running it on Git's source code triggered the bug for 66 of Git's .c files: @@ type T = { unsigned int }; T:n b; type S != { unsigned int, size_t }; S s; binary operator o != { &&, || }; @@ -s o b +s o (S)b @@ type T = { unsigned int }; T:n b; type S != { unsigned int, size_t }; S s; binary operator o != { &&, || }; @@ -b o s +(S)b o s Johannes Schindelin (3): add -p: avoid ambiguous signed/unsigned comparison t3701: test the built-in `add -i` regardless of NO_PERL t6132(NO_PERL): do not run the scripted `add -p` add-patch.c | 2 +- t/t3701-add-interactive.sh | 4 ++-- t/t6132-pathspec-exclude.sh | 6 +++++- 3 files changed, 8 insertions(+), 4 deletions(-) base-commit: ef46584831268a83591412a33783caf866867482 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1340%2Fdscho%2Fbuilt-in-add-i-does-not-need-perl-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1340/dscho/built-in-add-i-does-not-need-perl-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1340