Re: [PATCH v6 00/30] Builtin FSMonitor Part 2
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Mar 1, 2022, 19:20 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2203012014290.11118@tvgsbejvaqbjf.bet>
- In-Reply-To
- <pull.1041.v6.git.1646160212.gitgitgadget@gmail.com>
Hi Jeff,
On Tue, 1 Mar 2022, Jeff Hostetler via GitGitGadget wrote:
Show 10 quoted lines
> Here is V6 of Part 2 of my builtin FSmonitor series. > > This version contains mostly cleanup based on feedback from V5. Of note: > > * I squashed in the 1_file fix for p7519. > * I squashed in a commit from part 3 to optionally print the "running > daemon on..." message on stderr. > * I added a note to the documentation about incompatible changes around > core.fsmonitor. > * Removed/rephrased some obsolete NEEDSWORK items.
Thank you! I also saw a grammar fix ;-)
Show 5 quoted lines
> Tao has an ongoing parallel series to fix test-chmtime on Windows. > https://lore.kernel.org/all/pull.1166.git.1646041236.gitgitgadget@gmail.com/ > > If that lands first, we should be able to drop my 't/helper/test-chmtime: > skip directories on Windows' commit.
Thank you for this note so that this overwhelmed reviewer knows about this.
> A followup Part 3 will contain additional refinements to the daemon and > additional tests. I drew the line here between Part 2 and 3 to make it > easier to review.
Excellent!
I only found one slight issue with the range-diff: I _assume_ that Junio likes to add his own Sign-off (but I am being told frequently that my assumptions are typically incorrect, so take that with a grain of salt ;-)).
Apart from that, I saw that you addressed all of my concerns, and I liked in particular this elegant change:
Show 27 quoted lines
> Range-diff vs v5:
>
> [...]
> 3: 384516ce1a1 ! 3: ae622a517cf fsmonitor: config settings are repository-specific
> [...]
> @@ fsmonitor-settings.c (new)
> +
> +enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
> +{
> ++ if (!r)
> ++ r = the_repository;
> ++
> + lookup_fsmonitor_settings(r);
> +
> + return r->settings.fsmonitor->mode;
> [...]
> @@ fsmonitor.c: void remove_fsmonitor(struct index_state *istate)
> {
> unsigned int i;
> - int fsmonitor_enabled = git_config_get_fsmonitor();
> -+ struct repository *r = istate->repo ? istate->repo : the_repository;
> -+ int fsmonitor_enabled = (fsm_settings__get_mode(r) > FSMONITOR_MODE_DISABLED);
> ++ int fsmonitor_enabled = (fsm_settings__get_mode(istate->repo)
> ++ > FSMONITOR_MODE_DISABLED);
>
> if (istate->fsmonitor_dirty) {
> if (fsmonitor_enabled) {From my side, this patch series is all clear to advance to `next`.
Thank you, Dscho