{"thread":{"id":"59072","subject":"[PATCH v1 0/3] fixes for commented out code in tests (was \"Re: [PATCH] *: fix typos which duplicate a word\")","startedAt":"2023-01-11T23:32:52Z","lastAt":"2023-01-14T09:44:27Z","messageCount":8,"participants":["Andrei Rybak","Tim Schumacher","Elijah Newren","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"470126","messageId":"20230111233242.16870-1-rybak.a.v@gmail.com","threadId":"59072","inReplyTo":null,"subject":"[PATCH v1 0/3] fixes for commented out code in tests (was \"Re: [PATCH] *: fix typos which duplicate a word\")","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-01-11T23:32:39Z","receivedAt":"2023-01-11T23:32:52Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"[ I apologize for some people getting this twice -- I messed up when\n invoking `git send-email` ]\n\nOn 2023-01-06T19:25, Eric Sunshine wrote:\n> Not related to your patch at all, but I notice in this test that the\n> call to test_when_finished() is commented out:\n> \n> # test_when_finished \"stop_daemon_delete_repo test_insensitive\" &&\n> \n> which makes me wonder if it was commented out while the test was being\n> debugged but then forgotten, and that the script is now potentially\n> leaking a running daemon if something in the test fails after the\n> daemon was started, or if the daemon does not shut down on its own as\n> it's supposed to do. [cc:+Jeff Hostetler]\n\nHere's a patch series that fixes some of the commented out test code.\n\nI skipped changing the following:\n\n1. a minute-long test_expect_failure is commented out in t0014-alias.sh .\n Technically, this could be uncommented and marked with `EXPENSIVE`\n prerequisite, but it doesn't seem worth it for a `test_expect_failure`.\n [ cc Tim Schumacher, who added this test in fef5f7fc43 (t0014: introduce an\n alias testing suite, 2018-09-16) ]\n\n2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify\n b & add c VS rename b->c' is commented out with an explicit \"# FIXME:\n rename/add conflicts are horribly broken right now;\" above the commented out\n part.\n [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether\n updates can be skipped in a merge, 2018-04-19) ]\n\n3. I've experimented a bit with commented out test in file\n t9200-git-cvsexportcommit.sh. Tests in this file rely on state from previous\n tests, which complicates more thorough investigation. Unfortunately, I\n didn't have bandwidth to investigate this further.\n [ cc Robin Rosenberg, who added it in fe142b3a45 (Rework cvsexportcommit to\n handle binary files for all cases., 2006-11-12) and commented out in\n e86ad71fe5 (Make cvsexportcommit work with filenames with spaces and\n non-ascii characters., 2006-12-11) ]\n\nI found these places in tests with:\n\n git grep -P '^\\s*[#]\\s*test_' -- 't/t[0-9]*'\n\nThe rest of the results of this grep are documentation comments.\n\nAndrei Rybak (3):\n t6003: uncomment test '--max-age=c3, --topo-order'\n t6422: drop commented out code\n t7527: uncomment test_when_finished step in a test\n\n t/t6003-rev-list-topo-order.sh | 23 ++++++++++-------------\n t/t6422-merge-rename-corner-cases.sh | 2 --\n t/t7527-builtin-fsmonitor.sh | 2 +-\n 3 files changed, 11 insertions(+), 16 deletions(-)\n\n-- \n2.39.0\n\n"},{"id":"470127","messageId":"20230111233242.16870-2-rybak.a.v@gmail.com","threadId":"59072","inReplyTo":"20230111233242.16870-1-rybak.a.v@gmail.com","subject":"[PATCH v1 1/3] t6003: uncomment test '--max-age=c3, --topo-order'","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-01-11T23:32:40Z","receivedAt":"2023-01-11T23:32:54Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Test '--max-age=c3, --topo-order' in t6003-rev-list-topo-order.sh has\nbeen commented out as failing since its introduction in [1]. However,\nthe test is successful at least since commit [2] -- bisecting further is\nharder because of incompatibility of such old Git code with modern\nheader file [3].\n\nUncomment this test to gain test coverage.\n\n[1] f573571a21 ([PATCH] Add t/t6003 with some --topo-order tests,\n 2005-07-07)\n[2] 765ac8ec46 (Rip out merge-order and make \"git log ...\" work\n again., 2006-02-28)\n[3] BIGNUM used in git's `epoch.c` which was removed in [2] changed\n significantly between OpenSSL 1.0.2 and OpenSSL 1.1.0\n See also https://stackoverflow.com/a/42295243/1083697 and\n https://lore.kernel.org/git/Y71qiCs+oAS2OegH@coredump.intra.peff.net/\n\nSigned-off-by: Andrei Rybak \n---\n t/t6003-rev-list-topo-order.sh | 23 ++++++++++-------------\n 1 file changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t6003-rev-list-topo-order.sh b/t/t6003-rev-list-topo-order.sh\nindex 1f7d7dd20c..5cf2cee74d 100755\n--- a/t/t6003-rev-list-topo-order.sh\n+++ b/t/t6003-rev-list-topo-order.sh\n@@ -326,19 +326,16 @@ a2\n c3\n EOF\n \n-#\n-# this test fails on --topo-order - a fix is required\n-#\n-#test_output_expect_success '--max-age=c3, --topo-order' \"git rev-list --topo-order --max-age=$(commit_date c3) l5\" <\n\t'\n\nwith corresponding Shell functions\n\n\ttest_setup_... () {\n\t\t\n\t}\n\nDuring this replacement first and last lines of one of such tests got\nleft commented out in code. Drop these lines to avoid confusion.\n\n[1] da1e295e00 (t604[236]: do not run setup in separate tests, 2019-10-22)\n\nSigned-off-by: Andrei Rybak \n---\n t/t6422-merge-rename-corner-cases.sh | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh\nindex 346253c7c8..076b6a74d5 100755\n--- a/t/t6422-merge-rename-corner-cases.sh\n+++ b/t/t6422-merge-rename-corner-cases.sh\n@@ -1159,7 +1159,6 @@ test_conflicts_with_adds_and_renames() {\n \t# 4) There should not be any three~* files in the working\n \t# tree\n \ttest_setup_collision_conflict () {\n-\t#test_expect_success \"setup simple $sideL/$sideR conflict\" '\n \t\tgit init simple_${sideL}_${sideR} &&\n \t\t(\n \t\t\tcd simple_${sideL}_${sideR} &&\n@@ -1236,7 +1235,6 @@ test_conflicts_with_adds_and_renames() {\n \t\t\tfi &&\n \t\t\ttest_tick && git commit -m R\n \t\t)\n-\t#'\n \t}\n \n \ttest_expect_success \"check simple $sideL/$sideR conflict\" '\n-- \n2.39.0\n\n"},{"id":"470129","messageId":"20230111233242.16870-4-rybak.a.v@gmail.com","threadId":"59072","inReplyTo":"20230111233242.16870-1-rybak.a.v@gmail.com","subject":"[PATCH v1 3/3] t7527: use test_when_finished in 'case insensitive+preserving'","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-01-11T23:32:42Z","receivedAt":"2023-01-11T23:32:57Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Most tests in t7527-builtin-fsmonitor.sh that start a daemon, use the\nhelper function test_when_finished with stop_daemon_delete_repo.\nFunction stop_daemon_delete_repo explicitly stops the daemon. Calling\nit via test_when_finished is needed for tests that don't check daemon's\nautomatic shutdown logic [1] and it is needed to avoid daemons being\nleft running in case of breakage of the logic of automatic shutdown of\nthe daemon.\n\nUnlike these tests, test 'case insensitive+preserving' added in [2] has\na call to function test_when_finished commented out. It was commented\nout in all versions of the patch [2] during development [3]. This seems\nto not be intentional, because neither commit message in [2], nor the\ncomment above the test mention this line being commented out. Compare\nit, for example, to \"# unicode_debug=true\" which is explicitly described\nby a documentation comment above it.\n\nUncomment test_when_finished for stop_daemon_delete_repo in test 'case\ninsensitive+preserving' to ensure that daemons are not left running in\ncases when automatic shutdown logic of daemon itself is broken.\n\n[1] See documentation in \"fsmonitor--daemon.h\" for details.\n[2] caa9c37ec0 (t7527: test FSMonitor on case insensitive+preserving\n file system, 2022-05-26)\n[3] See mailing list thread\n https://lore.kernel.org/git/41f8cbc2ae45cb86e299eb230ad3cb0319256c37.1653601644.git.gitgitgadget@gmail.com/T/#t\n\nSigned-off-by: Andrei Rybak \n---\n t/t7527-builtin-fsmonitor.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex 76d0220daa..2c271b4d7e 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -910,7 +910,7 @@ test_expect_success \"submodule absorbgitdirs implicitly starts daemon\" '\n # the file/directory.\n #\n test_expect_success CASE_INSENSITIVE_FS 'case insensitive+preserving' '\n-#\ttest_when_finished \"stop_daemon_delete_repo test_insensitive\" &&\n+\ttest_when_finished \"stop_daemon_delete_repo test_insensitive\" &&\n \n \tgit init test_insensitive &&\n \n-- \n2.39.0\n\n"},{"id":"470130","messageId":"df736b4c-3773-9f14-f66b-1325688634ab@gmx.de","threadId":"59072","inReplyTo":"20230111233242.16870-1-rybak.a.v@gmail.com","subject":"Re: [PATCH v1 0/3] fixes for commented out code in tests (was \"Re: [PATCH] *: fix typos which duplicate a word\")","fromName":"Tim Schumacher","fromEmail":"timschumi@gmx.de","sentAt":"2023-01-12T00:45:00Z","receivedAt":"2023-01-12T00:45:23Z","isPatch":true,"sender":{"key":"timschumi@gmx.de","avatar":"https://avatars.githubusercontent.com/u/16820960?v=4"},"body":"On 12.01.23 00:32, Andrei Rybak wrote:\n> [...]\n>\n> Here's a patch series that fixes some of the commented out test code.\n>\n> I skipped changing the following:\n>\n> 1. a minute-long test_expect_failure is commented out in t0014-alias.sh .\n> Technically, this could be uncommented and marked with `EXPENSIVE`\n> prerequisite, but it doesn't seem worth it for a `test_expect_failure`.\n> [ cc Tim Schumacher, who added this test in fef5f7fc43 (t0014: introduce an\n> alias testing suite, 2018-09-16) ]\n\nThe reason why this particular test is commented out (and why it\nmentions a run time of one minute) is because support for detecting\nexternal alias loops isn't yet implemented. This means that running that\ntest would spin the test runner until the test times out due to an\nintentional infinite loop.\n\nAs soon as that is implemented properly, git would ideally detect the\nloop after a few iterations at latest, so the test wouldn't require to\nbe marked as 'EXPENSIVE' in the first place.\n\nFor the context of your patches, skipping adjusting this test is most\nlikely fine, as it references currently unimplemented behavior and it\npresumably would require more adjustments anyways before finally being\nenabled.\n\n>\n> [...]\n>\n\nTim\n"},{"id":"470132","messageId":"CABPp-BFxK7SGs3wsOfozSw_Uvr-ynr+x8ciPV2Rmfx6Nr4si6g@mail.gmail.com","threadId":"59072","inReplyTo":"20230111233242.16870-1-rybak.a.v@gmail.com","subject":"Re: [PATCH v1 0/3] fixes for commented out code in tests (was \"Re: [PATCH] *: fix typos which duplicate a word\")","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-12T01:54:01Z","receivedAt":"2023-01-12T01:54:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 11, 2023 at 4:05 PM Andrei Rybak wrote:\n>\n> [ I apologize for some people getting this twice -- I messed up when\n> invoking `git send-email` ]\n>\n> On 2023-01-06T19:25, Eric Sunshine wrote:\n> > Not related to your patch at all, but I notice in this test that the\n> > call to test_when_finished() is commented out:\n> >\n> > # test_when_finished \"stop_daemon_delete_repo test_insensitive\" &&\n> >\n> > which makes me wonder if it was commented out while the test was being\n> > debugged but then forgotten, and that the script is now potentially\n> > leaking a running daemon if something in the test fails after the\n> > daemon was started, or if the daemon does not shut down on its own as\n> > it's supposed to do. [cc:+Jeff Hostetler]\n>\n> Here's a patch series that fixes some of the commented out test code.\n\nPatch 2 is obviously correct. Patches 1 & 3 make sense to me, but it\nwould be nice to have someone familiar with fsmonitor look at #3.\n\nAs to your notes about other related testcases...\n\n> I skipped changing the following:\n[...]\n> 2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify\n> b & add c VS rename b->c' is commented out with an explicit \"# FIXME:\n> rename/add conflicts are horribly broken right now;\" above the commented out\n> part.\n> [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether\n> updates can be skipped in a merge, 2018-04-19) ]\n[...]\n\nYou missed the cc...but I looked up this email since you did cc me for\npatch 2/3.\n\nYeah, the commented out code was never tested, because the only thing\nI could have tested at the time was incorrect results. So I just took\na guess at what the improved testing would look like and apparently\nmade 3 small errors in doing so. I have fixed it locally and can\nsubmit a patch.\n"},{"id":"470236","messageId":"CABPp-BExVRPjO9DsFsqk8NhKcFcS=mxG91VT8HnPHfW0=XyC7A@mail.gmail.com","threadId":"59072","inReplyTo":"CABPp-BFxK7SGs3wsOfozSw_Uvr-ynr+x8ciPV2Rmfx6Nr4si6g@mail.gmail.com","subject":"Re: [PATCH v1 0/3] fixes for commented out code in tests (was \"Re: [PATCH] *: fix typos which duplicate a word\")","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-13T04:32:39Z","receivedAt":"2023-01-13T04:35:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 11, 2023 at 5:54 PM Elijah Newren wrote:\n>\n> On Wed, Jan 11, 2023 at 4:05 PM Andrei Rybak wrote:\n> >\n> > [ I apologize for some people getting this twice -- I messed up when\n> > invoking `git send-email` ]\n> >\n> > On 2023-01-06T19:25, Eric Sunshine wrote:\n> > > Not related to your patch at all, but I notice in this test that the\n> > > call to test_when_finished() is commented out:\n> > >\n> > > # test_when_finished \"stop_daemon_delete_repo test_insensitive\" &&\n> > >\n> > > which makes me wonder if it was commented out while the test was being\n> > > debugged but then forgotten, and that the script is now potentially\n> > > leaking a running daemon if something in the test fails after the\n> > > daemon was started, or if the daemon does not shut down on its own as\n> > > it's supposed to do. [cc:+Jeff Hostetler]\n> >\n> > Here's a patch series that fixes some of the commented out test code.\n>\n> Patch 2 is obviously correct. Patches 1 & 3 make sense to me, but it\n> would be nice to have someone familiar with fsmonitor look at #3.\n>\n> As to your notes about other related testcases...\n>\n> > I skipped changing the following:\n> [...]\n> > 2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify\n> > b & add c VS rename b->c' is commented out with an explicit \"# FIXME:\n> > rename/add conflicts are horribly broken right now;\" above the commented out\n> > part.\n> > [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether\n> > updates can be skipped in a merge, 2018-04-19) ]\n> [...]\n>\n> You missed the cc...but I looked up this email since you did cc me for\n> patch 2/3.\n>\n> Yeah, the commented out code was never tested, because the only thing\n> I could have tested at the time was incorrect results. So I just took\n> a guess at what the improved testing would look like and apparently\n> made 3 small errors in doing so. I have fixed it locally and can\n> submit a patch.\n\nSubmitted here:\nhttps://lore.kernel.org/git/pull.1462.git.1673584084761.gitgitgadget@gmail.com/\n"},{"id":"470333","messageId":"CAPig+cQ4k4qTE8JFryvc_DK0CAhwb7XjGhPqzMSTojeSqB4Qpw@mail.gmail.com","threadId":"59072","inReplyTo":"20230111233242.16870-4-rybak.a.v@gmail.com","subject":"Re: [PATCH v1 3/3] t7527: use test_when_finished in 'case insensitive+preserving'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-14T09:43:00Z","receivedAt":"2023-01-14T09:44:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 11, 2023 at 7:00 PM Andrei Rybak wrote:\n> Most tests in t7527-builtin-fsmonitor.sh that start a daemon, use the\n> helper function test_when_finished with stop_daemon_delete_repo.\n> Function stop_daemon_delete_repo explicitly stops the daemon. Calling\n> it via test_when_finished is needed for tests that don't check daemon's\n> automatic shutdown logic [1] and it is needed to avoid daemons being\n> left running in case of breakage of the logic of automatic shutdown of\n> the daemon.\n>\n> Unlike these tests, test 'case insensitive+preserving' added in [2] has\n> a call to function test_when_finished commented out. It was commented\n> out in all versions of the patch [2] during development [3]. This seems\n> to not be intentional, because neither commit message in [2], nor the\n> comment above the test mention this line being commented out. Compare\n> it, for example, to \"# unicode_debug=true\" which is explicitly described\n> by a documentation comment above it.\n>\n> Uncomment test_when_finished for stop_daemon_delete_repo in test 'case\n> insensitive+preserving' to ensure that daemons are not left running in\n> cases when automatic shutdown logic of daemon itself is broken.\n>\n> Signed-off-by: Andrei Rybak \n> ---\n> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\n> @@ -910,7 +910,7 @@ test_expect_success \"submodule absorbgitdirs implicitly starts daemon\" '\n> test_expect_success CASE_INSENSITIVE_FS 'case insensitive+preserving' '\n> -# test_when_finished \"stop_daemon_delete_repo test_insensitive\" &&\n> + test_when_finished \"stop_daemon_delete_repo test_insensitive\" &&\n\nNice. Thanks for working on this (and the series as a whole) in\nresponse to a very tangential comment of mine[1] when replying to an\nunrelated patch of yours.\n\n[1]: https://lore.kernel.org/git/CAPig+cTgUPWxMox_nSka52dML6_GHUUoY4HCtcq7+7J0oEyeNw@mail.gmail.com/\n"}]}