Re: [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later

[Johannes Schindelin]

Hi Alex,

On Tue, 2 Jan 2018, Alex Vandiver wrote:

> diff --git a/config.c b/config.c
> index e617c2018..7c6ed888e 100644
> --- a/config.c
> +++ b/config.c
> @@ -2174,8 +2174,13 @@ int git_config_get_fsmonitor(void)
>   if (core_fsmonitor && !*core_fsmonitor)
>   core_fsmonitor = NULL;
>  
> - if (core_fsmonitor)
> - return 1;
> +
> + if (core_fsmonitor) {
> + if (!strcasecmp(core_fsmonitor, "keep"))
> + return -1;
> + else
> + return 1;
> + }

It took me a while to reason about this:

- there is no existing code path that can return -1 from
  git_config_get_fsmonitor(),

- the callers in builtin/update-index.c (testing explicitly for 0 and 1)
  do not matter because they only trigger warnings.

- the remaining two callers are in fsmonitor.c:

  - tweak_fsmonitor() (which handles -1 specifically), and

  - inflate_fsmonitor_ewah(), which only tests whether
git_config_get_fsmonitor() returned a non-zero value, but that test is
inside a code block that is only triggered if the index has an
fsmonitor_dirty array, meaning: it already had fsmonitor enabled.
Therefore the test is legitimate.

This would take the next reader as much time, I would wager a bet. So
maybe you can include this information (or at least the information about
inflate_fsmonitor_ewah()) in the commit message?

> diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
> index ad452707e..48c4bab0b 100644
> --- a/t/helper/test-dump-fsmonitor.c
> +++ b/t/helper/test-dump-fsmonitor.c
> @@ -1,12 +1,14 @@
>  #include "cache.h"
> +#include "config.h"
>  
>  int cmd_main(int ac, const char **av)
>  {
>   struct index_state *istate = _index;
>   int i;
>  
> + git_config_push_parameter("core.fsmonitor=keep");

The alternative would be to use an environment variable. We already use
GIT_FSMONITOR_TEST.

However, I wonder why we need this. Do we really update the index anywhere
in the tests, then *toggle* the core.fsmonitor setting, and *then* call
test-dump-fsmonitor?

And if we do, can't we simply avoid it?

Ciao,
Johannes

Re: [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later

[Alex Vandiver]

On Mon, 18 Dec 2017, Alex Vandiver wrote:
> dd9005a0a ("fsmonitor: delay updating state until after split index is
> merged", 2017-10-27) began deferring the setting of the
> CE_FSMONITOR_VALID flag until later, such that do_read_index() does
> not perform those steps.  This means that t/helper/test-dump-fsmonitor
> showed all bits as always unset.

This isn't the right fix, actually.  With split indexes, this puts us
right back into "only shows a few bits" territory, because
do_read_index doesn't know about split indexes.

Which means we need a way to do the whole index load but _not_
add/remove the fsmonitor cache, even if the config says to do so.

The best I'm coming up with is the below -- but I'm not happy with
it, because 'keep' is meaningless as a configuration value outside of
testing, since it's normally treated as an executable path.  This uses
the fact that fsmonitor.c currently has a:

switch (fsmonitor_enabled) {
case -1: /* keep: do nothing */
break;

...despite get_config_get_fsmonitor() havong no way to return -1 at
present.

Is this sort of testing generally done via environment variables,
rather than magic config values?
 - Alex

-8<
diff --git a/config.c b/config.c
index 6fb06c213..75fcf1a52 100644
--- a/config.c
+++ b/config.c
@@ -2164,8 +2164,13 @@ int git_config_get_fsmonitor(void)
if (core_fsmonitor && !*core_fsmonitor)
core_fsmonitor = NULL;

-   if (core_fsmonitor)
-   return 1;
+
+   if (core_fsmonitor) {
+   if (!strcasecmp(core_fsmonitor, "keep"))
+   return -1;
+   else
+   return 1;
+   }

return 0;
 }
diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
index ad452707e..12e131530 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -5,8 +5,9 @@ int cmd_main(int ac, const char **av)
struct index_state *istate = _index;
int i;

+   git_config_push_parameter("core.fsmonitor=keep");
setup_git_directory();
-   if (do_read_index(istate, get_index_file(), 0) < 0)
+   if (read_index_from(istate, get_index_file()) < 0)
die("unable to read index file");
if (!istate->fsmonitor_last_update) {
printf("no fsmonitor\n");
-8<-