Re: [PATCH v2] fsmonitor OSX: fix hangs for submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 1, 2024, 11:57 UTC
Message-ID: <xmqqwmis11f7.fsf@gitster.g>
In-Reply-To: <pull.1802.v2.git.1727759371110.gitgitgadget@gmail.com>

"Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 21 quoted lines

> 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>

In none of the changes described above, I have any input to deserve such credit, though.

Show 26 quoted lines

> +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
> +}

On the "git" side you use process group because you expect that "git" would spawn subprocesses and you want to catch all of them, ...

Show 7 quoted lines

> +stop_watchdog () {
> +	while kill -0 $watchdog_pid
> +	do
> +		kill $watchdog_pid
> +		sleep 1
> +	done
> +}

... but "watchdog" you know is a single process, so you'd only need a single process id, is that the idea?

What is the motivation behind the change in this iteration to use process group? Was it observed that leftover processes hang around if we killed only the $git_pid, or something?

> +test_expect_success "submodule implicitly starts daemon by pull" '
> +	test_atexit "stop_watchdog" &&
> +	test_when_finished "stop_git && rm -rf cloned super sub" &&

If stop_git ever returns with non-zero status, "rm -rf" will be skipped, which I am not sure is a good idea.

The whole test_when_finished would fail in such a case, so you would notice the problem right away, which is a plus, though.

Show 9 quoted lines

> +	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 &&

I have to wonder how portable (and necessary) this is.

POSIX says it shall be supported if the implementation supports the User Portability Utilities option. It also says that it was added to apply only to the UPE because it applies primarily to interactive use, not shell script applications. And our test scripts are of course not interactive.

Thanks.