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

[Victoria Dye <vdye@github.com>]

Derrick Stolee wrote:
> On 8/16/2022 7:58 PM, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> 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).
> 
>> +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