From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-5.3 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A, RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 1C7DD1F953 for ; Wed, 27 Oct 2021 19:03:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240523AbhJ0TGF (ORCPT ); Wed, 27 Oct 2021 15:06:05 -0400 Received: from siwi.pair.com ([209.68.5.199]:47983 "EHLO siwi.pair.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231620AbhJ0TGE (ORCPT ); Wed, 27 Oct 2021 15:06:04 -0400 Received: from siwi.pair.com (localhost [127.0.0.1]) by siwi.pair.com (Postfix) with ESMTP id B27BE3F4820; Wed, 27 Oct 2021 15:03:38 -0400 (EDT) Received: from jeffhost-mbp.local (162-238-212-202.lightspeed.rlghnc.sbcglobal.net [162.238.212.202]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by siwi.pair.com (Postfix) with ESMTPSA id 80E383F483E; Wed, 27 Oct 2021 15:03:38 -0400 (EDT) Subject: Re: [PATCH v4 03/29] fsmonitor: config settings are repository-specific To: Junio C Hamano , Jeff Hostetler via GitGitGadget Cc: git@vger.kernel.org, Jeff Hostetler References: <882789b4dfebddb059f62b0b2edb95b92f3c69ee.1634826309.git.gitgitgadget@gmail.com> From: Jeff Hostetler Message-ID: Date: Wed, 27 Oct 2021 15:03:38 -0400 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On 10/21/21 5:05 PM, Junio C Hamano wrote: > "Jeff Hostetler via GitGitGadget" writes: > >> if (fsmonitor > 0) { >> - if (git_config_get_fsmonitor() == 0) >> + enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r); >> + >> + if (fsm_mode == FSMONITOR_MODE_DISABLED) { >> + warning(_("core.useBuiltinFSMonitor is unset; " >> + "set it if you really want to enable the " >> + "builtin fsmonitor")); >> warning(_("core.fsmonitor is unset; " >> - "set it if you really want to " >> - "enable fsmonitor")); >> + "set it if you really want to enable the " >> + "hook-based fsmonitor")); >> + } >> add_fsmonitor(&the_index); >> report(_("fsmonitor enabled")); >> } else if (!fsmonitor) { >> - if (git_config_get_fsmonitor() == 1) >> + enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r); >> + if (fsm_mode == FSMONITOR_MODE_IPC) >> + warning(_("core.useBuiltinFSMonitor is set; " >> + "remove it if you really want to " >> + "disable fsmonitor")); >> + if (fsm_mode == FSMONITOR_MODE_HOOK) >> warning(_("core.fsmonitor is set; " >> "remove it if you really want to " >> "disable fsmonitor")); > > Hmph. This does not change the behaviour per-se, but what are we > trying to achieve with these warning messages? > > The user uses --fsmonitor or --no-fsmonitor option from the command > line, presumably as a one-shot "this time I'd operate the command > differently from the configured way", so it seems unlikely that the > user is doing so because "... really want to enable/disable". The > "report()" calls in these if/else cases seem sufficient reminder of > what is going on---perhaps these warnings should be made silenceable > by turning them into advice messages? > The original code had the basic warning for `core.fsmonitor` which becomes ambiguous/confusing when we have two different config values. I was really wanting to just get rid of the warnings. They only appear if the users passes a `--[no-]fsmonitor` on the command line to temporarily override the config setting. I'll see about making them advice messages. >> -int git_config_get_fsmonitor(void) >> -{ >> - if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor)) >> - core_fsmonitor = getenv("GIT_TEST_FSMONITOR"); >> - >> - if (core_fsmonitor && !*core_fsmonitor) >> - core_fsmonitor = NULL; >> - >> - if (core_fsmonitor) >> - return 1; >> - >> - return 0; >> -} > > This used to be how we got the configuration. > >> --- a/config.h >> +++ b/config.h >> @@ -610,7 +610,6 @@ int git_config_get_pathname(const char *key, const char **dest); >> int git_config_get_index_threads(int *dest); >> int git_config_get_split_index(void); >> int git_config_get_max_percent_split_change(void); >> -int git_config_get_fsmonitor(void); > > And that is removed so any in-flight topic that adds new caller will > be caught by the compiler. OK. > >> diff --git a/environment.c b/environment.c >> index 9da7f3c1a19..68f90632245 100644 >> --- a/environment.c >> +++ b/environment.c >> @@ -82,7 +82,6 @@ int protect_hfs = PROTECT_HFS_DEFAULT; >> #define PROTECT_NTFS_DEFAULT 1 >> #endif >> int protect_ntfs = PROTECT_NTFS_DEFAULT; >> -const char *core_fsmonitor; > > So is this. > > All nice. > >> +static void lookup_fsmonitor_settings(struct repository *r) >> +{ >> + struct fsmonitor_settings *s; >> + >> + CALLOC_ARRAY(s, 1); >> + >> + r->settings.fsmonitor = s; >> + >> + if (check_for_ipc(r)) >> + return; >> + >> + if (check_for_hook(r)) >> + return; >> + >> + fsm_settings__set_disabled(r); >> +} >> + >> +enum fsmonitor_mode fsm_settings__get_mode(struct repository *r) >> +{ >> + if (!r->settings.fsmonitor) >> + lookup_fsmonitor_settings(r); > > OK, and these "lookup" calls are what make this field "lazily > loaded". A helper > > static inline void lazily_load_fsmonitor_settings(struct repository *r) > { > if (!r->settings.fsmonitor) > lookup_fsmonitor_settings(r); > } > > might be handy. Also an assert to ensure nobody calls lookup() on a > repository that already has lazily loaded the settings would be > necessary. > > static void lookup_fsmonitor_settings(struct repository *r) > { > if (r->settings.fsmonitor) > BUG("..."); > CALLOC_ARRAY(r->settings.fsmonitor, 1); good point. > >> +enum fsmonitor_mode { >> + FSMONITOR_MODE_DISABLED = 0, >> + FSMONITOR_MODE_HOOK = 1, /* core.fsmonitor */ >> + FSMONITOR_MODE_IPC = 2, /* core.useBuiltinFSMonitor */ >> +}; > > Please remind me why we need a new separate variable, instead of > turning the core.fsmonitor variable into an extended bool true, builtin>? The compatibility issues during transition is the > same either way. Old clients will ignore the request silently when > you set core.useBuiltinFSMonitor, or they will barf if you set > core.fsmonitor to 'builtin', so in a sense, extending the existing > variable may be a safer option. > >> diff --git a/repository.h b/repository.h >> index a057653981c..89a1873ade7 100644 >> --- a/repository.h >> +++ b/repository.h >> @@ -4,6 +4,7 @@ >> #include "path.h" >> >> struct config_set; >> +struct fsmonitor_settings; >> struct git_hash_algo; >> struct index_state; >> struct lock_file; >> @@ -34,6 +35,8 @@ struct repo_settings { >> int command_requires_full_index; >> int sparse_index; >> >> + struct fsmonitor_settings *fsmonitor; /* lazy loaded */ > > "lazily" loaded, I think. > >> GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor >> -code path for utilizing a file system monitor to speed up detecting >> -new or changed files. >> +code path for utilizing a (hook based) file system monitor to speed up >> +detecting new or changed files. > > Nice attention to the detail here. > > Thanks. >