[PATCH v1 0/3] fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

[PATCH v1 0/3] fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

[Andrei Rybak]

[ I apologize for some people getting this twice -- I messed up when
  invoking `git send-email` ]

On 2023-01-06T19:25, Eric Sunshine wrote:
> Not related to your patch at all, but I notice in this test that the
> call to test_when_finished() is commented out:
> 
>     # test_when_finished "stop_daemon_delete_repo test_insensitive" &&
> 
> which makes me wonder if it was commented out while the test was being
> debugged but then forgotten, and that the script is now potentially
> leaking a running daemon if something in the test fails after the
> daemon was started, or if the daemon does not shut down on its own as
> it's supposed to do. [cc:+Jeff Hostetler]

Here's a patch series that fixes some of the commented out test code.

I skipped changing the following:

1. a minute-long test_expect_failure is commented out in t0014-alias.sh .
   Technically, this could be uncommented and marked with `EXPENSIVE`
   prerequisite, but it doesn't seem worth it for a `test_expect_failure`.
   [ cc Tim Schumacher, who added this test in fef5f7fc43 (t0014: introduce an
   alias testing suite, 2018-09-16) ]

2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify
   b & add c VS rename b->c' is commented out with an explicit "# FIXME:
   rename/add conflicts are horribly broken right now;" above the commented out
   part.
   [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether
   updates can be skipped in a merge, 2018-04-19) ]

3. I've experimented a bit with commented out test in file
   t9200-git-cvsexportcommit.sh.  Tests in this file rely on state from previous
   tests, which complicates more thorough investigation.  Unfortunately,  I
   didn't have bandwidth to investigate this further.
   [ cc Robin Rosenberg, who added it in fe142b3a45 (Rework cvsexportcommit to
   handle binary files for all cases., 2006-11-12) and commented out in
   e86ad71fe5 (Make cvsexportcommit work with filenames with spaces and
   non-ascii characters., 2006-12-11) ]

I found these places in tests with:

    git grep -P '^\s*[#]\s*test_' -- 't/t[0-9]*'

The rest of the results of this grep are documentation comments.

Andrei Rybak (3):
  t6003: uncomment test '--max-age=c3, --topo-order'
  t6422: drop commented out code
  t7527: uncomment test_when_finished step in a test

 t/t6003-rev-list-topo-order.sh       | 23 ++++++++++-------------
 t/t6422-merge-rename-corner-cases.sh |  2 --
 t/t7527-builtin-fsmonitor.sh         |  2 +-
 3 files changed, 11 insertions(+), 16 deletions(-)

-- 
2.39.0

[PATCH v1 1/3] t6003: uncomment test '--max-age=c3, --topo-order'

[Andrei Rybak]

Test '--max-age=c3, --topo-order' in t6003-rev-list-topo-order.sh has
been commented out as failing since its introduction in [1].  However,
the test is successful at least since commit [2] -- bisecting further is
harder because of incompatibility of such old Git code with modern
header file <openssl/bn.h> [3].

Uncomment this test to gain test coverage.

[1] f573571a21 ([PATCH] Add t/t6003 with some --topo-order tests,
    2005-07-07)
[2] 765ac8ec46 (Rip out merge-order and make "git log <paths>..." work
    again., 2006-02-28)
[3] BIGNUM used in git's `epoch.c` which was removed in [2] changed
    significantly between OpenSSL 1.0.2 and OpenSSL 1.1.0
    See also https://stackoverflow.com/a/42295243/1083697 and
    https://lore.kernel.org/git/Y71qiCs+oAS2OegH@coredump.intra.peff.net/

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t6003-rev-list-topo-order.sh | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/t/t6003-rev-list-topo-order.sh b/t/t6003-rev-list-topo-order.sh
index 1f7d7dd20c..5cf2cee74d 100755
--- a/t/t6003-rev-list-topo-order.sh
+++ b/t/t6003-rev-list-topo-order.sh
@@ -326,19 +326,16 @@ a2
 c3
 EOF
 
-#
-# this test fails on --topo-order - a fix is required
-#
-#test_output_expect_success '--max-age=c3, --topo-order' "git rev-list --topo-order --max-age=$(commit_date c3) l5" <<EOF
-#l5
-#l4
-#l3
-#a4
-#c3
-#b4
-#a3
-#a2
-#EOF
+test_output_expect_success '--max-age=c3, --topo-order' "git rev-list --topo-order --max-age=$(commit_date c3) l5" <<EOF
+l5
+l4
+l3
+a4
+c3
+b4
+a3
+a2
+EOF
 
 test_output_expect_success 'one specified head reachable from another a4, c3, --topo-order' "list_duplicates git rev-list --topo-order a4 c3" <<EOF
 EOF
-- 
2.39.0

[PATCH v1 2/3] t6422: drop commented out code

[Andrei Rybak]

In commit [1] tests in t6422-merge-rename-corner-cases.sh were
refactored to not run setup steps separately.  This included replacing
all tests like

	test_expect_success "setup ..." '
		<code of setup>
	'

with corresponding Shell functions

	test_setup_... () {
		<code of setup>
	}

During this replacement first and last lines of one of such tests got
left commented out in code.  Drop these lines to avoid confusion.

[1] da1e295e00 (t604[236]: do not run setup in separate tests, 2019-10-22)

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t6422-merge-rename-corner-cases.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh
index 346253c7c8..076b6a74d5 100755
--- a/t/t6422-merge-rename-corner-cases.sh
+++ b/t/t6422-merge-rename-corner-cases.sh
@@ -1159,7 +1159,6 @@ test_conflicts_with_adds_and_renames() {
 	#   4) There should not be any three~* files in the working
 	#      tree
 	test_setup_collision_conflict () {
-	#test_expect_success "setup simple $sideL/$sideR conflict" '
 		git init simple_${sideL}_${sideR} &&
 		(
 			cd simple_${sideL}_${sideR} &&
@@ -1236,7 +1235,6 @@ test_conflicts_with_adds_and_renames() {
 			fi &&
 			test_tick && git commit -m R
 		)
-	#'
 	}
 
 	test_expect_success "check simple $sideL/$sideR conflict" '
-- 
2.39.0

[PATCH v1 3/3] t7527: use test_when_finished in 'case insensitive+preserving'

[Andrei Rybak]

Most tests in t7527-builtin-fsmonitor.sh that start a daemon, use the
helper function test_when_finished with stop_daemon_delete_repo.
Function stop_daemon_delete_repo explicitly stops the daemon.  Calling
it via test_when_finished is needed for tests that don't check daemon's
automatic shutdown logic [1] and it is needed to avoid daemons being
left running in case of breakage of the logic of automatic shutdown of
the daemon.

Unlike these tests, test 'case insensitive+preserving' added in [2] has
a call to function test_when_finished commented out.  It was commented
out in all versions of the patch [2] during development [3].  This seems
to not be intentional, because neither commit message in [2], nor the
comment above the test mention this line being commented out.  Compare
it, for example, to "# unicode_debug=true" which is explicitly described
by a documentation comment above it.

Uncomment test_when_finished for stop_daemon_delete_repo in test 'case
insensitive+preserving' to ensure that daemons are not left running in
cases when automatic shutdown logic of daemon itself is broken.

[1] See documentation in "fsmonitor--daemon.h" for details.
[2] caa9c37ec0 (t7527: test FSMonitor on case insensitive+preserving
    file system, 2022-05-26)
[3] See mailing list thread
    https://lore.kernel.org/git/41f8cbc2ae45cb86e299eb230ad3cb0319256c37.1653601644.git.gitgitgadget@gmail.com/T/#t

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t7527-builtin-fsmonitor.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 76d0220daa..2c271b4d7e 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -910,7 +910,7 @@ test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
 # the file/directory.
 #
 test_expect_success CASE_INSENSITIVE_FS 'case insensitive+preserving' '
-#	test_when_finished "stop_daemon_delete_repo test_insensitive" &&
+	test_when_finished "stop_daemon_delete_repo test_insensitive" &&
 
 	git init test_insensitive &&
 
-- 
2.39.0

Re: [PATCH v1 0/3] fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

[Tim Schumacher]

On 12.01.23 00:32, Andrei Rybak wrote:
> [...]
>
> Here's a patch series that fixes some of the commented out test code.
>
> I skipped changing the following:
>
> 1. a minute-long test_expect_failure is commented out in t0014-alias.sh .
>     Technically, this could be uncommented and marked with `EXPENSIVE`
>     prerequisite, but it doesn't seem worth it for a `test_expect_failure`.
>     [ cc Tim Schumacher, who added this test in fef5f7fc43 (t0014: introduce an
>     alias testing suite, 2018-09-16) ]

The reason why this particular test is commented out (and why it
mentions a run time of one minute) is because support for detecting
external alias loops isn't yet implemented. This means that running that
test would spin the test runner until the test times out due to an
intentional infinite loop.

As soon as that is implemented properly, git would ideally detect the
loop after a few iterations at latest, so the test wouldn't require to
be marked as 'EXPENSIVE' in the first place.

For the context of your patches, skipping adjusting this test is most
likely fine, as it references currently unimplemented behavior and it
presumably would require more adjustments anyways before finally being
enabled.

>
> [...]
>

Tim

Re: [PATCH v1 0/3] fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

[Elijah Newren]

On Wed, Jan 11, 2023 at 4:05 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:
>
> [ I apologize for some people getting this twice -- I messed up when
>   invoking `git send-email` ]
>
> On 2023-01-06T19:25, Eric Sunshine wrote:
> > Not related to your patch at all, but I notice in this test that the
> > call to test_when_finished() is commented out:
> >
> >     # test_when_finished "stop_daemon_delete_repo test_insensitive" &&
> >
> > which makes me wonder if it was commented out while the test was being
> > debugged but then forgotten, and that the script is now potentially
> > leaking a running daemon if something in the test fails after the
> > daemon was started, or if the daemon does not shut down on its own as
> > it's supposed to do. [cc:+Jeff Hostetler]
>
> Here's a patch series that fixes some of the commented out test code.

Patch 2 is obviously correct.  Patches 1 & 3 make sense to me, but it
would be nice to have someone familiar with fsmonitor look at #3.

As to your notes about other related testcases...

> I skipped changing the following:
[...]
> 2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify
>    b & add c VS rename b->c' is commented out with an explicit "# FIXME:
>    rename/add conflicts are horribly broken right now;" above the commented out
>    part.
>    [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether
>    updates can be skipped in a merge, 2018-04-19) ]
[...]

You missed the cc...but I looked up this email since you did cc me for
patch 2/3.

Yeah, the commented out code was never tested, because the only thing
I could have tested at the time was incorrect results.  So I just took
a guess at what the improved testing would look like and apparently
made 3 small errors in doing so.  I have fixed it locally and can
submit a patch.

Re: [PATCH v1 0/3] fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

[Elijah Newren]

On Wed, Jan 11, 2023 at 5:54 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Wed, Jan 11, 2023 at 4:05 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:
> >
> > [ I apologize for some people getting this twice -- I messed up when
> >   invoking `git send-email` ]
> >
> > On 2023-01-06T19:25, Eric Sunshine wrote:
> > > Not related to your patch at all, but I notice in this test that the
> > > call to test_when_finished() is commented out:
> > >
> > >     # test_when_finished "stop_daemon_delete_repo test_insensitive" &&
> > >
> > > which makes me wonder if it was commented out while the test was being
> > > debugged but then forgotten, and that the script is now potentially
> > > leaking a running daemon if something in the test fails after the
> > > daemon was started, or if the daemon does not shut down on its own as
> > > it's supposed to do. [cc:+Jeff Hostetler]
> >
> > Here's a patch series that fixes some of the commented out test code.
>
> Patch 2 is obviously correct.  Patches 1 & 3 make sense to me, but it
> would be nice to have someone familiar with fsmonitor look at #3.
>
> As to your notes about other related testcases...
>
> > I skipped changing the following:
> [...]
> > 2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify
> >    b & add c VS rename b->c' is commented out with an explicit "# FIXME:
> >    rename/add conflicts are horribly broken right now;" above the commented out
> >    part.
> >    [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether
> >    updates can be skipped in a merge, 2018-04-19) ]
> [...]
>
> You missed the cc...but I looked up this email since you did cc me for
> patch 2/3.
>
> Yeah, the commented out code was never tested, because the only thing
> I could have tested at the time was incorrect results.  So I just took
> a guess at what the improved testing would look like and apparently
> made 3 small errors in doing so.  I have fixed it locally and can
> submit a patch.

Submitted here:
https://lore.kernel.org/git/pull.1462.git.1673584084761.gitgitgadget@gmail.com/

Re: [PATCH v1 3/3] t7527: use test_when_finished in 'case insensitive+preserving'

[Eric Sunshine]

On Wed, Jan 11, 2023 at 7:00 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:
> Most tests in t7527-builtin-fsmonitor.sh that start a daemon, use the
> helper function test_when_finished with stop_daemon_delete_repo.
> Function stop_daemon_delete_repo explicitly stops the daemon.  Calling
> it via test_when_finished is needed for tests that don't check daemon's
> automatic shutdown logic [1] and it is needed to avoid daemons being
> left running in case of breakage of the logic of automatic shutdown of
> the daemon.
>
> Unlike these tests, test 'case insensitive+preserving' added in [2] has
> a call to function test_when_finished commented out.  It was commented
> out in all versions of the patch [2] during development [3].  This seems
> to not be intentional, because neither commit message in [2], nor the
> comment above the test mention this line being commented out.  Compare
> it, for example, to "# unicode_debug=true" which is explicitly described
> by a documentation comment above it.
>
> Uncomment test_when_finished for stop_daemon_delete_repo in test 'case
> insensitive+preserving' to ensure that daemons are not left running in
> cases when automatic shutdown logic of daemon itself is broken.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> @@ -910,7 +910,7 @@ test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
>  test_expect_success CASE_INSENSITIVE_FS 'case insensitive+preserving' '
> -#      test_when_finished "stop_daemon_delete_repo test_insensitive" &&
> +       test_when_finished "stop_daemon_delete_repo test_insensitive" &&

Nice. Thanks for working on this (and the series as a whole) in
response to a very tangential comment of mine[1] when replying to an
unrelated patch of yours.

[1]: https://lore.kernel.org/git/CAPig+cTgUPWxMox_nSka52dML6_GHUUoY4HCtcq7+7J0oEyeNw@mail.gmail.com/