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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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 = &the_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