On 11/21/2019 8:06 PM, SZEDER Gábor wrote:

Thanks for this message. Sorry I'm so late getting back to it.

On Thu, Nov 21, 2019 at 10:20:26PM +0000, Derrick Stolee via GitGitGadget wrote:

quoted

From: Derrick Stolee <redacted>

Signed-off-by: Derrick Stolee <redacted>
---
 t/test-lib-functions.sh | 15 +++++++++++++++
 t/test-lib.sh           |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e0b3f28d3a..03573caf42 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -1475,3 +1475,18 @@ test_set_port () {
 	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
 	eval $var=$port
 }
+
+test_clear_watchman () {
+	if test $GIT_TEST_FSMONITOR -ne ""

In the rare cases when this function is invoked (see below) this
condition triggers an error from the shell running test script:

  - when the variable is not set, because of the lack of quotes around
    the variable name:

      $ ./t5570-git-daemon.sh 
      [....]
      ok 21 - hostname interpolation works after LF-stripping
      ./t5570-git-daemon.sh: 1482: test: -ne: unexpected operator
      # passed all 21 test(s)
      1..21

  - when the variable is set, because the '-ne' operator does integer
    comparison:

      $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh
      [...]
      ok 21 - hostname interpolation works after LF-stripping
      ./t5570-git-daemon.sh: 1482: test: Illegal number: /home/szeder/src/git/t/t7519/fsmonitor-none
      # failed 1 among 21 test(s)
      1..21

Please use 'if test -n "$GIT_TEST_FSMONITOR"' instead.

Thanks for the pointers.

quoted

+	then
+		watchman watch-list |

Then with the above fixed, trying to run 'watchman' triggers another
error if it's not installed:

  $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh 
  [...]
  ok 21 - hostname interpolation works after LF-stripping
  ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found
  # failed 1 among 21 test(s)

I think we need an additional condition to run this only if
't7519/fsmonitor-watchman' is used in the tests.

The intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR,
and that can only use watchman (currently). Barring wanting to unset the
variable if it was set on purpose in a test script, the other options do
not actually return correct values to make use of the feature.

quoted

+			grep "$TRASH_DIRECTORY" |
+			sed "s/\t\"//g" |
+			sed "s/\",//g" >repo-list
+
+		for repo in $(cat repo-list)
+		do
+			watchman watch-del "$repo"
+		done
+	fi
+}

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 30b07e310f..067a432ea5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -1072,6 +1072,8 @@ test_atexit_handler () {
 	# sure that the registered cleanup commands are run only once.
 	test : != "$test_atexit_cleanup" || return 0
 
+	test_clear_watchman

I'm not sure where to put this call, but this is definitely not the
right place for it.  See that 'return 0' above in the context?  That's
where the test_atexit_handler function returns early when no atexit
handler commands are set, i.e. in all test scripts that don't involve
some kind of daemons, thus this call is not invoked in the majority of
test scripts.

Ah, I misunderstood the point of test_atexit_handler.

Simply moving this call before that early return is not good, because
then it would be invoked twice.

An option would be to register this call as an atexit command
somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON
is restored, perhaps).  That way it would be invoked most of the time,
and it would be invoked only once, but I'm not sure how it would work
out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the
middle for the remainder of the test script.  However, register the
atexit command only if GIT_TEST_FSMONITOR is set (to something
watchman-specific), so it won't be invoked at all if
GIT_TEST_FSMONITOR is not set, and thus it won't generate additional
test output and trace.

I don't have a better idea.

Shouldn't it be sufficient to add it into test_done? If the test fails,
then we could leave watches open, but that's no worse than we had without
this test_clear_watchman method.

Thanks,
-Stolee