From: Jeff Hostetler <git@jeffhostetler.com>
Date: Fri, 11 Mar 2022 22:25:49 GMT
Subject: Re: [PATCH v2 04/27] fsmonitor-settings: bare repos are incompatible with FSMonitor
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:
> 
> 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.

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


> 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