{"thread":{"id":"58000","subject":"[PATCH] t3701: two subtests are fixed","startedAt":"2022-06-14T15:32:05Z","lastAt":"2022-06-23T16:33:34Z","messageCount":16,"participants":["Michael J Gruber","Ævar Arnfjörð Bjarmason","Derrick Stolee","Todd Zullinger","Taylor Blau","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"457194","messageId":"cf6aee9acadfb666de6b24b9ed63e1a65bfc009e.1655220242.git.git@grubix.eu","threadId":"58000","inReplyTo":null,"subject":"[PATCH] t3701: two subtests are fixed","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-06-14T15:26:33Z","receivedAt":"2022-06-14T15:32:05Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"0527ccb1b5 (\"add -i: default to the built-in implementation\", 2021-11-30)\nswitched to the implementation which fixed to subtest. Mark them as\nexpect_success now.\n\nSigned-off-by: Michael J Gruber <git@grubix.eu>\n---\nI did check the ML but may have missed a series which contains this. (I\nonly found one which tries to make the test output clearer in CI.)\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 94537a6b40..9a06638704 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -538,7 +538,7 @@ test_expect_success 'split hunk \"add -p (edit)\"' '\n \t! grep \"^+15\" actual\n '\n \n-test_expect_failure 'split hunk \"add -p (no, yes, edit)\"' '\n+test_expect_success 'split hunk \"add -p (no, yes, edit)\"' '\n \ttest_write_lines 5 10 20 21 30 31 40 50 60 >test &&\n \tgit reset &&\n \t# test sequence is s(plit), n(o), y(es), e(dit)\n@@ -562,7 +562,7 @@ test_expect_success 'split hunk with incomplete line at end' '\n \ttest_must_fail git grep --cached before\n '\n \n-test_expect_failure 'edit, adding lines to the first hunk' '\n+test_expect_success 'edit, adding lines to the first hunk' '\n \ttest_write_lines 10 11 20 30 40 50 51 60 >test &&\n \tgit reset &&\n \ttr _ \" \" >patch <<-EOF &&\n-- \n2.37.0.rc0.107.g7a7be657e7\n\n"},{"id":"457197","messageId":"patch-v2-1.1-13c26e546f6-20220614T153746Z-avarab@gmail.com","threadId":"58000","inReplyTo":"cf6aee9acadfb666de6b24b9ed63e1a65bfc009e.1655220242.git.git@grubix.eu","subject":"[PATCH v2] add -i tests: mark \"TODO\" depending on GIT_TEST_ADD_I_USE_BUILTIN","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-14T15:40:07Z","receivedAt":"2022-06-14T15:41:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix an issue that existed before 0527ccb1b55 (add -i: default to the\nbuilt-in implementation, 2021-11-30), but which became the default\nwith that change, we should not be marking tests that are known to\npass as \"TODO\" tests.\n\nWhen GIT_TEST_ADD_I_USE_BUILTIN=1 was made the default we started\npassing the tests added in 0f0fba2cc87 (t3701: add a test for advanced\nsplit-hunk editing, 2019-12-06) and 1bf01040f0c (add -p: demonstrate\nfailure when running 'edit' after a split, 2015-04-16).\n\nThus we've been emitting this sort of output:\n\n\t$ prove ./t3701-add-interactive.sh\n\t./t3701-add-interactive.sh .. ok\n\tAll tests successful.\n\n\tTest Summary Report\n\t-------------------\n\t./t3701-add-interactive.sh (Wstat: 0 Tests: 70 Failed: 0)\n\t  TODO passed:   45, 47\n\tFiles=1, Tests=70,  2 wallclock secs ( 0.03 usr  0.00 sys +  0.86 cusr  0.33 csys =  1.22 CPU)\n\tResult: PASS\n\nWhich isn't just cosmetic, but due to issues with\ntest_expect_failure (see [1]) we could e.g. be hiding something as bad\nas a segfault in the new implementation. It makes sense catch that,\nespecially before we put out a release with the built-in \"add -i\", so\nlet's generalize the check we were already doing in 0527ccb1b55 with a\nnew \"ADD_I_USE_BUILTIN\" prerequisite.\n\n1. https://lore.kernel.org/git/patch-1.7-4624abc2591-20220318T002951Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nOn Tue, Jun 14 2022, Michael J Gruber wrote:\n\n> 0527ccb1b5 (\"add -i: default to the built-in implementation\", 2021-11-30)\n> switched to the implementation which fixed to subtest. Mark them as\n> expect_success now.\n>\n> Signed-off-by: Michael J Gruber <git@grubix.eu>\n> ---\n> I did check the ML but may have missed a series which contains this. (I\n> only found one which tries to make the test output clearer in CI.)\n\nI was looking at the same earlier and came up with this (before seeing\nyour patch here), so a proposed v2 I suppose.\n\nJust converting it to \"test_expect_success\" will break CI and other\nsetups that are testing with GIT_TEST_ADD_I_USE_BUILTIN=false.\n\nThe below fixes it, however.\n\n t/t2016-checkout-patch.sh  |  2 +-\n t/t3701-add-interactive.sh | 12 ++++++++++--\n t/test-lib.sh              |  4 ++++\n 3 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex bc3f69b4b1d..a5822e41af2 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -4,7 +4,7 @@ test_description='git checkout --patch'\n \n . ./lib-patch-mode.sh\n \n-if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL\n+if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL\n then\n \tskip_all='skipping interactive add tests, PERL not set'\n \ttest_done\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 94537a6b40a..fc26cb8bae8 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -538,7 +538,15 @@ test_expect_success 'split hunk \"add -p (edit)\"' '\n \t! grep \"^+15\" actual\n '\n \n-test_expect_failure 'split hunk \"add -p (no, yes, edit)\"' '\n+test_expect_success 'setup ADD_I_USE_BUILTIN check' '\n+\tresult=success &&\n+\tif ! test_have_prereq ADD_I_USE_BUILTIN\n+\tthen\n+\t\tresult=failure\n+\tfi\n+'\n+\n+test_expect_$result 'split hunk \"add -p (no, yes, edit)\"' '\n \ttest_write_lines 5 10 20 21 30 31 40 50 60 >test &&\n \tgit reset &&\n \t# test sequence is s(plit), n(o), y(es), e(dit)\n@@ -562,7 +570,7 @@ test_expect_success 'split hunk with incomplete line at end' '\n \ttest_must_fail git grep --cached before\n '\n \n-test_expect_failure 'edit, adding lines to the first hunk' '\n+test_expect_$result 'edit, adding lines to the first hunk' '\n \ttest_write_lines 10 11 20 30 40 50 51 60 >test &&\n \tgit reset &&\n \ttr _ \" \" >patch <<-EOF &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 736c6447ecf..f5291ef56ef 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1759,6 +1759,10 @@ test_lazy_prereq SHA1 '\n \tesac\n '\n \n+test_lazy_prereq ADD_I_USE_BUILTIN '\n+\ttest_bool_env GIT_TEST_ADD_I_USE_BUILTIN true\n+'\n+\n # Ensure that no test accidentally triggers a Git command\n # that runs the actual maintenance scheduler, affecting a user's\n # system permanently.\n-- \n2.36.1.1239.gfba91521d90\n\n"},{"id":"457199","messageId":"b7d4dc9f-613c-17b6-f3e7-83cbd88a24db@github.com","threadId":"58000","inReplyTo":"cf6aee9acadfb666de6b24b9ed63e1a65bfc009e.1655220242.git.git@grubix.eu","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-14T15:48:46Z","receivedAt":"2022-06-14T15:48:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/14/2022 11:26 AM, Michael J Gruber wrote:\n> 0527ccb1b5 (\"add -i: default to the built-in implementation\", 2021-11-30)\n> switched to the implementation which fixed to subtest. Mark them as\n> expect_success now.\n\ns/to subtest/two subtests/\n\n> \n> Signed-off-by: Michael J Gruber <git@grubix.eu>\n> ---\n> I did check the ML but may have missed a series which contains this. (I\n> only found one which tries to make the test output clearer in CI.)\n\nThe breakage vanished as of 1fc1879839 (Merge branch 'js/use-builtin-add-i',\n2022-05-30). The direct change is likely 0527ccb1b5 (add -i: default to the\nbuilt-in implementation, 2021-11-30), but that commit actually fails the\ntests, it seems. Something about a parallel topic must have made it work at\nthe merge point.\n\nPatch looks good. Thanks!\n\n-Stolee\n"},{"id":"457237","messageId":"Yqk1GCPkfauGHQQB@pobox.com","threadId":"58000","inReplyTo":"cf6aee9acadfb666de6b24b9ed63e1a65bfc009e.1655220242.git.git@grubix.eu","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2022-06-15T01:25:44Z","receivedAt":"2022-06-15T01:25:56Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Michael J Gruber wrote:\n> 0527ccb1b5 (\"add -i: default to the built-in implementation\", 2021-11-30)\n> switched to the implementation which fixed to subtest. Mark them as\n> expect_success now.\n> \n> Signed-off-by: Michael J Gruber <git@grubix.eu>\n> ---\n> I did check the ML but may have missed a series which contains this. (I\n> only found one which tries to make the test output clearer in CI.)\n\nI sent a patch (<20220614185218.1091413-1-tmz@pobox.com>) as\nwell.  I mentioned the commits which added these tests, but\ndidn't call out 0527ccb1b5 (add -i: default to the built-in\nimplementation, 2021-11-30) explicitly, which is a good\naddition.\n\nI'm just happy to see the builtin `add -i` as the default.\n\n>  t/t3701-add-interactive.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 94537a6b40..9a06638704 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -538,7 +538,7 @@ test_expect_success 'split hunk \"add -p (edit)\"' '\n>  \t! grep \"^+15\" actual\n>  '\n>  \n> -test_expect_failure 'split hunk \"add -p (no, yes, edit)\"' '\n> +test_expect_success 'split hunk \"add -p (no, yes, edit)\"' '\n>  \ttest_write_lines 5 10 20 21 30 31 40 50 60 >test &&\n>  \tgit reset &&\n>  \t# test sequence is s(plit), n(o), y(es), e(dit)\n> @@ -562,7 +562,7 @@ test_expect_success 'split hunk with incomplete line at end' '\n>  \ttest_must_fail git grep --cached before\n>  '\n>  \n> -test_expect_failure 'edit, adding lines to the first hunk' '\n> +test_expect_success 'edit, adding lines to the first hunk' '\n>  \ttest_write_lines 10 11 20 30 40 50 51 60 >test &&\n>  \tgit reset &&\n>  \ttr _ \" \" >patch <<-EOF &&\n\n-- \nTodd\n"},{"id":"457239","messageId":"Yqk7/HXsrqYK/2QR@nand.local","threadId":"58000","inReplyTo":"cf6aee9acadfb666de6b24b9ed63e1a65bfc009e.1655220242.git.git@grubix.eu","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-15T01:55:08Z","receivedAt":"2022-06-15T01:55:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 14, 2022 at 05:26:33PM +0200, Michael J Gruber wrote:\n> 0527ccb1b5 (\"add -i: default to the built-in implementation\", 2021-11-30)\n> switched to the implementation which fixed to subtest. Mark them as\n> expect_success now.\n\nSince v2.37.0-rc0 is the first tag to contain 0527ccb1b5, I bisected\nbetween v2.36 (when these two tests indeed failed) and the tip of\nmaster (8168d5e9c2 (Git 2.37-rc0, 2022-06-13) at the time of writing).\n\nAnd I also got 0527ccb1b5, so the bisection looks good to me, and this\nwas likely an oversight when 0527ccb1b5 was written. Thanks for putting\nthe author on the CC list just in case there is any additional context.\n\nOtherwise, this patch looks good to me.\n\nThanks,\nTaylor\n"},{"id":"457244","messageId":"YqlIRveupj6tOO4P@pobox.com","threadId":"58000","inReplyTo":"patch-v2-1.1-13c26e546f6-20220614T153746Z-avarab@gmail.com","subject":"Re: [PATCH v2] add -i tests: mark \"TODO\" depending on GIT_TEST_ADD_I_USE_BUILTIN","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2022-06-15T02:47:34Z","receivedAt":"2022-06-15T02:49:43Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> Fix an issue that existed before 0527ccb1b55 (add -i: default to the\n> built-in implementation, 2021-11-30), but which became the default\n> with that change, we should not be marking tests that are known to\n> pass as \"TODO\" tests.\n[...]\n> ---\n> Just converting it to \"test_expect_success\" will break CI and other\n> setups that are testing with GIT_TEST_ADD_I_USE_BUILTIN=false.\n> \n> The below fixes it, however.\n\nNice catch.  FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0\nand without.\n\n> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\n> index bc3f69b4b1d..a5822e41af2 100755\n> --- a/t/t2016-checkout-patch.sh\n> +++ b/t/t2016-checkout-patch.sh\n> @@ -4,7 +4,7 @@ test_description='git checkout --patch'\n>  \n>  . ./lib-patch-mode.sh\n>  \n> -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL\n> +if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL\n>  then\n>  \tskip_all='skipping interactive add tests, PERL not set'\n\nIt's not the fault of this patch, but it makes it obvious\nthat the `skip_all` message is no longer accurate.  Perhaps\nsomethine like this?\n\n    skip_all='skipping interactive add tests, missing ADD_I_USE_BUILTIN or PERL'\n\nMaybe a separate `ADD_I` prereq would be better?  Though\nwithout looking closer, I don't know if that would end up\nbeing clearer to anyone running the tests without either\nPERL or the add -i builtin enabled.\n\nThanks for the keen eye and attention to detail, Ævar,\n\n-- \nTodd\n"},{"id":"457286","messageId":"nycvar.QRO.7.76.6.2206151649030.349@tvgsbejvaqbjf.bet","threadId":"58000","inReplyTo":"cf6aee9acadfb666de6b24b9ed63e1a65bfc009e.1655220242.git.git@grubix.eu","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-06-15T14:50:40Z","receivedAt":"2022-06-15T14:50:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Tue, 14 Jun 2022, Michael J Gruber wrote:\n\n> 0527ccb1b5 (\"add -i: default to the built-in implementation\", 2021-11-30)\n> switched to the implementation which fixed to subtest. Mark them as\n> expect_success now.\n\nGood catch!\n\nHowever... that commit specifically contains this change:\n\n\tdiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n\tindex cc62616d806..660ebe8d108 100755\n\t--- a/ci/run-build-and-tests.sh\n\t+++ b/ci/run-build-and-tests.sh\n\t@@ -29,7 +29,7 @@ linux-gcc)\n\t\texport GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1\n\t\texport GIT_TEST_MULTI_PACK_INDEX=1\n\t\texport GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1\n\t-       export GIT_TEST_ADD_I_USE_BUILTIN=1\n\t+       export GIT_TEST_ADD_I_USE_BUILTIN=0\n\t\texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n\t\texport GIT_TEST_WRITE_REV_INDEX=1\n\t\texport GIT_TEST_CHECKOUT_WORKERS=2\n\nThe intention is to have t3701 be run with the non-built-in version of\n`git add -i` in the `linux-gcc` job, and I am surprised that those two\ntests do not fail for you in that case.\n\nDid you run this through the CI builds?\n\nThank you,\nDscho\n"},{"id":"457362","messageId":"165537087609.19905.821171947957640468.git@grubix.eu","threadId":"58000","inReplyTo":"nycvar.QRO.7.76.6.2206151649030.349@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-06-16T09:14:36Z","receivedAt":"2022-06-16T09:23:53Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Johannes Schindelin venit, vidit, dixit 2022-06-15 16:50:40:\n> Hi Michael,\n\nHallo Dscho!\n\n> On Tue, 14 Jun 2022, Michael J Gruber wrote:\n> \n> > 0527ccb1b5 (\"add -i: default to the built-in implementation\", 2021-11-30)\n> > switched to the implementation which fixed to subtest. Mark them as\n> > expect_success now.\n> \n> Good catch!\n \nI'm no list regular anymore, but still a \"next+ regular\". While\nexperimenting with my own patch I noticed something got fixed\nunexpectedly. That goes to show that these unexpected successes\n(from expect_failure) go unnoticed too easily. I had missed this on my\nregular rebuilds.\n\n> However... that commit specifically contains this change:\n> \n>         diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n>         index cc62616d806..660ebe8d108 100755\n>         --- a/ci/run-build-and-tests.sh\n>         +++ b/ci/run-build-and-tests.sh\n>         @@ -29,7 +29,7 @@ linux-gcc)\n>                 export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1\n>                 export GIT_TEST_MULTI_PACK_INDEX=1\n>                 export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1\n>         -       export GIT_TEST_ADD_I_USE_BUILTIN=1\n>         +       export GIT_TEST_ADD_I_USE_BUILTIN=0\n>                 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n>                 export GIT_TEST_WRITE_REV_INDEX=1\n>                 export GIT_TEST_CHECKOUT_WORKERS=2\n> \n> The intention is to have t3701 be run with the non-built-in version of\n> `git add -i` in the `linux-gcc` job, and I am surprised that those two\n> tests do not fail for you in that case.\n> \n> Did you run this through the CI builds?\n\nThat's why I mentioned \"no list regular\" - I didn't know about that knob\nnor the intention to have the test suite run with either implementation\n(rather than switching to the new one for good).\n\nI do local builds, usually with\n\n```\nDEVELOPER=1 (which I had to disable during the bisect run; gcc12...)\nDEFAULT_TEST_TARGET=prove\nGIT_PROVE_OPTS=--jobs 4\nGIT_TEST_OPTS=--root=/dev/shm/t --chain-lint\nSHELL_PATH=/bin/dash\nSKIP_DASHED_BUILT_INS=y\n```\n\nin config.mak. Nothing else strikes me as potentially relevant.\n\nÆvar noticed this and has a better version of my patch, I think.\n\nMichael\n"},{"id":"457364","messageId":"220616.86sfo4x5zw.gmgdl@evledraar.gmail.com","threadId":"58000","inReplyTo":"YqlIRveupj6tOO4P@pobox.com","subject":"Re: [PATCH v2] add -i tests: mark \"TODO\" depending on GIT_TEST_ADD_I_USE_BUILTIN","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-16T10:16:46Z","receivedAt":"2022-06-16T10:22:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 14 2022, Todd Zullinger wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> Fix an issue that existed before 0527ccb1b55 (add -i: default to the\n>> built-in implementation, 2021-11-30), but which became the default\n>> with that change, we should not be marking tests that are known to\n>> pass as \"TODO\" tests.\n> [...]\n>> ---\n>> Just converting it to \"test_expect_success\" will break CI and other\n>> setups that are testing with GIT_TEST_ADD_I_USE_BUILTIN=false.\n>> \n>> The below fixes it, however.\n>\n> Nice catch.  FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0\n> and without.\n\nMy patch landed on \"master\" as 7ccbea564e8 (add -i tests: mark \"TODO\"\ndepending on GIT_TEST_ADD_I_USE_BUILTIN, 2022-06-14) so this is water\nunder the bridge.\n\nBut just to tie this loose knot I think something went wrong in your\ntesting.\n\nIf I:\n\n    git checkout v2.37.0-rc0\n    # Apply your patch from <20220614185218.1091413-1-tmz@pobox.com>\n\nI'll consistently get a failure from:\n\n    GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh\n\nSince we do fail that test with the Perl implementation, and now it's no\nlonger a TODO test.\n\nPerhaps you used it as a parameter to \"make\"? I.e.:\n\n    make GIT_TEST_ADD_I_USE_BUILTIN=false\n    make test\n\nWhich isn't how it works, just speculating...\n \n>> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\n>> index bc3f69b4b1d..a5822e41af2 100755\n>> --- a/t/t2016-checkout-patch.sh\n>> +++ b/t/t2016-checkout-patch.sh\n>> @@ -4,7 +4,7 @@ test_description='git checkout --patch'\n>>  \n>>  . ./lib-patch-mode.sh\n>>  \n>> -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL\n>> +if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL\n>>  then\n>>  \tskip_all='skipping interactive add tests, PERL not set'\n>\n> It's not the fault of this patch, but it makes it obvious\n> that the `skip_all` message is no longer accurate.  Perhaps\n> somethine like this?\n>\n>     skip_all='skipping interactive add tests, missing ADD_I_USE_BUILTIN or PERL'\n>\n> Maybe a separate `ADD_I` prereq would be better?  Though\n> without looking closer, I don't know if that would end up\n> being clearer to anyone running the tests without either\n> PERL or the add -i builtin enabled.\n\nYeah seems like a good idea for a follow-up, but since it's landed I'll\nprobably forget :)\n\n> Thanks for the keen eye and attention to detail, Ævar,\n\nHappy to have it fixed!\n"},{"id":"457376","messageId":"Yqs0fA7FOA3WuaiR@pobox.com","threadId":"58000","inReplyTo":"220616.86sfo4x5zw.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] add -i tests: mark \"TODO\" depending on GIT_TEST_ADD_I_USE_BUILTIN","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2022-06-16T13:47:40Z","receivedAt":"2022-06-16T13:47:50Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Jun 14 2022, Todd Zullinger wrote:\n>> Nice catch.  FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0\n>> and without.\n> \n> My patch landed on \"master\" as 7ccbea564e8 (add -i tests: mark \"TODO\"\n> depending on GIT_TEST_ADD_I_USE_BUILTIN, 2022-06-14) so this is water\n> under the bridge.\n> \n> But just to tie this loose knot I think something went wrong in your\n> testing.\n> \n> If I:\n> \n>     git checkout v2.37.0-rc0\n>     # Apply your patch from <20220614185218.1091413-1-tmz@pobox.com>\n> \n> I'll consistently get a failure from:\n> \n>     GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh\n> \n\nSorry for being unclear.  I meant that I tested your patch.\nThe patch I sent didn't handle that case. :)\n\n-- \nTodd\n"},{"id":"457387","messageId":"xmqqsfo4v9gs.fsf@gitster.g","threadId":"58000","inReplyTo":"165537087609.19905.821171947957640468.git@grubix.eu","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T16:50:27Z","receivedAt":"2022-06-16T16:50:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@grubix.eu> writes:\n\n> Johannes Schindelin venit, vidit, dixit 2022-06-15 16:50:40:\n>> Hi Michael,\n>\n> Hallo Dscho!\n>\n>> On Tue, 14 Jun 2022, Michael J Gruber wrote:\n>> \n>> > 0527ccb1b5 (\"add -i: default to the built-in implementation\", 2021-11-30)\n>> > switched to the implementation which fixed to subtest. Mark them as\n>> > expect_success now.\n>> \n>> Good catch!\n>  \n> I'm no list regular anymore, but still a \"next+ regular\". While\n> experimenting with my own patch I noticed something got fixed\n> unexpectedly. That goes to show that these unexpected successes\n> (from expect_failure) go unnoticed too easily. I had missed this on my\n> regular rebuilds.\n\nThanks for being a \"next+ regular\".  They are giving us a valuable\nservice to catch bugs and questionable design decisions before they\nhit the \"master\" branch.\n\n> Ævar noticed this and has a better version of my patch, I think.\n\nYup.  Eventually we will make it even impossible to opt out of the\nbuilt-in variant, but until then, we'd need the conditional stuff.\n\nThanks.\n"},{"id":"457500","messageId":"nycvar.QRO.7.76.6.2206181342200.349@tvgsbejvaqbjf.bet","threadId":"58000","inReplyTo":"165537087609.19905.821171947957640468.git@grubix.eu","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-06-18T11:55:24Z","receivedAt":"2022-06-18T11:56:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Thu, 16 Jun 2022, Michael J Gruber wrote:\n\n> Johannes Schindelin venit, vidit, dixit 2022-06-15 16:50:40:\n>\n> > On Tue, 14 Jun 2022, Michael J Gruber wrote:\n> >\n> > > 0527ccb1b5 (\"add -i: default to the built-in implementation\", 2021-11-30)\n> > > switched to the implementation which fixed to subtest. Mark them as\n> > > expect_success now.\n> >\n> > Good catch!\n>\n> I'm no list regular anymore, but still a \"next+ regular\". While\n> experimenting with my own patch I noticed something got fixed\n> unexpectedly. That goes to show that these unexpected successes\n> (from expect_failure) go unnoticed too easily. I had missed this on my\n> regular rebuilds.\n\nMakes sense.\n\n> > However... that commit specifically contains this change:\n> >\n> >         diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n> >         index cc62616d806..660ebe8d108 100755\n> >         --- a/ci/run-build-and-tests.sh\n> >         +++ b/ci/run-build-and-tests.sh\n> >         @@ -29,7 +29,7 @@ linux-gcc)\n> >                 export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1\n> >                 export GIT_TEST_MULTI_PACK_INDEX=1\n> >                 export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1\n> >         -       export GIT_TEST_ADD_I_USE_BUILTIN=1\n> >         +       export GIT_TEST_ADD_I_USE_BUILTIN=0\n> >                 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n> >                 export GIT_TEST_WRITE_REV_INDEX=1\n> >                 export GIT_TEST_CHECKOUT_WORKERS=2\n> >\n> > The intention is to have t3701 be run with the non-built-in version of\n> > `git add -i` in the `linux-gcc` job, and I am surprised that those two\n> > tests do not fail for you in that case.\n> >\n> > Did you run this through the CI builds?\n>\n> That's why I mentioned \"no list regular\" - I didn't know about that knob\n> nor the intention to have the test suite run with either implementation\n> (rather than switching to the new one for good).\n>\n> I do local builds, usually with\n>\n> ```\n> DEVELOPER=1 (which I had to disable during the bisect run; gcc12...)\n> DEFAULT_TEST_TARGET=prove\n> GIT_PROVE_OPTS=--jobs 4\n> GIT_TEST_OPTS=--root=/dev/shm/t --chain-lint\n> SHELL_PATH=/bin/dash\n> SKIP_DASHED_BUILT_INS=y\n> ```\n>\n> in config.mak. Nothing else strikes me as potentially relevant.\n>\n> Ævar noticed this and has a better version of my patch, I think.\n\nSo you did not find it utterly rude and presumptuous that somebody sent a\nnew iteration of your patch without even so much as consulting with you\nwhether you're okay with this? I salute your forbearance, then.\n\nBesides, it is not really a better version of your patch. That would have\nbeen:\n\n-- snip --\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 94537a6b40a..6d1032fe8ae 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -538,7 +538,9 @@ test_expect_success 'split hunk \"add -p (edit)\"' '\n \t! grep \"^+15\" actual\n '\n\n-test_expect_failure 'split hunk \"add -p (no, yes, edit)\"' '\n+test_lazy_prereq BUILTIN_ADD_I 'test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true'\n+\n+test_expect_success BUILTIN_ADD_I 'split hunk \"add -p (no, yes, edit)\"' '\n \ttest_write_lines 5 10 20 21 30 31 40 50 60 >test &&\n \tgit reset &&\n \t# test sequence is s(plit), n(o), y(es), e(dit)\n@@ -562,7 +564,7 @@ test_expect_success 'split hunk with incomplete line at end' '\n \ttest_must_fail git grep --cached before\n '\n\n-test_expect_failure 'edit, adding lines to the first hunk' '\n+test_expect_failure BUILTIN_ADD_I 'edit, adding lines to the first hunk' '\n \ttest_write_lines 10 11 20 30 40 50 51 60 >test &&\n \tgit reset &&\n \ttr _ \" \" >patch <<-EOF &&\n-- snap --\n\nAs you can see, this is _actually_ building on your work rather than\nreplacing it.\n\nBut since that replacement made it into -rc1, I will stop spending brain\ncycles on it.\n\nThank you for your contribution, I am glad that you keep sending patches\nto the Git mailing list!\nDscho\n"},{"id":"457621","messageId":"xmqq8rpqja0v.fsf@gitster.g","threadId":"58000","inReplyTo":"nycvar.QRO.7.76.6.2206181342200.349@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-21T15:45:04Z","receivedAt":"2022-06-21T15:45:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> in config.mak. Nothing else strikes me as potentially relevant.\n>>\n>> Ævar noticed this and has a better version of my patch, I think.\n>\n> So you did not find it utterly rude and presumptuous that somebody sent a\n> new iteration of your patch without even so much as consulting with you\n> whether you're okay with this? I salute your forbearance, then.\n\nI had an impression that these (wasn't there another one) were\nindependent discoveries and patching that happened at the same time.\n\n> Besides, it is not really a better version of your patch. That would have\n> been:\n>\n> -- snip --\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 94537a6b40a..6d1032fe8ae 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -538,7 +538,9 @@ test_expect_success 'split hunk \"add -p (edit)\"' '\n>  \t! grep \"^+15\" actual\n>  '\n>\n> -test_expect_failure 'split hunk \"add -p (no, yes, edit)\"' '\n> +test_lazy_prereq BUILTIN_ADD_I 'test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true'\n> +\n> +test_expect_success BUILTIN_ADD_I 'split hunk \"add -p (no, yes, edit)\"' '\n>  \ttest_write_lines 5 10 20 21 30 31 40 50 60 >test &&\n>  \tgit reset &&\n>  \t# test sequence is s(plit), n(o), y(es), e(dit)\n\nPrerequisite lets you skip.  \n\nThis stops saying that \"with scripted version 'add -p' does not\nbehave in the way we want to see, and we want to leave us a mental\nnote about it\".  I do not know if that is what we want.\n\nOnce scripted version gets fully retired, it of course stops\nmattering ;-)\n\n> @@ -562,7 +564,7 @@ test_expect_success 'split hunk with incomplete line at end' '\n>  \ttest_must_fail git grep --cached before\n>  '\n>\n> -test_expect_failure 'edit, adding lines to the first hunk' '\n> +test_expect_failure BUILTIN_ADD_I 'edit, adding lines to the first hunk' '\n\nI am not sure if this is a good change, quite honestly.  With\ns/failure/success/, perhaps, but not in the posted form.\n"},{"id":"457687","messageId":"165588628205.6252.637148192313375325.git@grubix.eu","threadId":"58000","inReplyTo":"xmqq8rpqja0v.fsf@gitster.g","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-06-22T08:24:42Z","receivedAt":"2022-06-22T08:24:50Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 2022-06-21 17:45:04:\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> in config.mak. Nothing else strikes me as potentially relevant.\n> >>\n> >> Ævar noticed this and has a better version of my patch, I think.\n> >\n> > So you did not find it utterly rude and presumptuous that somebody sent a\n> > new iteration of your patch without even so much as consulting with you\n> > whether you're okay with this? I salute your forbearance, then.\n> \n> I had an impression that these (wasn't there another one) were\n> independent discoveries and patching that happened at the same time.\n\nYes, while it looked funny at first, Ævar explained it well. So,\neverything is fine for me. Besides, we're no strangers to each other ;)\n\nAs for the question which version covers the expectations best: I'm\nlacking the necessary overview of the expectations (which implementation\nto check by default, in CI etc.) which is why I won't chime in on that.\n"},{"id":"457798","messageId":"nycvar.QRO.7.76.6.2206231747220.349@tvgsbejvaqbjf.bet","threadId":"58000","inReplyTo":"xmqq8rpqja0v.fsf@gitster.g","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-06-23T15:55:44Z","receivedAt":"2022-06-23T15:56:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nI did not want to spend more brain cycles about this, but since you left a\nfew questions hanging...\n\nOn Tue, 21 Jun 2022, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n> >> in config.mak. Nothing else strikes me as potentially relevant.\n> >>\n> >> Ævar noticed this and has a better version of my patch, I think.\n> >\n> > So you did not find it utterly rude and presumptuous that somebody sent a\n> > new iteration of your patch without even so much as consulting with you\n> > whether you're okay with this? I salute your forbearance, then.\n>\n> I had an impression that these (wasn't there another one) were\n> independent discoveries and patching that happened at the same time.\n\nIf this was the first time an unsolicited iteration was sent on another\ncontributor's behalf, I would be able to give the benefit of the doubt.\nEven if it was the second or third time. It's been many more times,\nthough. And it is not leaving the impression of an inviting, welcoming\nculture I would like to see on the Git mailing list. But it's your\nproject to lead, not mine, therefore I have no say in this.\n\n> > -- snip --\n> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> > index 94537a6b40a..6d1032fe8ae 100755\n> > --- a/t/t3701-add-interactive.sh\n> > +++ b/t/t3701-add-interactive.sh\n> > @@ -538,7 +538,9 @@ test_expect_success 'split hunk \"add -p (edit)\"' '\n> >  \t! grep \"^+15\" actual\n> >  '\n> >\n> > -test_expect_failure 'split hunk \"add -p (no, yes, edit)\"' '\n> > +test_lazy_prereq BUILTIN_ADD_I 'test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true'\n> > +\n> > +test_expect_success BUILTIN_ADD_I 'split hunk \"add -p (no, yes, edit)\"' '\n> >  \ttest_write_lines 5 10 20 21 30 31 40 50 60 >test &&\n> >  \tgit reset &&\n> >  \t# test sequence is s(plit), n(o), y(es), e(dit)\n>\n> Prerequisite lets you skip.\n\nYes. It lets you skip a test for a known breakage in code we're never\ngoing to fix because we're going to delete it instead, for example. Saving\nsome electricity, too, by avoiding to run said test case.\n\n> > @@ -562,7 +564,7 @@ test_expect_success 'split hunk with incomplete line at end' '\n> >  \ttest_must_fail git grep --cached before\n> >  '\n> >\n> > -test_expect_failure 'edit, adding lines to the first hunk' '\n> > +test_expect_failure BUILTIN_ADD_I 'edit, adding lines to the first hunk' '\n>\n> I am not sure if this is a good change, quite honestly.  With\n> s/failure/success/, perhaps, but not in the posted form.\n\nIndeed, this was an oversight on my part, as you might have guessed from\nthe `failure` being replaced with `success` in the previous hunk. I simply\nforgot it here.\n\nBut a more complicated solution for the same problem was applied directly\nto the main branch, so I'd like to shift my attention to problems where my\ninput has a chance of mattering.\n\nCiao,\nDscho\n"},{"id":"457804","messageId":"xmqqilor1grs.fsf@gitster.g","threadId":"58000","inReplyTo":"nycvar.QRO.7.76.6.2206231747220.349@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] t3701: two subtests are fixed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-23T16:33:27Z","receivedAt":"2022-06-23T16:33:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> But a more complicated solution for the same problem was applied directly\n> to the main branch, so I'd like to shift my attention to problems where my\n> input has a chance of mattering.\n\nAny reasonable input makes difference.  You can even improve incrementally\nwith follow-up patches.\n\nThanks.\n"}]}