Re: [PATCH v6 00/30] Builtin FSMonitor Part 2

Hi Jeff,

On Tue, 1 Mar 2022, Jeff Hostetler via GitGitGadget wrote:

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 ;-)

Tao has an ongoing parallel series to fix test-chmtime on Windows.
https://lore.kernel.org/all/pull.1166.git.1646041236.gitgitgadget@gmail.com/ (local)

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:

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