[PATCH v2] fsmonitor OSX: fix hangs for submodules

From: Koji Nakamaru via GitGitGadget <gitgitgadget@gmail.com>
Date: Oct 1, 2024, 05:09 UTC
Message-ID: <pull.1802.v2.git.1727759371110.gitgitgadget@gmail.com>
In-Reply-To: <pull.1802.git.1727577690390.gitgitgadget@gmail.com>

From: Koji Nakamaru <koji.nakamaru@gree.net>

fsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf has no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets the value with trailing "/." (as repo_get_git_dir(the_repository) on Darwin returns ".") so that fsmonitor_classify_path_absolute() returns IS_OUTSIDE_CONE.

In this case, fsevent_callback() doesn't update cookie_list so that fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is not invoked.

As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond that with_lock__mark_cookies_seen() should unlock, the whole daemon hangs.

Remove trailing "/." from state->path_gitdir_watch.buf for submodules and add a corresponding test in t7527-builtin-fsmonitor.sh.

Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
---
    fsmonitor/darwin: fix hangs for submodules

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1802%2FKojiNakamaru%2Ffix%2Ffsmonitor-darwin-hangs-for-submodules-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1802/KojiNakamaru/fix/fsmonitor-darwin-hangs-for-submodules-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1802

Range-diff vs v1:

 1:  1889cbb193d ! 1:  decf68499f7 fsmonitor OSX: fix hangs for submodules
     @@ Commit message
          Remove trailing "/." from state->path_gitdir_watch.buf for submodules
          and add a corresponding test in t7527-builtin-fsmonitor.sh.
      
     -    Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
     +    Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
     +    Suggested-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
      
       ## builtin/fsmonitor--daemon.c ##
     @@ builtin/fsmonitor--daemon.c: static int fsmonitor_run_daemon(void)
       
      
       ## t/t7527-builtin-fsmonitor.sh ##
     -@@ t/t7527-builtin-fsmonitor.sh: have_t2_data_event () {
     - 	grep -e '"event":"data".*"category":"'"$c"'".*"key":"'"$k"'"'
     - }
     +@@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
     + 	test_subcommand git fsmonitor--daemon start <super-sub.trace
     + '
       
      +start_git_in_background () {
      +	git "$@" &
      +	git_pid=$!
     ++	git_pgid=$(ps -o pgid= -p $git_pid)
      +	nr_tries_left=10
      +	while true
      +	do
      +		if test $nr_tries_left -eq 0
      +		then
     -+			kill $git_pid
     ++			kill -- -$git_pgid
      +			exit 1
      +		fi
      +		sleep 1
      +		nr_tries_left=$(($nr_tries_left - 1))
     -+	done > /dev/null 2>&1 &
     ++	done >/dev/null 2>&1 &
      +	watchdog_pid=$!
      +	wait $git_pid
      +}
      +
     -+stop_git_and_watchdog () {
     -+	kill $git_pid $watchdog_pid
     ++stop_git () {
     ++	while kill -0 -- -$git_pgid
     ++	do
     ++		kill -- -$git_pgid
     ++		sleep 1
     ++	done
     ++}
     ++
     ++stop_watchdog () {
     ++	while kill -0 $watchdog_pid
     ++	do
     ++		kill $watchdog_pid
     ++		sleep 1
     ++	done
      +}
      +
     - test_expect_success 'explicit daemon start and stop' '
     - 	test_when_finished "stop_daemon_delete_repo test_explicit" &&
     - 
     -@@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
     - 	test_subcommand git fsmonitor--daemon start <super-sub.trace
     - '
     - 
      +test_expect_success "submodule implicitly starts daemon by pull" '
     -+	test_atexit "stop_git_and_watchdog" &&
     -+	test_when_finished "rm -rf cloned; \
     -+			    rm -rf super; \
     -+			    rm -rf sub" &&
     ++	test_atexit "stop_watchdog" &&
     ++	test_when_finished "stop_git && rm -rf cloned super sub" &&
      +
      +	create_super super &&
      +	create_sub sub &&
     @@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs impli
      +	git clone --recurse-submodules super cloned &&
      +
      +	git -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&
     ++	set -m &&
      +	start_git_in_background -C cloned pull --recurse-submodules
      +'
      +

 builtin/fsmonitor--daemon.c  |  1 +
 t/t7527-builtin-fsmonitor.sh | 51 ++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index dce8a3b2482..e1e6b96d09e 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1314,6 +1314,7 @@ static int fsmonitor_run_daemon(void)
 		strbuf_reset(&state.path_gitdir_watch);
 		strbuf_addstr(&state.path_gitdir_watch,
 			      absolute_path(repo_get_git_dir(the_repository)));
+		strbuf_strip_suffix(&state.path_gitdir_watch, "/.");
 		state.nr_paths_watching = 2;
 	}
 
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 730f3c7f810..2dd1ca1a7b7 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -907,6 +907,57 @@ test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
 	test_subcommand git fsmonitor--daemon start <super-sub.trace
 '
 
+start_git_in_background () {
+	git "$@" &
+	git_pid=$!
+	git_pgid=$(ps -o pgid= -p $git_pid)
+	nr_tries_left=10
+	while true
+	do
+		if test $nr_tries_left -eq 0
+		then
+			kill -- -$git_pgid
+			exit 1
+		fi
+		sleep 1
+		nr_tries_left=$(($nr_tries_left - 1))
+	done >/dev/null 2>&1 &
+	watchdog_pid=$!
+	wait $git_pid
+}
+
+stop_git () {
+	while kill -0 -- -$git_pgid
+	do
+		kill -- -$git_pgid
+		sleep 1
+	done
+}
+
+stop_watchdog () {
+	while kill -0 $watchdog_pid
+	do
+		kill $watchdog_pid
+		sleep 1
+	done
+}
+
+test_expect_success "submodule implicitly starts daemon by pull" '
+	test_atexit "stop_watchdog" &&
+	test_when_finished "stop_git && rm -rf cloned super sub" &&
+
+	create_super super &&
+	create_sub sub &&
+
+	git -C super submodule add ../sub ./dir_1/dir_2/sub &&
+	git -C super commit -m "add sub" &&
+	git clone --recurse-submodules super cloned &&
+
+	git -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&
+	set -m &&
+	start_git_in_background -C cloned pull --recurse-submodules
+'
+
 # On a case-insensitive file system, confirm that the daemon
 # notices when the .git directory is moved/renamed/deleted
 # regardless of how it is spelled in the FS event.

base-commit: 3857aae53f3633b7de63ad640737c657387ae0c6

-- 
gitgitgadget