Re: [PATCH v2 4/5] scalar unregister: stop FSMonitor daemon

Derrick Stolee wrote:

On 8/16/2022 7:58 PM, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Johannes Schindelin <redacted>

Especially on Windows, we will need to stop that daemon, just in case
that the directory needs to be removed (the daemon would otherwise hold
a handle to that directory, preventing it from being deleted).

quoted

+static int stop_fsmonitor_daemon(void)
+{
+	assert(fsmonitor_ipc__is_supported());
+
+	if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
+		return run_git("fsmonitor--daemon", "stop", NULL);
+
+	return 0;
+}
+
 static int register_dir(void)
 {
 	if (add_or_remove_enlistment(1))

@@ -281,6 +291,9 @@ static int unregister_dir(void)
 	if (add_or_remove_enlistment(0))
 		res = error(_("could not remove enlistment"));
 
+	if (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)
+		res = error(_("could not stop the FSMonitor daemon"));
+

One thing that is interesting about 'scalar unregister' is that it does
not change config values. At that point, we don't know which config values
are valuable to keep or not because the user may have set them before
'scalar register', or otherwise liked the config options.

Here, the reason to stop the daemon is so we unlock the ability to delete
the directory on Windows.

Should this become part of cmd_delete() instead of unregister_dir()? Or,
do we think that users would opt to run 'scalar unregister' before trying
to delete their directory manually?

After reading this, my first thought was that 'scalar unregister' should
still turn off the FSMonitor daemon because, in addition to allowing for
directory deletion in 'scalar delete', it's "cleaning up" some
optionally-enabled behavior associated with Scalar (a la
'toggle_maintenance(0)'). However, given that 'unregister' doesn't clear
'core.fsmonitor', it really *isn't* comparable to 'toggle_maintenance(0)'.

So I think you're right that it should only be associated with enlistment
deletion (although I think 'delete_enlistment()' is the place for that -
right before 'remove_dir_recursively()' - rather than 'cmd_delete()').

Thanks!

Thanks,
-Stolee