{"thread":{"id":"62202","subject":"[PATCH] fsmonitor OSX: fix hangs for submodules","startedAt":"2024-09-29T02:41:34Z","lastAt":"2024-10-05T03:13:10Z","messageCount":19,"participants":["Koji Nakamaru via GitGitGadget","Junio C Hamano","Koji Nakamaru","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"503633","messageId":"pull.1802.git.1727577690390.gitgitgadget@gmail.com","threadId":"62202","inReplyTo":null,"subject":"[PATCH] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-29T02:41:30Z","receivedAt":"2024-09-29T02:41:34Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru <koji.nakamaru@gree.net>\n\nfsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf\nhas no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets\nthe value with trailing \"/.\" (as repo_get_git_dir(the_repository) on\nDarwin returns \".\") so that fsmonitor_classify_path_absolute() returns\nIS_OUTSIDE_CONE.\n\nIn this case, fsevent_callback() doesn't update cookie_list so that\nfsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is\nnot invoked.\n\nAs with_lock__wait_for_cookie() infinitely waits for state->cookies_cond\nthat with_lock__mark_cookies_seen() should unlock, the whole daemon\nhangs.\n\nRemove trailing \"/.\" from state->path_gitdir_watch.buf for submodules\nand add a corresponding test in t7527-builtin-fsmonitor.sh.\n\nHelped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Koji Nakamaru <koji.nakamaru@gree.net>\n---\n    fsmonitor/darwin: fix hangs for submodules\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1802%2FKojiNakamaru%2Ffix%2Ffsmonitor-darwin-hangs-for-submodules-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1802/KojiNakamaru/fix/fsmonitor-darwin-hangs-for-submodules-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1802\n\n builtin/fsmonitor--daemon.c  |  1 +\n t/t7527-builtin-fsmonitor.sh | 39 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 40 insertions(+)\n\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex dce8a3b2482..e1e6b96d09e 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1314,6 +1314,7 @@ static int fsmonitor_run_daemon(void)\n \t\tstrbuf_reset(&state.path_gitdir_watch);\n \t\tstrbuf_addstr(&state.path_gitdir_watch,\n \t\t\t      absolute_path(repo_get_git_dir(the_repository)));\n+\t\tstrbuf_strip_suffix(&state.path_gitdir_watch, \"/.\");\n \t\tstate.nr_paths_watching = 2;\n \t}\n \ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex 730f3c7f810..7acd074a97f 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -82,6 +82,28 @@ have_t2_data_event () {\n \tgrep -e '\"event\":\"data\".*\"category\":\"'\"$c\"'\".*\"key\":\"'\"$k\"'\"'\n }\n \n+start_git_in_background () {\n+\tgit \"$@\" &\n+\tgit_pid=$!\n+\tnr_tries_left=10\n+\twhile true\n+\tdo\n+\t\tif test $nr_tries_left -eq 0\n+\t\tthen\n+\t\t\tkill $git_pid\n+\t\t\texit 1\n+\t\tfi\n+\t\tsleep 1\n+\t\tnr_tries_left=$(($nr_tries_left - 1))\n+\tdone > /dev/null 2>&1 &\n+\twatchdog_pid=$!\n+\twait $git_pid\n+}\n+\n+stop_git_and_watchdog () {\n+\tkill $git_pid $watchdog_pid\n+}\n+\n test_expect_success 'explicit daemon start and stop' '\n \ttest_when_finished \"stop_daemon_delete_repo test_explicit\" &&\n \n@@ -907,6 +929,23 @@ test_expect_success \"submodule absorbgitdirs implicitly starts daemon\" '\n \ttest_subcommand git fsmonitor--daemon start <super-sub.trace\n '\n \n+test_expect_success \"submodule implicitly starts daemon by pull\" '\n+\ttest_atexit \"stop_git_and_watchdog\" &&\n+\ttest_when_finished \"rm -rf cloned; \\\n+\t\t\t    rm -rf super; \\\n+\t\t\t    rm -rf sub\" &&\n+\n+\tcreate_super super &&\n+\tcreate_sub sub &&\n+\n+\tgit -C super submodule add ../sub ./dir_1/dir_2/sub &&\n+\tgit -C super commit -m \"add sub\" &&\n+\tgit clone --recurse-submodules super cloned &&\n+\n+\tgit -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&\n+\tstart_git_in_background -C cloned pull --recurse-submodules\n+'\n+\n # On a case-insensitive file system, confirm that the daemon\n # notices when the .git directory is moved/renamed/deleted\n # regardless of how it is spelled in the FS event.\n\nbase-commit: 3857aae53f3633b7de63ad640737c657387ae0c6\n-- \ngitgitgadget\n"},{"id":"503751","messageId":"xmqqcykl82fr.fsf@gitster.g","threadId":"62202","inReplyTo":"pull.1802.git.1727577690390.gitgitgadget@gmail.com","subject":"Re: [PATCH] fsmonitor OSX: fix hangs for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-30T17:40:56Z","receivedAt":"2024-09-30T17:40:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Koji Nakamaru via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Koji Nakamaru <koji.nakamaru@gree.net>\n>\n> fsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf\n> has no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets\n> the value with trailing \"/.\" (as repo_get_git_dir(the_repository) on\n> Darwin returns \".\") so that fsmonitor_classify_path_absolute() returns\n> IS_OUTSIDE_CONE.\n>\n> In this case, fsevent_callback() doesn't update cookie_list so that\n> fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is\n> not invoked.\n>\n> As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond\n> that with_lock__mark_cookies_seen() should unlock, the whole daemon\n> hangs.\n\nThe above very nicely describes the cause, the mechansim that leads\nto the end-user observable effect, and the (bad) effect the bug has.\n\nI wish everybody wrote their proposed commit messages like this ;-)\n\n> Remove trailing \"/.\" from state->path_gitdir_watch.buf for submodules\n> and add a corresponding test in t7527-builtin-fsmonitor.sh.\n>\n> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>\n> ---\n>     fsmonitor/darwin: fix hangs for submodules\n\n> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\n> index 730f3c7f810..7acd074a97f 100755\n> --- a/t/t7527-builtin-fsmonitor.sh\n> +++ b/t/t7527-builtin-fsmonitor.sh\n> @@ -82,6 +82,28 @@ have_t2_data_event () {\n>  \tgrep -e '\"event\":\"data\".*\"category\":\"'\"$c\"'\".*\"key\":\"'\"$k\"'\"'\n>  }\n>  \n> +start_git_in_background () {\n> +\tgit \"$@\" &\n> +\tgit_pid=$!\n> +\tnr_tries_left=10\n> +\twhile true\n> +\tdo\n> +\t\tif test $nr_tries_left -eq 0\n> +\t\tthen\n> +\t\t\tkill $git_pid\n> +\t\t\texit 1\n> +\t\tfi\n> +\t\tsleep 1\n> +\t\tnr_tries_left=$(($nr_tries_left - 1))\n> +\tdone > /dev/null 2>&1 &\n\nSo, the command is allowed to run for 10 seconds and then a signal\nis sent to the process (by the way, we do not write the SP between\n\">\" and \"/dev/null\").\n\n> +\twatchdog_pid=$!\n> +\twait $git_pid\n\nAnd the process to ensure the command gets killed in 10 seconds is\ncalled the \"watchdog\".  We let the command run for completion (and\nwe'd be happy if it did without watchdog needing to forcibly kill\nit).\n\nWhich means that even after the test finishes normally (e.g., the\ncommand completes without getting killed by the watchdog, because it\nis on a fast box and finishes in 0.5 second), we have leftover\nwatchdog process hanging around for 10 seconds, which might interfere\nwith the removal of the $TRASH_DIRECTORY at the end of the test.\n\nThere is a helper function to kill both (below), which probably is\nused to avoid it.  Let's keep reading.\n\n> +}\n> +\n> +stop_git_and_watchdog () {\n> +\tkill $git_pid $watchdog_pid\n> +}\n\nThis sends a signal and let the process die.  Without waiting to\nmake sure they indeed died, at which point we can safely remove the\n$TRASH_DIRECTORY on filesystems that refuse to remove a directory\nwhen a process still has it as its current working directory.\n\nShouldn't it loop, like\n\n\tfor pid in $git_pid $watchdog_pid\n\tdo\n                until kill -0 $pid\n                do\n                        kill $pid\n                done\n\tdone\n\nor something?  Or is there a mechanism already to ensure that we\nreturn after they get killed that I am failing to find?\n\n>  test_expect_success 'explicit daemon start and stop' '\n>  \ttest_when_finished \"stop_daemon_delete_repo test_explicit\" &&\n>  \n> @@ -907,6 +929,23 @@ test_expect_success \"submodule absorbgitdirs implicitly starts daemon\" '\n>  \ttest_subcommand git fsmonitor--daemon start <super-sub.trace\n>  '\n>  \n> +test_expect_success \"submodule implicitly starts daemon by pull\" '\n> +\ttest_atexit \"stop_git_and_watchdog\" &&\n\nHmph, this is _atexit and not _when_finished because...?\n\n> +\ttest_when_finished \"rm -rf cloned; \\\n> +\t\t\t    rm -rf super; \\\n> +\t\t\t    rm -rf sub\" &&\n\nMakes me wonder why it is not written like so:\n\n\ttest_when_finished \"rm -rf cloned super sub\" &&\n\nwhich is short enough to still fit on a line.  Is there something I\nam missing that these directories must be removed separately and in\nthis order?\n\n> +\tcreate_super super &&\n> +\tcreate_sub sub &&\n> +\n> +\tgit -C super submodule add ../sub ./dir_1/dir_2/sub &&\n> +\tgit -C super commit -m \"add sub\" &&\n> +\tgit clone --recurse-submodules super cloned &&\n> +\n> +\tgit -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&\n> +\tstart_git_in_background -C cloned pull --recurse-submodules\n> +'\n\nOther than that, very nicely done.\n\nThanks.\n\n>  # On a case-insensitive file system, confirm that the daemon\n>  # notices when the .git directory is moved/renamed/deleted\n>  # regardless of how it is spelled in the FS event.\n>\n> base-commit: 3857aae53f3633b7de63ad640737c657387ae0c6\n"},{"id":"503795","messageId":"CAOTNsDwe=RXy5sH7myzj6g4JZpVfHm7F1ch38jqVE+YUV=Efmg@mail.gmail.com","threadId":"62202","inReplyTo":"xmqqcykl82fr.fsf@gitster.g","subject":"Re: [PATCH] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-10-01T04:11:54Z","receivedAt":"2024-10-01T04:12:06Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"Thank you very much for carefully checking the patch and suggesting\nbetter ways. I'll later revise it and submit a new one.\n\n> > +}\n> > +\n> > +stop_git_and_watchdog () {\n> > +     kill $git_pid $watchdog_pid\n> > +}\n>\n> This sends a signal and let the process die.  Without waiting to\n> make sure they indeed died, at which point we can safely remove the\n> $TRASH_DIRECTORY on filesystems that refuse to remove a directory\n> when a process still has it as its current working directory.\n>\n> Shouldn't it loop, like\n>\n>         for pid in $git_pid $watchdog_pid\n>         do\n>                 until kill -0 $pid\n>                 do\n>                         kill $pid\n>                 done\n>         done\n>\n> or something?  Or is there a mechanism already to ensure that we\n> return after they get killed that I am failing to find?\n\nI agree that we have to wait for pids. I also realized that we should\nrun git in another process group and kill the group for killing all git\nchild processes. I'll fix the code.\n\n> >  test_expect_success 'explicit daemon start and stop' '\n> >       test_when_finished \"stop_daemon_delete_repo test_explicit\" &&\n> >\n> > @@ -907,6 +929,23 @@ test_expect_success \"submodule absorbgitdirs implicitly starts daemon\" '\n> >       test_subcommand git fsmonitor--daemon start <super-sub.trace\n> >  '\n> >\n> > +test_expect_success \"submodule implicitly starts daemon by pull\" '\n> > +     test_atexit \"stop_git_and_watchdog\" &&\n>\n> Hmph, this is _atexit and not _when_finished because...?\n\nThis is because README describes _atexit to run unconditionally to clean\nup before the test script exits, e.g. to stop (kill) a daemon. More\nappropriately, we should kill git before \"rm -rf cloned super sub\" in\n_when_finished and kill watchdog in _atexit. I'll adjust the code.\n\n> > +     test_when_finished \"rm -rf cloned; \\\n> > +                         rm -rf super; \\\n> > +                         rm -rf sub\" &&\n>\n> Makes me wonder why it is not written like so:\n>\n>         test_when_finished \"rm -rf cloned super sub\" &&\n>\n> which is short enough to still fit on a line.  Is there something I\n> am missing that these directories must be removed separately and in\n> this order?\n\nThere is no special reason, I simply followed the style used in\nt7527-builtin-fsmonitor.sh. I'll fix this part.\n\n\nKoji Nakamaru\n"},{"id":"503800","messageId":"pull.1802.v2.git.1727759371110.gitgitgadget@gmail.com","threadId":"62202","inReplyTo":"pull.1802.git.1727577690390.gitgitgadget@gmail.com","subject":"[PATCH v2] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-01T05:09:30Z","receivedAt":"2024-10-01T05:09:35Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru <koji.nakamaru@gree.net>\n\nfsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf\nhas no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets\nthe value with trailing \"/.\" (as repo_get_git_dir(the_repository) on\nDarwin returns \".\") so that fsmonitor_classify_path_absolute() returns\nIS_OUTSIDE_CONE.\n\nIn this case, fsevent_callback() doesn't update cookie_list so that\nfsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is\nnot invoked.\n\nAs with_lock__wait_for_cookie() infinitely waits for state->cookies_cond\nthat with_lock__mark_cookies_seen() should unlock, the whole daemon\nhangs.\n\nRemove trailing \"/.\" from state->path_gitdir_watch.buf for submodules\nand add a corresponding test in t7527-builtin-fsmonitor.sh.\n\nSuggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Koji Nakamaru <koji.nakamaru@gree.net>\n---\n    fsmonitor/darwin: fix hangs for submodules\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1802%2FKojiNakamaru%2Ffix%2Ffsmonitor-darwin-hangs-for-submodules-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1802/KojiNakamaru/fix/fsmonitor-darwin-hangs-for-submodules-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1802\n\nRange-diff vs v1:\n\n 1:  1889cbb193d ! 1:  decf68499f7 fsmonitor OSX: fix hangs for submodules\n     @@ Commit message\n          Remove trailing \"/.\" from state->path_gitdir_watch.buf for submodules\n          and add a corresponding test in t7527-builtin-fsmonitor.sh.\n      \n     -    Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n     +    Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n     +    Suggested-by: Junio C Hamano <gitster@pobox.com>\n          Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>\n      \n       ## builtin/fsmonitor--daemon.c ##\n     @@ builtin/fsmonitor--daemon.c: static int fsmonitor_run_daemon(void)\n       \n      \n       ## t/t7527-builtin-fsmonitor.sh ##\n     -@@ t/t7527-builtin-fsmonitor.sh: have_t2_data_event () {\n     - \tgrep -e '\"event\":\"data\".*\"category\":\"'\"$c\"'\".*\"key\":\"'\"$k\"'\"'\n     - }\n     +@@ t/t7527-builtin-fsmonitor.sh: test_expect_success \"submodule absorbgitdirs implicitly starts daemon\" '\n     + \ttest_subcommand git fsmonitor--daemon start <super-sub.trace\n     + '\n       \n      +start_git_in_background () {\n      +\tgit \"$@\" &\n      +\tgit_pid=$!\n     ++\tgit_pgid=$(ps -o pgid= -p $git_pid)\n      +\tnr_tries_left=10\n      +\twhile true\n      +\tdo\n      +\t\tif test $nr_tries_left -eq 0\n      +\t\tthen\n     -+\t\t\tkill $git_pid\n     ++\t\t\tkill -- -$git_pgid\n      +\t\t\texit 1\n      +\t\tfi\n      +\t\tsleep 1\n      +\t\tnr_tries_left=$(($nr_tries_left - 1))\n     -+\tdone > /dev/null 2>&1 &\n     ++\tdone >/dev/null 2>&1 &\n      +\twatchdog_pid=$!\n      +\twait $git_pid\n      +}\n      +\n     -+stop_git_and_watchdog () {\n     -+\tkill $git_pid $watchdog_pid\n     ++stop_git () {\n     ++\twhile kill -0 -- -$git_pgid\n     ++\tdo\n     ++\t\tkill -- -$git_pgid\n     ++\t\tsleep 1\n     ++\tdone\n     ++}\n     ++\n     ++stop_watchdog () {\n     ++\twhile kill -0 $watchdog_pid\n     ++\tdo\n     ++\t\tkill $watchdog_pid\n     ++\t\tsleep 1\n     ++\tdone\n      +}\n      +\n     - test_expect_success 'explicit daemon start and stop' '\n     - \ttest_when_finished \"stop_daemon_delete_repo test_explicit\" &&\n     - \n     -@@ t/t7527-builtin-fsmonitor.sh: test_expect_success \"submodule absorbgitdirs implicitly starts daemon\" '\n     - \ttest_subcommand git fsmonitor--daemon start <super-sub.trace\n     - '\n     - \n      +test_expect_success \"submodule implicitly starts daemon by pull\" '\n     -+\ttest_atexit \"stop_git_and_watchdog\" &&\n     -+\ttest_when_finished \"rm -rf cloned; \\\n     -+\t\t\t    rm -rf super; \\\n     -+\t\t\t    rm -rf sub\" &&\n     ++\ttest_atexit \"stop_watchdog\" &&\n     ++\ttest_when_finished \"stop_git && rm -rf cloned super sub\" &&\n      +\n      +\tcreate_super super &&\n      +\tcreate_sub sub &&\n     @@ t/t7527-builtin-fsmonitor.sh: test_expect_success \"submodule absorbgitdirs impli\n      +\tgit clone --recurse-submodules super cloned &&\n      +\n      +\tgit -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&\n     ++\tset -m &&\n      +\tstart_git_in_background -C cloned pull --recurse-submodules\n      +'\n      +\n\n\n builtin/fsmonitor--daemon.c  |  1 +\n t/t7527-builtin-fsmonitor.sh | 51 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+)\n\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex dce8a3b2482..e1e6b96d09e 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1314,6 +1314,7 @@ static int fsmonitor_run_daemon(void)\n \t\tstrbuf_reset(&state.path_gitdir_watch);\n \t\tstrbuf_addstr(&state.path_gitdir_watch,\n \t\t\t      absolute_path(repo_get_git_dir(the_repository)));\n+\t\tstrbuf_strip_suffix(&state.path_gitdir_watch, \"/.\");\n \t\tstate.nr_paths_watching = 2;\n \t}\n \ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex 730f3c7f810..2dd1ca1a7b7 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -907,6 +907,57 @@ test_expect_success \"submodule absorbgitdirs implicitly starts daemon\" '\n \ttest_subcommand git fsmonitor--daemon start <super-sub.trace\n '\n \n+start_git_in_background () {\n+\tgit \"$@\" &\n+\tgit_pid=$!\n+\tgit_pgid=$(ps -o pgid= -p $git_pid)\n+\tnr_tries_left=10\n+\twhile true\n+\tdo\n+\t\tif test $nr_tries_left -eq 0\n+\t\tthen\n+\t\t\tkill -- -$git_pgid\n+\t\t\texit 1\n+\t\tfi\n+\t\tsleep 1\n+\t\tnr_tries_left=$(($nr_tries_left - 1))\n+\tdone >/dev/null 2>&1 &\n+\twatchdog_pid=$!\n+\twait $git_pid\n+}\n+\n+stop_git () {\n+\twhile kill -0 -- -$git_pgid\n+\tdo\n+\t\tkill -- -$git_pgid\n+\t\tsleep 1\n+\tdone\n+}\n+\n+stop_watchdog () {\n+\twhile kill -0 $watchdog_pid\n+\tdo\n+\t\tkill $watchdog_pid\n+\t\tsleep 1\n+\tdone\n+}\n+\n+test_expect_success \"submodule implicitly starts daemon by pull\" '\n+\ttest_atexit \"stop_watchdog\" &&\n+\ttest_when_finished \"stop_git && rm -rf cloned super sub\" &&\n+\n+\tcreate_super super &&\n+\tcreate_sub sub &&\n+\n+\tgit -C super submodule add ../sub ./dir_1/dir_2/sub &&\n+\tgit -C super commit -m \"add sub\" &&\n+\tgit clone --recurse-submodules super cloned &&\n+\n+\tgit -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&\n+\tset -m &&\n+\tstart_git_in_background -C cloned pull --recurse-submodules\n+'\n+\n # On a case-insensitive file system, confirm that the daemon\n # notices when the .git directory is moved/renamed/deleted\n # regardless of how it is spelled in the FS event.\n\nbase-commit: 3857aae53f3633b7de63ad640737c657387ae0c6\n-- \ngitgitgadget\n"},{"id":"503836","messageId":"xmqqwmis11f7.fsf@gitster.g","threadId":"62202","inReplyTo":"pull.1802.v2.git.1727759371110.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-01T11:57:00Z","receivedAt":"2024-10-01T11:57:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Koji Nakamaru via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Koji Nakamaru <koji.nakamaru@gree.net>\n>\n> fsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf\n> has no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets\n> the value with trailing \"/.\" (as repo_get_git_dir(the_repository) on\n> Darwin returns \".\") so that fsmonitor_classify_path_absolute() returns\n> IS_OUTSIDE_CONE.\n>\n> In this case, fsevent_callback() doesn't update cookie_list so that\n> fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is\n> not invoked.\n>\n> As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond\n> that with_lock__mark_cookies_seen() should unlock, the whole daemon\n> hangs.\n>\n> Remove trailing \"/.\" from state->path_gitdir_watch.buf for submodules\n> and add a corresponding test in t7527-builtin-fsmonitor.sh.\n>\n> Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Suggested-by: Junio C Hamano <gitster@pobox.com>\n\nIn none of the changes described above, I have any input to deserve\nsuch credit, though.\n\n> +start_git_in_background () {\n> +\tgit \"$@\" &\n> +\tgit_pid=$!\n> +\tgit_pgid=$(ps -o pgid= -p $git_pid)\n> +\tnr_tries_left=10\n> +\twhile true\n> +\tdo\n> +\t\tif test $nr_tries_left -eq 0\n> +\t\tthen\n> +\t\t\tkill -- -$git_pgid\n> +\t\t\texit 1\n> +\t\tfi\n> +\t\tsleep 1\n> +\t\tnr_tries_left=$(($nr_tries_left - 1))\n> +\tdone >/dev/null 2>&1 &\n> +\twatchdog_pid=$!\n> +\twait $git_pid\n> +}\n> +\n> +stop_git () {\n> +\twhile kill -0 -- -$git_pgid\n> +\tdo\n> +\t\tkill -- -$git_pgid\n> +\t\tsleep 1\n> +\tdone\n> +}\n\nOn the \"git\" side you use process group because you expect that\n\"git\" would spawn subprocesses and you want to catch all of them,\n...\n\n> +stop_watchdog () {\n> +\twhile kill -0 $watchdog_pid\n> +\tdo\n> +\t\tkill $watchdog_pid\n> +\t\tsleep 1\n> +\tdone\n> +}\n\n... but \"watchdog\" you know is a single process, so you'd only need\na single process id, is that the idea?\n\nWhat is the motivation behind the change in this iteration to use\nprocess group?  Was it observed that leftover processes hang around\nif we killed only the $git_pid, or something?\n\n> +test_expect_success \"submodule implicitly starts daemon by pull\" '\n> +\ttest_atexit \"stop_watchdog\" &&\n> +\ttest_when_finished \"stop_git && rm -rf cloned super sub\" &&\n\nIf stop_git ever returns with non-zero status, \"rm -rf\" will be\nskipped, which I am not sure is a good idea.\n\nThe whole test_when_finished would fail in such a case, so you would\nnotice the problem right away, which is a plus, though.\n\n> +\tcreate_super super &&\n> +\tcreate_sub sub &&\n> +\n> +\tgit -C super submodule add ../sub ./dir_1/dir_2/sub &&\n> +\tgit -C super commit -m \"add sub\" &&\n> +\tgit clone --recurse-submodules super cloned &&\n> +\n> +\tgit -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&\n> +\tset -m &&\n\nI have to wonder how portable (and necessary) this is.\n\nPOSIX says it shall be supported if the implementation supports the\nUser Portability Utilities option.  It also says that it was added\nto apply only to the UPE because it applies primarily to interactive\nuse, not shell script applications.  And our test scripts are of\ncourse not interactive.\n\nThanks.\n"},{"id":"503851","messageId":"CAOTNsDyg2SB-wd+a7vrctXck46jyfqV4uME6nf4YQZEafWbxMw@mail.gmail.com","threadId":"62202","inReplyTo":"xmqqwmis11f7.fsf@gitster.g","subject":"Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-10-01T17:51:47Z","receivedAt":"2024-10-01T17:51:59Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"On Tue, Oct 1, 2024 at 8:57 PM Junio C Hamano <gitster@pobox.com> wrote:\n\n> > fsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf\n> > has no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets\n> > the value with trailing \"/.\" (as repo_get_git_dir(the_repository) on\n> > Darwin returns \".\") so that fsmonitor_classify_path_absolute() returns\n> > IS_OUTSIDE_CONE.\n> >\n> > In this case, fsevent_callback() doesn't update cookie_list so that\n> > fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is\n> > not invoked.\n> >\n> > As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond\n> > that with_lock__mark_cookies_seen() should unlock, the whole daemon\n> > hangs.\n> >\n> > Remove trailing \"/.\" from state->path_gitdir_watch.buf for submodules\n> > and add a corresponding test in t7527-builtin-fsmonitor.sh.\n> >\n> > Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > Suggested-by: Junio C Hamano <gitster@pobox.com>\n>\n> In none of the changes described above, I have any input to deserve\n> such credit, though.\n\nYour points are very helpful :)\n\n> > +start_git_in_background () {\n> > + git \"$@\" &\n> > + git_pid=$!\n> > + git_pgid=$(ps -o pgid= -p $git_pid)\n> > + nr_tries_left=10\n> > + while true\n> > + do\n> > + if test $nr_tries_left -eq 0\n> > + then\n> > + kill -- -$git_pgid\n> > + exit 1\n> > + fi\n> > + sleep 1\n> > + nr_tries_left=$(($nr_tries_left - 1))\n> > + done >/dev/null 2>&1 &\n> > + watchdog_pid=$!\n> > + wait $git_pid\n> > +}\n> > +\n> > +stop_git () {\n> > + while kill -0 -- -$git_pgid\n> > + do\n> > + kill -- -$git_pgid\n> > + sleep 1\n> > + done\n> > +}\n>\n> On the \"git\" side you use process group because you expect that\n> \"git\" would spawn subprocesses and you want to catch all of them,\n> ...\n>\n> > +stop_watchdog () {\n> > + while kill -0 $watchdog_pid\n> > + do\n> > + kill $watchdog_pid\n> > + sleep 1\n> > + done\n> > +}\n>\n> ... but \"watchdog\" you know is a single process, so you'd only need\n> a single process id, is that the idea?\n\nYes, that is the idea.\n\n> What is the motivation behind the change in this iteration to use\n> process group?  Was it observed that leftover processes hang around\n> if we killed only the $git_pid, or something?\n\nYes, if the issue occurs, three processes remains:\n\n  git fetch --update-head-ok --recurse-submodules=on\n\n  git fetch --no-prune --no-prune-tags --update-head-ok\n    --recurse-submodules --recurse-submodules-default yes\n    --submodule-prefix=dir_1/dir_2/sub/\n\n  git fsmonitor--daemon run --detach --ipc-threads=8\n\nIf there is no issue, only the fsmonitor process remains.\n\n> > +test_expect_success \"submodule implicitly starts daemon by pull\" '\n> > + test_atexit \"stop_watchdog\" &&\n> > + test_when_finished \"stop_git && rm -rf cloned super sub\" &&\n>\n> If stop_git ever returns with non-zero status, \"rm -rf\" will be\n> skipped, which I am not sure is a good idea.\n>\n> The whole test_when_finished would fail in such a case, so you would\n> notice the problem right away, which is a plus, though.\n\nt/README discusses that test_when_finished and test_atexit differ about\nthe \"--immediate\" option. As git and its subprocesses are the test\ntarget, I moved stop_git to the current place. This might be however\nconfusing when someone later reads this test. Should we simply put\nstop_git and stop_watchdong in test_atexit?\n\n> > + create_super super &&\n> > + create_sub sub &&\n> > +\n> > + git -C super submodule add ../sub ./dir_1/dir_2/sub &&\n> > + git -C super commit -m \"add sub\" &&\n> > + git clone --recurse-submodules super cloned &&\n> > +\n> > + git -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&\n> > + set -m &&\n>\n> I have to wonder how portable (and necessary) this is.\n>\n> POSIX says it shall be supported if the implementation supports the\n> User Portability Utilities option.  It also says that it was added\n> to apply only to the UPE because it applies primarily to interactive\n> use, not shell script applications.  And our test scripts are of\n> course not interactive.\n\nHow about the following modification? It still utilizes $git_pgid to\nfilter processes, but avoids \"set -m\".\n\n  diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\n  index 2dd1ca1a7b..23d9a7c953 100755\n  --- a/t/t7527-builtin-fsmonitor.sh\n  +++ b/t/t7527-builtin-fsmonitor.sh\n  @@ -916,7 +916,7 @@ start_git_in_background () {\n          do\n                  if test $nr_tries_left -eq 0\n                  then\n  -                       kill -- -$git_pgid\n  +                       kill $git_pid\n                          exit 1\n                  fi\n                  sleep 1\n  @@ -927,10 +927,13 @@ start_git_in_background () {\n   }\n\n   stop_git () {\n  -       while kill -0 -- -$git_pgid\n  +       for p in $(ps -o pgid=,pid=,comm= | grep \"^$git_pgid .*git\"\n| sed 's/^[0-9][0-9]* \\([0-9][0-9]*\\) .*/\\1/')\n          do\n  -               kill -- -$git_pgid\n  -               sleep 1\n  +               while kill -0 $p\n  +               do\n  +                       kill $p\n  +                       sleep 1\n  +               done\n          done\n   }\n\n  @@ -954,7 +957,6 @@ test_expect_success \"submodule implicitly starts\ndaemon by pull\" '\n          git clone --recurse-submodules super cloned &&\n\n          git -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&\n  -       set -m &&\n          start_git_in_background -C cloned pull --recurse-submodules\n   '\n\n\nKoji Nakamaru\n"},{"id":"503854","messageId":"xmqqmsjnya1c.fsf@gitster.g","threadId":"62202","inReplyTo":"CAOTNsDyg2SB-wd+a7vrctXck46jyfqV4uME6nf4YQZEafWbxMw@mail.gmail.com","subject":"Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-01T18:04:31Z","receivedAt":"2024-10-01T18:04:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Koji Nakamaru <koji.nakamaru@gree.net> writes:\n\n>> > +test_expect_success \"submodule implicitly starts daemon by pull\" '\n>> > + test_atexit \"stop_watchdog\" &&\n>> > + test_when_finished \"stop_git && rm -rf cloned super sub\" &&\n>>\n>> If stop_git ever returns with non-zero status, \"rm -rf\" will be\n>> skipped, which I am not sure is a good idea.\n>>\n>> The whole test_when_finished would fail in such a case, so you would\n>> notice the problem right away, which is a plus, though.\n>\n> t/README discusses that test_when_finished and test_atexit differ about\n> the \"--immediate\" option. As git and its subprocesses are the test\n> target, I moved stop_git to the current place. This might be however\n> confusing when someone later reads this test. Should we simply put\n> stop_git and stop_watchdong in test_atexit?\n\nThat is not what I meant.\n\nI was merely questioning the &&-chaining that stops \"rm -fr\" from\nrunning if stop_git ever fails (and your earlier iteration you had\nmultiple \"rm -fr\" ;-chained, not &&-chained---not using && is often\nmore appropriate in a when_finished handler).\n\n>> > + set -m &&\n>>\n>> I have to wonder how portable (and necessary) this is.\n>>\n>> POSIX says it shall be supported if the implementation supports the\n>> User Portability Utilities option.  It also says that it was added\n>> to apply only to the UPE because it applies primarily to interactive\n>> use, not shell script applications.  And our test scripts are of\n>> course not interactive.\n>\n> How about the following modification? It still utilizes $git_pgid to\n> filter processes, but avoids \"set -m\".\n\nNah, your original reads much better, and the code is grabbing and\nusing the process group information anyway (and my question about\n\"-m\" was more about \"should we be relying on process group features\nin this test to kill them all?\").\n\nI am OK with the idea that we can assume, at least among the\nplatforms that support fsmonitor, that sending a signal to a process\ngroup would cause the signal delivered to the member processes just\nas we expect.\n\nThanks.\n"},{"id":"503857","messageId":"CAOTNsDz=gV1TQ=N+8V+CdhWkPSogNM-42B+vzhDNWdM-Wz9CfQ@mail.gmail.com","threadId":"62202","inReplyTo":"xmqqmsjnya1c.fsf@gitster.g","subject":"Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-10-01T18:42:07Z","receivedAt":"2024-10-01T18:42:19Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"On Wed, Oct 2, 2024 at 3:04 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>> > +test_expect_success \"submodule implicitly starts daemon by pull\" '\n>>> > + test_atexit \"stop_watchdog\" &&\n>>> > + test_when_finished \"stop_git && rm -rf cloned super sub\" &&\n>>>\n>>> If stop_git ever returns with non-zero status, \"rm -rf\" will be\n>>> skipped, which I am not sure is a good idea.\n>>>\n>>> The whole test_when_finished would fail in such a case, so you would\n>>> notice the problem right away, which is a plus, though.\n>>\n>> t/README discusses that test_when_finished and test_atexit differ about\n>> the \"--immediate\" option. As git and its subprocesses are the test\n>> target, I moved stop_git to the current place. This might be however\n>> confusing when someone later reads this test. Should we simply put\n>> stop_git and stop_watchdong in test_atexit?\n>\n> That is not what I meant.\n>\n> I was merely questioning the &&-chaining that stops \"rm -fr\" from\n> running if stop_git ever fails (and your earlier iteration you had\n> multiple \"rm -fr\" ;-chained, not &&-chained---not using && is often\n> more appropriate in a when_finished handler).\n\nI see. I'll fix this part.\n\n>>> > + set -m &&\n>>>\n>>> I have to wonder how portable (and necessary) this is.\n>>>\n>>> POSIX says it shall be supported if the implementation supports the\n>>> User Portability Utilities option.  It also says that it was added\n>>> to apply only to the UPE because it applies primarily to interactive\n>>> use, not shell script applications.  And our test scripts are of\n>>> course not interactive.\n>>\n>> How about the following modification? It still utilizes $git_pgid to\n>> filter processes, but avoids \"set -m\".\n>\n> Nah, your original reads much better, and the code is grabbing and\n> using the process group information anyway (and my question about\n> \"-m\" was more about \"should we be relying on process group features\n> in this test to kill them all?\").\n>\n> I am OK with the idea that we can assume, at least among the\n> platforms that support fsmonitor, that sending a signal to a process\n> group would cause the signal delivered to the member processes just\n> as we expect.\n\nThank you for the clarification and the support.\n\nKoji Nakamaru\n"},{"id":"503866","messageId":"pull.1802.v3.git.1727810964571.gitgitgadget@gmail.com","threadId":"62202","inReplyTo":"pull.1802.v2.git.1727759371110.gitgitgadget@gmail.com","subject":"[PATCH v3] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-01T19:29:24Z","receivedAt":"2024-10-01T19:29:27Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru <koji.nakamaru@gree.net>\n\nfsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf\nhas no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets\nthe value with trailing \"/.\" (as repo_get_git_dir(the_repository) on\nDarwin returns \".\") so that fsmonitor_classify_path_absolute() returns\nIS_OUTSIDE_CONE.\n\nIn this case, fsevent_callback() doesn't update cookie_list so that\nfsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is\nnot invoked.\n\nAs with_lock__wait_for_cookie() infinitely waits for state->cookies_cond\nthat with_lock__mark_cookies_seen() should unlock, the whole daemon\nhangs.\n\nRemove trailing \"/.\" from state->path_gitdir_watch.buf for submodules\nand add a corresponding test in t7527-builtin-fsmonitor.sh.\n\nSuggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Koji Nakamaru <koji.nakamaru@gree.net>\n---\n    fsmonitor/darwin: fix hangs for submodules\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1802%2FKojiNakamaru%2Ffix%2Ffsmonitor-darwin-hangs-for-submodules-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1802/KojiNakamaru/fix/fsmonitor-darwin-hangs-for-submodules-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1802\n\nRange-diff vs v2:\n\n 1:  decf68499f7 ! 1:  aabc1c2f6ee fsmonitor OSX: fix hangs for submodules\n     @@ t/t7527-builtin-fsmonitor.sh: test_expect_success \"submodule absorbgitdirs impli\n      +\n      +test_expect_success \"submodule implicitly starts daemon by pull\" '\n      +\ttest_atexit \"stop_watchdog\" &&\n     -+\ttest_when_finished \"stop_git && rm -rf cloned super sub\" &&\n     ++\ttest_when_finished \"stop_git; rm -rf cloned super sub\" &&\n      +\n      +\tcreate_super super &&\n      +\tcreate_sub sub &&\n\n\n builtin/fsmonitor--daemon.c  |  1 +\n t/t7527-builtin-fsmonitor.sh | 51 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+)\n\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex dce8a3b2482..e1e6b96d09e 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1314,6 +1314,7 @@ static int fsmonitor_run_daemon(void)\n \t\tstrbuf_reset(&state.path_gitdir_watch);\n \t\tstrbuf_addstr(&state.path_gitdir_watch,\n \t\t\t      absolute_path(repo_get_git_dir(the_repository)));\n+\t\tstrbuf_strip_suffix(&state.path_gitdir_watch, \"/.\");\n \t\tstate.nr_paths_watching = 2;\n \t}\n \ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex 730f3c7f810..e6ddc7048c0 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -907,6 +907,57 @@ test_expect_success \"submodule absorbgitdirs implicitly starts daemon\" '\n \ttest_subcommand git fsmonitor--daemon start <super-sub.trace\n '\n \n+start_git_in_background () {\n+\tgit \"$@\" &\n+\tgit_pid=$!\n+\tgit_pgid=$(ps -o pgid= -p $git_pid)\n+\tnr_tries_left=10\n+\twhile true\n+\tdo\n+\t\tif test $nr_tries_left -eq 0\n+\t\tthen\n+\t\t\tkill -- -$git_pgid\n+\t\t\texit 1\n+\t\tfi\n+\t\tsleep 1\n+\t\tnr_tries_left=$(($nr_tries_left - 1))\n+\tdone >/dev/null 2>&1 &\n+\twatchdog_pid=$!\n+\twait $git_pid\n+}\n+\n+stop_git () {\n+\twhile kill -0 -- -$git_pgid\n+\tdo\n+\t\tkill -- -$git_pgid\n+\t\tsleep 1\n+\tdone\n+}\n+\n+stop_watchdog () {\n+\twhile kill -0 $watchdog_pid\n+\tdo\n+\t\tkill $watchdog_pid\n+\t\tsleep 1\n+\tdone\n+}\n+\n+test_expect_success \"submodule implicitly starts daemon by pull\" '\n+\ttest_atexit \"stop_watchdog\" &&\n+\ttest_when_finished \"stop_git; rm -rf cloned super sub\" &&\n+\n+\tcreate_super super &&\n+\tcreate_sub sub &&\n+\n+\tgit -C super submodule add ../sub ./dir_1/dir_2/sub &&\n+\tgit -C super commit -m \"add sub\" &&\n+\tgit clone --recurse-submodules super cloned &&\n+\n+\tgit -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&\n+\tset -m &&\n+\tstart_git_in_background -C cloned pull --recurse-submodules\n+'\n+\n # On a case-insensitive file system, confirm that the daemon\n # notices when the .git directory is moved/renamed/deleted\n # regardless of how it is spelled in the FS event.\n\nbase-commit: 3857aae53f3633b7de63ad640737c657387ae0c6\n-- \ngitgitgadget\n"},{"id":"503890","messageId":"CAOTNsDygwBkNdFX4K_ixL5kP-AvDxWWVXkvfkmV4Kh04ozdYkA@mail.gmail.com","threadId":"62202","inReplyTo":"CAOTNsDz=gV1TQ=N+8V+CdhWkPSogNM-42B+vzhDNWdM-Wz9CfQ@mail.gmail.com","subject":"Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-10-02T06:58:29Z","receivedAt":"2024-10-02T06:58:41Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"Although I submitted [PATCH v3], it was incomplete about the following:\n\nOn Wed, Oct 2, 2024 at 3:04 AM Junio C Hamano <gitster@pobox.com> wrote:\n> I am OK with the idea that we can assume, at least among the\n> platforms that support fsmonitor, that sending a signal to a process\n> group would cause the signal delivered to the member processes just\n> as we expect.\n\nOn windows, there is no process group so the test cannot run\ncorrectly. As hangs corrected with the patch occur only for darwin, I\nwould like to skip MINGW in the test. Is it okay?\n\nKoji Nakamaru\n"},{"id":"503969","messageId":"xmqqploimkx1.fsf@gitster.g","threadId":"62202","inReplyTo":"CAOTNsDygwBkNdFX4K_ixL5kP-AvDxWWVXkvfkmV4Kh04ozdYkA@mail.gmail.com","subject":"Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-02T18:14:50Z","receivedAt":"2024-10-02T18:14:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Koji Nakamaru <koji.nakamaru@gree.net> writes:\n\n> On windows, there is no process group so the test cannot run\n> correctly. As hangs corrected with the patch occur only for darwin, I\n> would like to skip MINGW in the test. Is it okay?\n\nSurely.  But can we do so without spelling MINGW or WINDOWS out?\n\nThat is, if your test requires process group features available, can\nwe come up with a lazy prerequisite and use that to decide if we\nskip the test?\n\nEarlier in the discussion, you said who are left behind if we did so\non systems with process groups, but I wonder what happens when we\nthrow a signal at the top-level \"git\" process on Windows, and if it\nbehaves better, perhaps we can implement stop_git differently where\nprocess groups are not available, instead of skipping the tests\naltogether?\n\nThanks.\n\n"},{"id":"504018","messageId":"CAOTNsDwSTO_iXqG7-ez0O0Y-xbDbEBa6-EiAL-DL=PR1nbM6Zw@mail.gmail.com","threadId":"62202","inReplyTo":"xmqqploimkx1.fsf@gitster.g","subject":"Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-10-03T08:54:52Z","receivedAt":"2024-10-03T08:55:04Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"On Thu, Oct 3, 2024 at 3:14 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> On windows, there is no process group so the test cannot run\n>> correctly. As hangs corrected with the patch occur only for darwin, I\n>> would like to skip MINGW in the test. Is it okay?\n>\n> Surely.  But can we do so without spelling MINGW or WINDOWS out?\n>\n> That is, if your test requires process group features available, can\n> we come up with a lazy prerequisite and use that to decide if we\n> skip the test?\n\nI tweaked fsm-listen-win32.c to cause hangs and tested on windows. I'm\nsorry that simply saying \"there is no process group\" was not quite\ncorrect. Each mingw process has a process group and its win32\nsubprocesses can be killed by \"kill -- -$pgid\"\n\nFor example, when a hang occurs, the following processes remain.\n\n      PID    PPID    PGID     WINPID   TTY         UID    STIME\n        COMMAND\n    56782   40923   56782       9484  pty0     1052296 16:23:22\n        /mingw64/bin/git\n        # mingw git process\n    78100       0       0      12564  ?              0 16:23:23\n        C:\\git-sdk-64\\mingw64\\libexec\\git-core\\git.exe\n        # win32 process\n    86108       0       0      20572  ?              0 16:23:23\n        C:\\git-sdk-64\\mingw64\\libexec\\git-core\\git.exe\n        # win32 subprocess\n    73328       0       0       7792  ?              0 16:23:23\n        C:\\git-sdk-64\\mingw64\\libexec\\git-core\\git.exe\n        # win32 fsmonitor\n\n> Earlier in the discussion, you said who are left behind if we did so\n> on systems with process groups, but I wonder what happens when we\n> throw a signal at the top-level \"git\" process on Windows, and if it\n> behaves better, perhaps we can implement stop_git differently where\n> process groups are not available, instead of skipping the tests\n> altogether?\n\nIf we do \"kill 56782\" or \"kill -- -56782\" for the above example, most of\nprocesses are terminated except the win32 fsmonitor. This is because the\nwin32 fsmonitor is detached by FreeConsole().\n\nI also tried \"git fsmonitor--daemon stop\". It was able to communicate\nwith the win32 fsmonitor and the internal status of the win32 fsmonitor\nchanged, but the win32 daemon didn't terminate.\n\nBecause it's getting complicated, how about the following:\n\n* specify MINGW\n* note in the commit log:\n  The test is disabled for MINGW because hangs treated with this patch\n  occur only for Darwin and there is no simple way to terminate the\n  win32 fsmonitor daemon that hangs.\n\nKoji Nakamaru\n"},{"id":"504034","messageId":"xmqqcykhhyj3.fsf@gitster.g","threadId":"62202","inReplyTo":"CAOTNsDwSTO_iXqG7-ez0O0Y-xbDbEBa6-EiAL-DL=PR1nbM6Zw@mail.gmail.com","subject":"Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-03T17:44:16Z","receivedAt":"2024-10-03T17:44:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Koji Nakamaru <koji.nakamaru@gree.net> writes:\n\n> Because it's getting complicated, how about the following:\n>\n> * specify MINGW\n> * note in the commit log:\n>   The test is disabled for MINGW because hangs treated with this patch\n>   occur only for Darwin and there is no simple way to terminate the\n>   win32 fsmonitor daemon that hangs.\n\nSounds good to me.\n\nThanks.\n"},{"id":"504063","messageId":"pull.1802.v4.git.1728000433326.gitgitgadget@gmail.com","threadId":"62202","inReplyTo":"pull.1802.v3.git.1727810964571.gitgitgadget@gmail.com","subject":"[PATCH v4] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-04T00:07:13Z","receivedAt":"2024-10-04T00:07:17Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru <koji.nakamaru@gree.net>\n\nfsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf\nhas no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets\nthe value with trailing \"/.\" (as repo_get_git_dir(the_repository) on\nDarwin returns \".\") so that fsmonitor_classify_path_absolute() returns\nIS_OUTSIDE_CONE.\n\nIn this case, fsevent_callback() doesn't update cookie_list so that\nfsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is\nnot invoked.\n\nAs with_lock__wait_for_cookie() infinitely waits for state->cookies_cond\nthat with_lock__mark_cookies_seen() should unlock, the whole daemon\nhangs.\n\nRemove trailing \"/.\" from state->path_gitdir_watch.buf for submodules\nand add a corresponding test in t7527-builtin-fsmonitor.sh. The test is\ndisabled for MINGW because hangs treated with this patch occur only for\nDarwin and there is no simple way to terminate the win32 fsmonitor\ndaemon that hangs.\n\nSuggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Koji Nakamaru <koji.nakamaru@gree.net>\n---\n    fsmonitor/darwin: fix hangs for submodules\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1802%2FKojiNakamaru%2Ffix%2Ffsmonitor-darwin-hangs-for-submodules-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1802/KojiNakamaru/fix/fsmonitor-darwin-hangs-for-submodules-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1802\n\nRange-diff vs v3:\n\n 1:  aabc1c2f6ee ! 1:  7b7224ef718 fsmonitor OSX: fix hangs for submodules\n     @@ Commit message\n          hangs.\n      \n          Remove trailing \"/.\" from state->path_gitdir_watch.buf for submodules\n     -    and add a corresponding test in t7527-builtin-fsmonitor.sh.\n     +    and add a corresponding test in t7527-builtin-fsmonitor.sh. The test is\n     +    disabled for MINGW because hangs treated with this patch occur only for\n     +    Darwin and there is no simple way to terminate the win32 fsmonitor\n     +    daemon that hangs.\n      \n          Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n          Suggested-by: Junio C Hamano <gitster@pobox.com>\n     @@ t/t7527-builtin-fsmonitor.sh: test_expect_success \"submodule absorbgitdirs impli\n      +\tdone\n      +}\n      +\n     -+test_expect_success \"submodule implicitly starts daemon by pull\" '\n     ++test_expect_success !MINGW \"submodule implicitly starts daemon by pull\" '\n      +\ttest_atexit \"stop_watchdog\" &&\n      +\ttest_when_finished \"stop_git; rm -rf cloned super sub\" &&\n      +\n\n\n builtin/fsmonitor--daemon.c  |  1 +\n t/t7527-builtin-fsmonitor.sh | 51 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+)\n\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex dce8a3b2482..e1e6b96d09e 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1314,6 +1314,7 @@ static int fsmonitor_run_daemon(void)\n \t\tstrbuf_reset(&state.path_gitdir_watch);\n \t\tstrbuf_addstr(&state.path_gitdir_watch,\n \t\t\t      absolute_path(repo_get_git_dir(the_repository)));\n+\t\tstrbuf_strip_suffix(&state.path_gitdir_watch, \"/.\");\n \t\tstate.nr_paths_watching = 2;\n \t}\n \ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex 730f3c7f810..9b15baa02d3 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -907,6 +907,57 @@ test_expect_success \"submodule absorbgitdirs implicitly starts daemon\" '\n \ttest_subcommand git fsmonitor--daemon start <super-sub.trace\n '\n \n+start_git_in_background () {\n+\tgit \"$@\" &\n+\tgit_pid=$!\n+\tgit_pgid=$(ps -o pgid= -p $git_pid)\n+\tnr_tries_left=10\n+\twhile true\n+\tdo\n+\t\tif test $nr_tries_left -eq 0\n+\t\tthen\n+\t\t\tkill -- -$git_pgid\n+\t\t\texit 1\n+\t\tfi\n+\t\tsleep 1\n+\t\tnr_tries_left=$(($nr_tries_left - 1))\n+\tdone >/dev/null 2>&1 &\n+\twatchdog_pid=$!\n+\twait $git_pid\n+}\n+\n+stop_git () {\n+\twhile kill -0 -- -$git_pgid\n+\tdo\n+\t\tkill -- -$git_pgid\n+\t\tsleep 1\n+\tdone\n+}\n+\n+stop_watchdog () {\n+\twhile kill -0 $watchdog_pid\n+\tdo\n+\t\tkill $watchdog_pid\n+\t\tsleep 1\n+\tdone\n+}\n+\n+test_expect_success !MINGW \"submodule implicitly starts daemon by pull\" '\n+\ttest_atexit \"stop_watchdog\" &&\n+\ttest_when_finished \"stop_git; rm -rf cloned super sub\" &&\n+\n+\tcreate_super super &&\n+\tcreate_sub sub &&\n+\n+\tgit -C super submodule add ../sub ./dir_1/dir_2/sub &&\n+\tgit -C super commit -m \"add sub\" &&\n+\tgit clone --recurse-submodules super cloned &&\n+\n+\tgit -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&\n+\tset -m &&\n+\tstart_git_in_background -C cloned pull --recurse-submodules\n+'\n+\n # On a case-insensitive file system, confirm that the daemon\n # notices when the .git directory is moved/renamed/deleted\n # regardless of how it is spelled in the FS event.\n\nbase-commit: 3857aae53f3633b7de63ad640737c657387ae0c6\n-- \ngitgitgadget\n"},{"id":"504066","messageId":"CAOTNsDzOscsO2iv63J-hXzz0NHhuZNZqLiz+QBy5WAbjzr+ipQ@mail.gmail.com","threadId":"62202","inReplyTo":"xmqqcykhhyj3.fsf@gitster.g","subject":"Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-10-04T02:42:20Z","receivedAt":"2024-10-04T02:42:33Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"On Fri, Oct 4, 2024 at 2:44 AM Junio C Hamano <gitster@pobox.com> wrote:\n> > Because it's getting complicated, how about the following:\n> >\n> > * specify MINGW\n> > * note in the commit log:\n> >   The test is disabled for MINGW because hangs treated with this patch\n> >   occur only for Darwin and there is no simple way to terminate the\n> >   win32 fsmonitor daemon that hangs.\n>\n> Sounds good to me.\n\nThank you. I've submitted [PATCH v4].\n\nKoji Nakamaru\n"},{"id":"504104","messageId":"xmqqiku7epa7.fsf@gitster.g","threadId":"62202","inReplyTo":"pull.1802.v4.git.1728000433326.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] fsmonitor OSX: fix hangs for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-04T17:44:32Z","receivedAt":"2024-10-04T17:44:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Koji Nakamaru via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> ... The test is\n> disabled for MINGW because hangs treated with this patch occur only for\n> Darwin and there is no simple way to terminate the win32 fsmonitor\n> daemon that hangs.\n> ...\n>      @@ t/t7527-builtin-fsmonitor.sh: test_expect_success \"submodule absorbgitdirs impli\n>       +\tdone\n>       +}\n>       +\n>      -+test_expect_success \"submodule implicitly starts daemon by pull\" '\n>      ++test_expect_success !MINGW \"submodule implicitly starts daemon by pull\" '\n>       +\ttest_atexit \"stop_watchdog\" &&\n>       +\ttest_when_finished \"stop_git; rm -rf cloned super sub\" &&\n>       +\n\nLet me update !MINGW to !WINDOWS while queuing.\n"},{"id":"504106","messageId":"d350d598-fc99-4384-b2d5-5cc898a17c12@ramsayjones.plus.com","threadId":"62202","inReplyTo":"xmqqiku7epa7.fsf@gitster.g","subject":"Re: [PATCH v4] fsmonitor OSX: fix hangs for submodules","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-10-04T18:47:56Z","receivedAt":"2024-10-04T18:48:01Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/10/2024 18:44, Junio C Hamano wrote:\n> \"Koji Nakamaru via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> ... The test is\n>> disabled for MINGW because hangs treated with this patch occur only for\n>> Darwin and there is no simple way to terminate the win32 fsmonitor\n>> daemon that hangs.\n>> ...\n>>      @@ t/t7527-builtin-fsmonitor.sh: test_expect_success \"submodule absorbgitdirs impli\n>>       +\tdone\n>>       +}\n>>       +\n>>      -+test_expect_success \"submodule implicitly starts daemon by pull\" '\n>>      ++test_expect_success !MINGW \"submodule implicitly starts daemon by pull\" '\n>>       +\ttest_atexit \"stop_watchdog\" &&\n>>       +\ttest_when_finished \"stop_git; rm -rf cloned super sub\" &&\n>>       +\n> \n> Let me update !MINGW to !WINDOWS while queuing.\n> \n\nWhile this won't hurt, this test file is skipped on cygwin:\n\n[23:19:33] t7527-builtin-fsmonitor.sh ......................... skipped: fsmonitor--daemon is not supported on this platform\n\n(my eternal TODO list has an 'fsmonitor on cygwin?' item ...)\n\nThanks.\n\nATB,\nRamsay Jones\n\n\n\n"},{"id":"504107","messageId":"xmqqa5fjelfv.fsf@gitster.g","threadId":"62202","inReplyTo":"d350d598-fc99-4384-b2d5-5cc898a17c12@ramsayjones.plus.com","subject":"Re: [PATCH v4] fsmonitor OSX: fix hangs for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-04T19:07:32Z","receivedAt":"2024-10-04T19:07:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> While this won't hurt, this test file is skipped on cygwin:\n>\n> [23:19:33] t7527-builtin-fsmonitor.sh ......................... skipped: fsmonitor--daemon is not supported on this platform\n>\n> (my eternal TODO list has an 'fsmonitor on cygwin?' item ...)\n\nThanks, then let's not bother.\n\n\n"},{"id":"504125","messageId":"CAOTNsDzTNG8-CtRb_Uwn92cUP6+4YHsLmJt4bMcORq1vp7Wa6g@mail.gmail.com","threadId":"62202","inReplyTo":"xmqqiku7epa7.fsf@gitster.g","subject":"Re: [PATCH v4] fsmonitor OSX: fix hangs for submodules","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-10-05T03:12:58Z","receivedAt":"2024-10-05T03:13:10Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"On Sat, Oct 5, 2024 at 2:44 AM Junio C Hamano <gitster@pobox.com> wrote:\n> > ... The test is\n> > disabled for MINGW because hangs treated with this patch occur only for\n> > Darwin and there is no simple way to terminate the win32 fsmonitor\n> > daemon that hangs.\n> > ...\n> >      @@ t/t7527-builtin-fsmonitor.sh: test_expect_success \"submodule absorbgitdirs impli\n> >       +       done\n> >       +}\n> >       +\n> >      -+test_expect_success \"submodule implicitly starts daemon by pull\" '\n> >      ++test_expect_success !MINGW \"submodule implicitly starts daemon by pull\" '\n> >       +       test_atexit \"stop_watchdog\" &&\n> >       +       test_when_finished \"stop_git; rm -rf cloned super sub\" &&\n> >       +\n>\n> Let me update !MINGW to !WINDOWS while queuing.\n\nI see, Thank you.\n\nKoji Nakamaru\n"}]}