Re: [PATCH v2 04/27] fsmonitor-settings: bare repos are incompatible with FSMonitor

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Mar 11, 2022, 22:25 UTC
Message-ID: <73f6702b-95cd-794e-6ca7-5c995294935a@jeffhostetler.com>
In-Reply-To: <220311.86k0d1i7xr.gmgdl@evledraar.gmail.com>

On 3/10/22 8:31 PM, Ævar Arnfjörð Bjarmason wrote:

Show 42 quoted lines

> 
> On Tue, Mar 08 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>> [...]
>> +	prepare_repo_settings(the_repository);
>> +	fsm_settings__set_ipc(the_repository);
>> +
>> +	if (fsm_settings__get_mode(the_repository) == FSMONITOR_MODE_INCOMPATIBLE) {
>> +		const char *msg = fsm_settings__get_reason_msg(the_repository);
>> +
>> +		return error("%s '%s'", msg ? msg : "???", xgetcwd());
>> +	}
>> +
>>   	if (!strcmp(subcmd, "start"))
>>   		return !!try_to_start_background_daemon();
>>   
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index d335f1ac72a..8f460e7195f 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1237,6 +1237,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>>   
>>   	if (fsmonitor > 0) {
>>   		enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
>> +
>> +		if (fsm_mode == FSMONITOR_MODE_INCOMPATIBLE) {
>> +			const char *msg = fsm_settings__get_reason_msg(r);
>> +
>> +			return error("%s '%s'", msg ? msg : "???", xgetcwd());
>> +		}
>> +
>>   		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
>>   			advise(_("core.fsmonitor is unset; "
>>   				 "set it if you really want to "
> 
> Can w assert somewhere earlier that ->mode can't be
> FSMONITOR_MODE_INCOMPATIBLE at the same time that ->reason ==
> FSMONITOR_REASON_OK, should that ever happen?
> 
> Then we can get rid of the "???" case here.
>

Yeah, it would be nice to assert() the pair and simplify things. I'll make a note to look at that.

> The "%s '%s'" here should really be marked for translation, but just
> "some reason '$path'" is a pretty confusing message. This will emit
> e.g.:

I already have translations in the code that looks up the message, so doing it here for a pair of %s's felt wrong.

Show 7 quoted lines

> 
>      "bare repos are incompatible with fsmonitor '/some/path/to/repo'"
> 
> Since we always hand these to error maybe have the helper do e.g.:
> 
>      error(_("bare repository '%s' is incompatible with fsmonitor"), path);
>

I'm wondering now if we should just drop the path from the message and use the error message from __get_reason_msg() as is. (It was useful during debugging, but I could see it going away.)

Show 14 quoted lines

> I find the second-guessing in fsmonitor-settings.c really hard to
> follow, i.e. how seemingly every function has some "not loaded yet? load
> it" instead of a more typical "init it", "use it", "free it"
> pattern. Including stuff like this:
> 	
> 	enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
> 	{
> 	        if (!r)
> 	                r = the_repository;
> 
> But anyway, seeing as we do try really hard to load the_repository (or a
> repository) can't we use the_repository->gitdir etc. here instead of
> xgetcwd(), or the_repository->worktree when non-bare?
>

I'll take a look and see if I can simplify it.

Thanks Jeff