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

[Junio C Hamano <gitster@pobox.com>]

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

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

> +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,
...

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

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