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
