{"thread":{"id":"58377","subject":"[PATCH 0/3] A couple of CI fixes regarding the built-in add --patch","startedAt":"2022-08-30T13:54:53Z","lastAt":"2022-08-30T21:46:31Z","messageCount":15,"participants":["Johannes Schindelin via GitGitGadget","Johannes Schindelin","Phillip Wood","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"462192","messageId":"pull.1340.git.1661867664.gitgitgadget@gmail.com","threadId":"58377","inReplyTo":null,"subject":"[PATCH 0/3] A couple of CI fixes regarding the built-in add --patch","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-30T13:54:21Z","receivedAt":"2022-08-30T13:54:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is an attempt to address the concern Junio raised in\nhttps://lore.kernel.org/git/xmqq7d3gm1bl.fsf@gitster.g/: originally\nmotivated by a test suite failure when running Git's test suite in Visual\nStudio, this pulls out the signed vs unsigned fix whose implications are\npotentially much wider than Visual C.\n\nWhile in that space, I spent the time (which took almost as long as I\nexpected\n[https://lore.kernel.org/git/nrr2312s-q256-61n7-2843-7r0s817rp432@tzk.qr/])\nto craft a semantic patch to scrutinize Git's source code for similar issues\n(narrator's voice: there were no other instances, what did ya expect?).\n\nTo verify the fix, I then worked on a patch to exercise the built-in git add\n-p in the test suite even when NO_PERL is set, and while developing this\npatch and validating it, I got really puzzled that the add -p test case in\nt6132 did not need to be guarded behind a PERL prereq. So this patch series\nalso includes a fix for that.\n\nThe story arc that binds all of these patches together is that they all\nrevolve around NO_PERL and CI issues that involve git add --patch.\n\nNote: This patch series is based on ds/github-actions-use-newer-ubuntu (but\nprobably applies cleanly even on maint) because I tried to develop a\nsemantic patch to fix similar issues in the code base. However, I've since\nrun into what looks like a bug in Coccinelle\n[https://github.com/coccinelle/coccinelle/issues/284]. My latest version of\nthat semantic patch looks like this, but I stopped when running it on Git's\nsource code triggered the bug for 66 of Git's .c files:\n\n@@\ntype T = { unsigned int };\nT:n b;\ntype S != { unsigned int, size_t };\nS s;\nbinary operator o != { &&, || };\n@@\n-s o b\n+s o (S)b\n\n@@\ntype T = { unsigned int };\nT:n b;\ntype S != { unsigned int, size_t };\nS s;\nbinary operator o != { &&, || };\n@@\n-b o s\n+(S)b o s\n\n\nJohannes Schindelin (3):\n add -p: avoid ambiguous signed/unsigned comparison\n t3701: test the built-in `add -i` regardless of NO_PERL\n t6132(NO_PERL): do not run the scripted `add -p`\n\n add-patch.c | 2 +-\n t/t3701-add-interactive.sh | 4 ++--\n t/t6132-pathspec-exclude.sh | 6 +++++-\n 3 files changed, 8 insertions(+), 4 deletions(-)\n\n\nbase-commit: ef46584831268a83591412a33783caf866867482\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1340%2Fdscho%2Fbuilt-in-add-i-does-not-need-perl-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1340/dscho/built-in-add-i-does-not-need-perl-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1340\n-- \ngitgitgadget\n"},{"id":"462193","messageId":"0691d7eaaa03e8bf8b460b9e20ec05eec09fb574.1661867664.git.gitgitgadget@gmail.com","threadId":"58377","inReplyTo":"pull.1340.git.1661867664.gitgitgadget@gmail.com","subject":"[PATCH 1/3] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-30T13:54:22Z","receivedAt":"2022-08-30T13:54:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the interactive `add` operation, users can choose to jump to specific\nhunks, and Git will present the hunk list in that case. To avoid showing\ntoo many lines at once, only a maximum of 21 hunks are shown, skipping\nthe \"mode change\" pseudo hunk.\n\nThe comparison performed to skip the \"mode change\" pseudo hunk (if any)\ncompares a signed integer `i` to the unsigned value `mode_change` (which\ncan be 0 or 1 because it is a 1-bit type).\n\nAccording to section 6.3.1.8 of the C99 standard (see e.g.\nhttps://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should\nhappen is an automatic conversion of the \"lesser\" type to the \"greater\"\ntype, but since the types differ in signedness, it is ill-defined what\nis the correct \"usual arithmetic conversion\".\n\nWhich means that Visual C's behavior can (and does) differ from GCC's:\nWhen compiling Git using the latter, `add -p`'s `goto` command shows no\nhunks by default because it casts a negative start offset to a pretty\nlarge unsigned value, breaking the \"goto hunk\" test case in\n`t3701-add-interactive.sh`.\n\nLet's avoid that by converting the unsigned bit explicitly to a signed\ninteger.\n\nNote: This is a long-standing bug in the Visual C build of Git, but it\nhas never been caught because t3701 is skipped when `NO_PERL` is set,\nwhich is the case in the `vs-test` jobs of Git's CI runs.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 509ca04456b..3524555e2b0 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1547,7 +1547,7 @@ soft_increment:\n \t\t\tstrbuf_remove(&s->answer, 0, 1);\n \t\t\tstrbuf_trim(&s->answer);\n \t\t\ti = hunk_index - DISPLAY_HUNKS_LINES / 2;\n-\t\t\tif (i < file_diff->mode_change)\n+\t\t\tif (i < (int)file_diff->mode_change)\n \t\t\t\ti = file_diff->mode_change;\n \t\t\twhile (s->answer.len == 0) {\n \t\t\t\ti = display_hunks(s, file_diff, i);\n-- \ngitgitgadget\n\n"},{"id":"462194","messageId":"54d25d991b09219f6992dc3e8c102ce1ccef6313.1661867664.git.gitgitgadget@gmail.com","threadId":"58377","inReplyTo":"pull.1340.git.1661867664.gitgitgadget@gmail.com","subject":"[PATCH 2/3] t3701: test the built-in `add -i` regardless of NO_PERL","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-30T13:54:23Z","receivedAt":"2022-08-30T13:55:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe built-in `git add --interactive` does not require Perl, therefore we\ncan safely run these tests even when building with `NO_PERL=LetsDoThat`.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex b354fb39de8..8d16cd45821 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -7,9 +7,9 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-if ! test_have_prereq PERL\n+if test_have_prereq !ADD_I_USE_BUILTIN,!PERL\n then\n-\tskip_all='skipping add -i tests, perl not available'\n+\tskip_all='skipping add -i (scripted) tests, perl not available'\n \ttest_done\n fi\n \n-- \ngitgitgadget\n\n"},{"id":"462195","messageId":"736f2dde6918822e5f8234dc8cddcb94aa3f92ac.1661867664.git.gitgitgadget@gmail.com","threadId":"58377","inReplyTo":"pull.1340.git.1661867664.gitgitgadget@gmail.com","subject":"[PATCH 3/3] t6132(NO_PERL): do not run the scripted `add -p`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-30T13:54:24Z","receivedAt":"2022-08-30T13:55:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen using the non-built-in version of `git add -p` in a `NO_PERL`\nbuild, we expect that invocation to fail.\n\nHowever, when b02fdbc80a4 (pathspec: correct an empty string used as a\npathspec element, 2022-05-29) added a test case to t6132 to exercise\n`git add -p`, it did not add appropriate prereqs (which admittedly did\nnot exist back then).\n\nLet's specify the appropriate prereqs.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t6132-pathspec-exclude.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh\nindex 9fdafeb1e90..cada952f9ae 100755\n--- a/t/t6132-pathspec-exclude.sh\n+++ b/t/t6132-pathspec-exclude.sh\n@@ -293,7 +293,11 @@ test_expect_success 'add with all negative' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'add -p with all negative' '\n+test_lazy_prereq ADD_I_USE_BUILTIN_OR_PERL '\n+\ttest_have_prereq ADD_I_USE_BUILTIN || test_have_prereq PERL\n+'\n+\n+test_expect_success ADD_I_USE_BUILTIN_OR_PERL 'add -p with all negative' '\n \tH=$(git rev-parse HEAD) &&\n \tgit reset --hard $H &&\n \tgit clean -f &&\n-- \ngitgitgadget\n"},{"id":"462199","messageId":"p9p431r8-nq64-02sq-3049-n10rrn1o89o7@tzk.qr","threadId":"58377","inReplyTo":"pull.1340.git.1661867664.gitgitgadget@gmail.com","subject":"validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-30T14:19:00Z","receivedAt":"2022-08-30T14:19:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 30 Aug 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but\n> probably applies cleanly even on maint) because I tried to develop a\n> semantic patch to fix similar issues in the code base. However, I've since\n> run into what looks like a bug in Coccinelle\n> [https://github.com/coccinelle/coccinelle/issues/284]. My latest version of\n> that semantic patch looks like this, but I stopped when running it on Git's\n> source code triggered the bug for 66 of Git's .c files:\n>\n> @@\n> type T = { unsigned int };\n> T:n b;\n> type S != { unsigned int, size_t };\n> S s;\n> binary operator o != { &&, || };\n> @@\n> -s o b\n> +s o (S)b\n>\n> @@\n> type T = { unsigned int };\n> T:n b;\n> type S != { unsigned int, size_t };\n> S s;\n> binary operator o != { &&, || };\n> @@\n> -b o s\n> +(S)b o s\n\nThe bug in Coccinelle is already fixed (are you impressed? I certainly\nam at the incredible speed and at the wonderful conversation I had!), and\nI verified with this semantic patch that our code is clean:\n\n-- snip --\n@@\ntype T = { unsigned int };\nT:n b;\ntype S != { unsigned int, size_t, float, double };\nS s;\nbinary operator o != { &&, || };\n@@\n s o\n-b\n+(S)b\n\n@@\ntype T = { unsigned int };\nT:n b;\ntype S != { unsigned int, size_t, float, double };\nS s;\nbinary operator o != { &&, || };\n@@\n-b\n+(S)b\n o s\n-- snap --\n\nI do not currently plan on integrating this into `contrib/coccinelle/`,\nthough, because it will take a while until we can benefit from the fix in\nGit's CI/PR runs.\n\nCiao,\nDscho\n"},{"id":"462206","messageId":"26fdecfe-1715-1b12-2cd4-ec114f5b426b@gmail.com","threadId":"58377","inReplyTo":"pull.1340.git.1661867664.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-30T15:19:47Z","receivedAt":"2022-08-30T15:20:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nAll three patches look good to me, thanks for working on them.\n\nPhillip\n\nOn 30/08/2022 14:54, Johannes Schindelin via GitGitGadget wrote:\n> This is an attempt to address the concern Junio raised in\n> https://lore.kernel.org/git/xmqq7d3gm1bl.fsf@gitster.g/: originally\n> motivated by a test suite failure when running Git's test suite in Visual\n> Studio, this pulls out the signed vs unsigned fix whose implications are\n> potentially much wider than Visual C.\n> \n> While in that space, I spent the time (which took almost as long as I\n> expected\n> [https://lore.kernel.org/git/nrr2312s-q256-61n7-2843-7r0s817rp432@tzk.qr/])\n> to craft a semantic patch to scrutinize Git's source code for similar issues\n> (narrator's voice: there were no other instances, what did ya expect?).\n> \n> To verify the fix, I then worked on a patch to exercise the built-in git add\n> -p in the test suite even when NO_PERL is set, and while developing this\n> patch and validating it, I got really puzzled that the add -p test case in\n> t6132 did not need to be guarded behind a PERL prereq. So this patch series\n> also includes a fix for that.\n> \n> The story arc that binds all of these patches together is that they all\n> revolve around NO_PERL and CI issues that involve git add --patch.\n> \n> Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but\n> probably applies cleanly even on maint) because I tried to develop a\n> semantic patch to fix similar issues in the code base. However, I've since\n> run into what looks like a bug in Coccinelle\n> [https://github.com/coccinelle/coccinelle/issues/284]. My latest version of\n> that semantic patch looks like this, but I stopped when running it on Git's\n> source code triggered the bug for 66 of Git's .c files:\n> \n> @@\n> type T = { unsigned int };\n> T:n b;\n> type S != { unsigned int, size_t };\n> S s;\n> binary operator o != { &&, || };\n> @@\n> -s o b\n> +s o (S)b\n> \n> @@\n> type T = { unsigned int };\n> T:n b;\n> type S != { unsigned int, size_t };\n> S s;\n> binary operator o != { &&, || };\n> @@\n> -b o s\n> +(S)b o s\n> \n> \n> Johannes Schindelin (3):\n> add -p: avoid ambiguous signed/unsigned comparison\n> t3701: test the built-in `add -i` regardless of NO_PERL\n> t6132(NO_PERL): do not run the scripted `add -p`\n> \n> add-patch.c | 2 +-\n> t/t3701-add-interactive.sh | 4 ++--\n> t/t6132-pathspec-exclude.sh | 6 +++++-\n> 3 files changed, 8 insertions(+), 4 deletions(-)\n> \n> \n> base-commit: ef46584831268a83591412a33783caf866867482\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1340%2Fdscho%2Fbuilt-in-add-i-does-not-need-perl-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1340/dscho/built-in-add-i-does-not-need-perl-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1340\n"},{"id":"462207","messageId":"dc48ce59-530e-da74-93ad-9eb4a17e391c@gmail.com","threadId":"58377","inReplyTo":"p9p431r8-nq64-02sq-3049-n10rrn1o89o7@tzk.qr","subject":"Re: validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-30T15:22:03Z","receivedAt":"2022-08-30T15:22:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 30/08/2022 15:19, Johannes Schindelin wrote:\n> Hi,\n> \n> On Tue, 30 Aug 2022, Johannes Schindelin via GitGitGadget wrote:\n> \n>> Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but\n>> probably applies cleanly even on maint) because I tried to develop a\n>> semantic patch to fix similar issues in the code base. However, I've since\n>> run into what looks like a bug in Coccinelle\n>> [https://github.com/coccinelle/coccinelle/issues/284]. My latest version of\n>> that semantic patch looks like this, but I stopped when running it on Git's\n>> source code triggered the bug for 66 of Git's .c files:\n>>\n>> @@\n>> type T = { unsigned int };\n>> T:n b;\n>> type S != { unsigned int, size_t };\n>> S s;\n>> binary operator o != { &&, || };\n>> @@\n>> -s o b\n>> +s o (S)b\n>>\n>> @@\n>> type T = { unsigned int };\n>> T:n b;\n>> type S != { unsigned int, size_t };\n>> S s;\n>> binary operator o != { &&, || };\n>> @@\n>> -b o s\n>> +(S)b o s\n> \n> The bug in Coccinelle is already fixed (are you impressed? I certainly\n> am at the incredible speed and at the wonderful conversation I had!), and\n> I verified with this semantic patch that our code is clean:\n\nWow that's fast, I wonder if they would be interested in fixing the \nparsing bug we found with Peff's UNUSED() series. It's good news that \nthe patch does not find any other problems.\n\nBest Wishes\n\nPhillip\n\n> \n> -- snip --\n> @@\n> type T = { unsigned int };\n> T:n b;\n> type S != { unsigned int, size_t, float, double };\n> S s;\n> binary operator o != { &&, || };\n> @@\n> s o\n> -b\n> +(S)b\n> \n> @@\n> type T = { unsigned int };\n> T:n b;\n> type S != { unsigned int, size_t, float, double };\n> S s;\n> binary operator o != { &&, || };\n> @@\n> -b\n> +(S)b\n> o s\n> -- snap --\n> \n> I do not currently plan on integrating this into `contrib/coccinelle/`,\n> though, because it will take a while until we can benefit from the fix in\n> Git's CI/PR runs.\n> \n> Ciao,\n> Dscho\n"},{"id":"462228","messageId":"Yw5c+8kaTRtJ52fk@coredump.intra.peff.net","threadId":"58377","inReplyTo":"54d25d991b09219f6992dc3e8c102ce1ccef6313.1661867664.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] t3701: test the built-in `add -i` regardless of NO_PERL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-30T18:54:51Z","receivedAt":"2022-08-30T18:55:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 30, 2022 at 01:54:23PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> The built-in `git add --interactive` does not require Perl, therefore we\n> can safely run these tests even when building with `NO_PERL=LetsDoThat`.\n\nMake sense. The patch is small enough that it is certainly worth doing\nin the meantime, but is it time to start thinking about dropping the\nperl implementation (and hence this prereq) entirely?\n\n-Peff\n"},{"id":"462230","messageId":"xmqqtu5tk0pi.fsf@gitster.g","threadId":"58377","inReplyTo":"0691d7eaaa03e8bf8b460b9e20ec05eec09fb574.1661867664.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] add -p: avoid ambiguous signed/unsigned comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-30T19:09:29Z","receivedAt":"2022-08-30T19:09:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In the interactive `add` operation, users can choose to jump to specific\n> hunks, and Git will present the hunk list in that case. To avoid showing\n> too many lines at once, only a maximum of 21 hunks are shown, skipping\n> the \"mode change\" pseudo hunk.\n\nWow.\n\nWhile the whole \"add -i\" was what I started, I admit that everything\nmore recent than the support of the \"edit\" action in \"add -p\" is\nforeign to me. I didn't remember the existence of, and the\njustification for, the 20-line limit. It seems that this came from\n3f6aff68 (Add subroutine to display one-line summary of hunks,\n2008-12-04).\n\n> The comparison performed to skip the \"mode change\" pseudo hunk (if any)\n> compares a signed integer `i` to the unsigned value `mode_change` (which\n> can be 0 or 1 because it is a 1-bit type).\n\nOK, that warrants a casting to signed, surely.\n\n> Note: This is a long-standing bug in the Visual C build of Git, but it\n> has never been caught because t3701 is skipped when `NO_PERL` is set,\n> which is the case in the `vs-test` jobs of Git's CI runs.\n\nGood finding.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> add-patch.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/add-patch.c b/add-patch.c\n> index 509ca04456b..3524555e2b0 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1547,7 +1547,7 @@ soft_increment:\n> \t\t\tstrbuf_remove(&s->answer, 0, 1);\n> \t\t\tstrbuf_trim(&s->answer);\n> \t\t\ti = hunk_index - DISPLAY_HUNKS_LINES / 2;\n> -\t\t\tif (i < file_diff->mode_change)\n> +\t\t\tif (i < (int)file_diff->mode_change)\n> \t\t\t\ti = file_diff->mode_change;\n> \t\t\twhile (s->answer.len == 0) {\n> \t\t\t\ti = display_hunks(s, file_diff, i);\n"},{"id":"462231","messageId":"xmqqo7w1k0ph.fsf@gitster.g","threadId":"58377","inReplyTo":"54d25d991b09219f6992dc3e8c102ce1ccef6313.1661867664.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] t3701: test the built-in `add -i` regardless of NO_PERL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-30T19:09:30Z","receivedAt":"2022-08-30T19:09:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> The built-in `git add --interactive` does not require Perl, therefore we\n> can safely run these tests even when building with `NO_PERL=LetsDoThat`.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t3701-add-interactive.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n\nA companion to the previous step that is obvious and very much to\nthe point. Looking good.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index b354fb39de8..8d16cd45821 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -7,9 +7,9 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-terminal.sh\n> \n> -if ! test_have_prereq PERL\n> +if test_have_prereq !ADD_I_USE_BUILTIN,!PERL\n> then\n> -\tskip_all='skipping add -i tests, perl not available'\n> +\tskip_all='skipping add -i (scripted) tests, perl not available'\n> \ttest_done\n> fi\n"},{"id":"462240","messageId":"rsrn5988-37n4-7q45-s1o9-6n40rropp120@tzk.qr","threadId":"58377","inReplyTo":"Yw5c+8kaTRtJ52fk@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] t3701: test the built-in `add -i` regardless of NO_PERL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-30T21:03:08Z","receivedAt":"2022-08-30T21:03:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 30 Aug 2022, Jeff King wrote:\n\n> On Tue, Aug 30, 2022 at 01:54:23PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin \n> >\n> > The built-in `git add --interactive` does not require Perl, therefore we\n> > can safely run these tests even when building with `NO_PERL=LetsDoThat`.\n>\n> Make sense. The patch is small enough that it is certainly worth doing\n> in the meantime, but is it time to start thinking about dropping the\n> perl implementation (and hence this prereq) entirely?\n\nA couple of months ago, I would have said the same (and you seemed to\nsuggest something similar in [*1*], too), but since we flipped the default\nto the built-in version in 0527ccb1b55 (add -i: default to the built-in\nimplementation, 2021-11-30), we discovered the need for several fixes:\n\n- pw/add-p-hunk-split-fix\n- js/add-i-delete\n- js/add-p-diff-parsing-fix\n\nTherefore I consider it premature to drop `git-add--interactive.perl` just\nyet.\n\nCiao,\nDscho\n\nFootnote *1*:\nhttps://lore.kernel.org/git/YaaP%2Fg74KA63MCmx@coredump.intra.peff.net/\n"},{"id":"462242","messageId":"73spp934-1q41-1123-41no-q2337954op92@tzk.qr","threadId":"58377","inReplyTo":"dc48ce59-530e-da74-93ad-9eb4a17e391c@gmail.com","subject":"Re: validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-30T21:12:24Z","receivedAt":"2022-08-30T21:12:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 30 Aug 2022, Phillip Wood wrote:\n\n> I wonder if they would be interested in fixing the parsing bug we found\n> with Peff's UNUSED() series.\n\nCould you point me to the relevant mail? I am sure that I can come up with\nan MCVE that will help them pinpoint the bug and fix it as quickly as they\ndid with this here bug.\n\nStill amazed at the turn-around time,\nDscho\n"},{"id":"462244","messageId":"xmqqbks1ifmw.fsf@gitster.g","threadId":"58377","inReplyTo":"73spp934-1q41-1123-41no-q2337954op92@tzk.qr","subject":"Re: validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-30T21:29:59Z","receivedAt":"2022-08-30T21:30:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Phillip,\n>\n> On Tue, 30 Aug 2022, Phillip Wood wrote:\n>\n>> I wonder if they would be interested in fixing the parsing bug we found\n>> with Peff's UNUSED() series.\n>\n> Could you point me to the relevant mail? I am sure that I can come up with\n> an MCVE that will help them pinpoint the bug and fix it as quickly as they\n> did with this here bug.\n>\n> Still amazed at the turn-around time,\n> Dscho\n\nhttps://lore.kernel.org/git/xmqqwnax438x.fsf@gitster.g\n\n"},{"id":"462245","messageId":"Yw6B6OB5gJhPVn5/@coredump.intra.peff.net","threadId":"58377","inReplyTo":"73spp934-1q41-1123-41no-q2337954op92@tzk.qr","subject":"Re: validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-30T21:32:24Z","receivedAt":"2022-08-30T21:32:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 30, 2022 at 11:12:24PM +0200, Johannes Schindelin wrote:\n\n> On Tue, 30 Aug 2022, Phillip Wood wrote:\n> \n> > I wonder if they would be interested in fixing the parsing bug we found\n> > with Peff's UNUSED() series.\n> \n> Could you point me to the relevant mail? I am sure that I can come up with\n> an MCVE that will help them pinpoint the bug and fix it as quickly as they\n> did with this here bug.\n\nIt's the sub-thread starting here:\n\n https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/\n\nI tried to put together a minimal example, which would be something\nlike:\n\n $ cat foo.c\n void old_function(int);\n void new_function(int);\n \n #if defined(__GNUC__)\n #define UNUSED(var) UNUSED_##var __attribute__((unused))\n #else\n #define UNUSED(var) UNUSED_##var\n #endif\n \n void foo(int UNUSED(bar), int x)\n {\n \told_function(x);\n }\n\n\n $ cat foo.cocci\n @@\n expression E;\n @@\n -old_function(E)\n +new_function(E)\n\nbut to my frustration, it actually works! If you add --verbose-parsing\nto the spatch command, you can see it complaining about some of the\nparsing. And it produces wrong outputs in the real-world with our actual\nunused.cocci file. I suspect the example semantic patch might just need\nto be a little more complicated.\n\n-Peff\n"},{"id":"462247","messageId":"xmqqzgflh0b5.fsf@gitster.g","threadId":"58377","inReplyTo":"xmqqbks1ifmw.fsf@gitster.g","subject":"Re: validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-30T21:46:22Z","receivedAt":"2022-08-30T21:46:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> Hi Phillip,\n>>\n>> On Tue, 30 Aug 2022, Phillip Wood wrote:\n>>\n>>> I wonder if they would be interested in fixing the parsing bug we found\n>>> with Peff's UNUSED() series.\n>>\n>> Could you point me to the relevant mail? I am sure that I can come up with\n>> an MCVE that will help them pinpoint the bug and fix it as quickly as they\n>> did with this here bug.\n\nI think the reference Peff posted is much closer to the description\nof the \"parsing bug\". Sorry for the noise.\n\n>> Still amazed at the turn-around time,\n>> Dscho\n\nYes, indeed.\n\nThanks.\n"}]}