{"thread":{"id":"47526","subject":"[PATCH v2 0/6] Minor fsmonitor bugfixes, use with `git diff`","startedAt":"2018-01-03T03:05:44Z","lastAt":"2018-01-08T20:58:24Z","messageCount":15,"participants":["Alex Vandiver","Johannes Schindelin","Junio C Hamano","Ben Peart"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"335690","messageId":"20180103030456.8181-1-alexmv@dropbox.com","threadId":"47526","inReplyTo":null,"subject":"[PATCH v2 0/6] Minor fsmonitor bugfixes, use with `git diff`","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2018-01-03T03:04:50Z","receivedAt":"2018-01-03T03:05:44Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"Changes in this reroll:\n - Instead of including dir.h from fsmonitor.h, stop inlining the\n   functions that made that necessary.\n\n - test-dump-fsmonitor sets a config variable soas to be able to\n   access the fsmonitor state (after it has been un-split if using the\n   split index) from the index without modifying it.  I'm least sure\n   of this change, but I'm short on ideas for a cleaner\n   implementation.\n\n - test-dump-fsmonitor now outputs a more concise output, with a\n   trailing newline, instead of just patching over the lack of\n   trailing newline on the old format.\n\n\nSomehow patch 6/6 didn't get included in Junio's\nav/fsmonitor-updates.  That's really the most useful patch of the\nseries, and the original impetus for it, as it turns out. :)\n\nBest,\n - Alex\n\n\n"},{"id":"335691","messageId":"023b0090bc7dc0ff9c3bee1efce8c85fdba27de3.1514948078.git.alexmv@dropbox.com","threadId":"47526","inReplyTo":"20180103030456.8181-1-alexmv@dropbox.com","subject":"[PATCH 1/6] Fix comments to agree with argument name","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2018-01-03T03:04:51Z","receivedAt":"2018-01-03T03:05:45Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"Signed-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n dir.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7c4b45e30..cf05b1da0 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -790,9 +790,9 @@ static int add_excludes_from_buffer(char *buf, size_t size,\n  * an index if 'istate' is non-null), parse it and store the\n  * exclude rules in \"el\".\n  *\n- * If \"ss\" is not NULL, compute SHA-1 of the exclude file and fill\n+ * If sha1_stat is not NULL, compute SHA-1 of the exclude file and fill\n  * stat data from disk (only valid if add_excludes returns zero). If\n- * ss_valid is non-zero, \"ss\" must contain good value as input.\n+ * sha1_stat.valid is non-zero, sha1_stat must contain good value as input.\n  */\n static int add_excludes(const char *fname, const char *base, int baselen,\n \t\t\tstruct exclude_list *el,\n-- \n2.15.1.31.gddce0adfe\n\n"},{"id":"335692","messageId":"121828fc14bc6f3096d16005feffb58bf68f070a.1514948078.git.alexmv@dropbox.com","threadId":"47526","inReplyTo":"20180103030456.8181-1-alexmv@dropbox.com","subject":"[PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2018-01-03T03:04:56Z","receivedAt":"2018-01-03T03:05:47Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"This makes use of the fsmonitor extension to skip lstat() calls on\nfiles that fsmonitor judged as unmodified.  We skip use of the\nfsmonitor extension when called by \"add\" because the format_callback\nin such cases expects to be called even when the file is believed to\nbe \"up to date\" with the index.\n\nSigned-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n builtin/add.c | 2 +-\n diff-lib.c    | 6 ++++++\n diff.h        | 2 ++\n 3 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex bf01d89e2..bba20b46e 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -119,7 +119,7 @@ int add_files_to_cache(const char *prefix,\n \trev.diffopt.format_callback_data = &data;\n \trev.diffopt.flags.override_submodule_config = 1;\n \trev.max_count = 0; /* do not compare unmerged paths with stage #2 */\n-\trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n+\trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED | DIFF_SKIP_FSMONITOR);\n \tclear_pathspec(&rev.prune_data);\n \treturn !!data.add_errors;\n }\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 8104603a3..13ff00d81 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -95,6 +95,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n+\tif (!(option & DIFF_SKIP_FSMONITOR))\n+\t\trefresh_fsmonitor(&the_index);\n+\n \tif (diff_unmerged_stage < 0)\n \t\tdiff_unmerged_stage = 2;\n \tentries = active_nr;\n@@ -197,6 +200,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (ce_uptodate(ce) || ce_skip_worktree(ce))\n \t\t\tcontinue;\n \n+\t\tif (ce->ce_flags & CE_FSMONITOR_VALID && !(option & DIFF_SKIP_FSMONITOR))\n+\t\t\tcontinue;\n+\n \t\t/* If CE_VALID is set, don't look at workdir for file removal */\n \t\tif (ce->ce_flags & CE_VALID) {\n \t\t\tchanged = 0;\ndiff --git a/diff.h b/diff.h\nindex 7cf276f07..5cf5866bd 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -392,6 +392,8 @@ extern const char *diff_aligned_abbrev(const struct object_id *sha1, int);\n #define DIFF_SILENT_ON_REMOVED 01\n /* report racily-clean paths as modified */\n #define DIFF_RACY_IS_MODIFIED 02\n+/* skip loading the fsmonitor data */\n+#define DIFF_SKIP_FSMONITOR 04\n extern int run_diff_files(struct rev_info *revs, unsigned int option);\n extern int run_diff_index(struct rev_info *revs, int cached);\n \n-- \n2.15.1.31.gddce0adfe\n\n"},{"id":"335693","messageId":"0ee51f4baaf07f388782e7a5904dcc6360e86f3d.1514948078.git.alexmv@dropbox.com","threadId":"47526","inReplyTo":"20180103030456.8181-1-alexmv@dropbox.com","subject":"[PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2018-01-03T03:04:55Z","receivedAt":"2018-01-03T03:05:49Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"These were mistakenly left in when the test was introduced, in\n1487372d3 (\"fsmonitor: store fsmonitor bitmap before splitting index\",\n2017-11-09)\n\nSigned-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n t/t7519-status-fsmonitor.sh | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex eb2d13bbc..19b2a0a0f 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -307,9 +307,7 @@ test_expect_success 'splitting the index results in the same state' '\n \tdirty_repo &&\n \tgit update-index --fsmonitor  &&\n \tgit ls-files -f >expect &&\n-\ttest-dump-fsmonitor >&2 && echo &&\n \tgit update-index --fsmonitor --split-index &&\n-\ttest-dump-fsmonitor >&2 && echo &&\n \tgit ls-files -f >actual &&\n \ttest_cmp expect actual\n '\n-- \n2.15.1.31.gddce0adfe\n\n"},{"id":"335694","messageId":"9bb36cea369530b980f6542e3e0f24dc142a20a8.1514948078.git.alexmv@dropbox.com","threadId":"47526","inReplyTo":"20180103030456.8181-1-alexmv@dropbox.com","subject":"[PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2018-01-03T03:04:52Z","receivedAt":"2018-01-03T03:05:51Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"These were inline'd when they were first introduced, presumably as an\noptimization for cases when they were called in tight loops.  This\ncomplicates using these functions, as untracked_cache_invalidate_path\nis defined in dir.h.\n\nLeave the inline'ing up to the compiler's decision, for ease of use.\n\nSigned-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n fsmonitor.c | 18 ++++++++++++++++++\n fsmonitor.h | 17 ++---------------\n 2 files changed, 20 insertions(+), 15 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 0af7c4edb..df084235b 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -194,6 +194,24 @@ void refresh_fsmonitor(struct index_state *istate)\n \tistate->fsmonitor_last_update = last_update;\n }\n \n+void mark_fsmonitor_valid(struct cache_entry *ce)\n+{\n+\tif (core_fsmonitor) {\n+\t\tce->ce_flags |= CE_FSMONITOR_VALID;\n+\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_clean '%s'\", ce->name);\n+\t}\n+}\n+\n+void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)\n+{\n+\tif (core_fsmonitor) {\n+\t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\n+\t\tuntracked_cache_invalidate_path(istate, ce->name);\n+\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_invalid '%s'\", ce->name);\n+\t}\n+}\n+\n+\n void add_fsmonitor(struct index_state *istate)\n {\n \tint i;\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex cd3cc0ccf..6328745b2 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -46,13 +46,7 @@ extern void refresh_fsmonitor(struct index_state *istate);\n  * called any time the cache entry has been updated to reflect the\n  * current state of the file on disk.\n  */\n-static inline void mark_fsmonitor_valid(struct cache_entry *ce)\n-{\n-\tif (core_fsmonitor) {\n-\t\tce->ce_flags |= CE_FSMONITOR_VALID;\n-\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_clean '%s'\", ce->name);\n-\t}\n-}\n+extern void mark_fsmonitor_valid(struct cache_entry *ce);\n \n /*\n  * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate\n@@ -61,13 +55,6 @@ static inline void mark_fsmonitor_valid(struct cache_entry *ce)\n  * trigger an lstat() or invalidate the untracked cache for the\n  * corresponding directory\n  */\n-static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)\n-{\n-\tif (core_fsmonitor) {\n-\t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\n-\t\tuntracked_cache_invalidate_path(istate, ce->name);\n-\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_invalid '%s'\", ce->name);\n-\t}\n-}\n+extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);\n \n #endif\n-- \n2.15.1.31.gddce0adfe\n\n"},{"id":"335695","messageId":"36db77ccb5c025a73bf3f5841cd11607427ffdc0.1514948078.git.alexmv@dropbox.com","threadId":"47526","inReplyTo":"20180103030456.8181-1-alexmv@dropbox.com","subject":"[PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2018-01-03T03:04:54Z","receivedAt":"2018-01-03T03:05:53Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"Rather than display one very long line, summarize the contents of that\nline.  The tests do not currently rely on any content except the first\nline (\"no fsmonitor\" / \"fsmonitor last update\").\n\nSigned-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n t/helper/test-dump-fsmonitor.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\nindex 48c4bab0b..5d61b0d62 100644\n--- a/t/helper/test-dump-fsmonitor.c\n+++ b/t/helper/test-dump-fsmonitor.c\n@@ -4,7 +4,8 @@\n int cmd_main(int ac, const char **av)\n {\n \tstruct index_state *istate = &the_index;\n-\tint i;\n+\tuint64_t now = getnanotime();\n+\tint i, valid = 0;\n \n \tgit_config_push_parameter(\"core.fsmonitor=keep\");\n \tsetup_git_directory();\n@@ -14,10 +15,17 @@ int cmd_main(int ac, const char **av)\n \t\tprintf(\"no fsmonitor\\n\");\n \t\treturn 0;\n \t}\n-\tprintf(\"fsmonitor last update %\"PRIuMAX\"\\n\", (uintmax_t)istate->fsmonitor_last_update);\n+\n+\tprintf(\"fsmonitor last update %\"PRIuMAX\", (%.2f seconds ago)\\n\",\n+\t       (uintmax_t)istate->fsmonitor_last_update,\n+\t       (now - istate->fsmonitor_last_update)/1.0e9);\n \n \tfor (i = 0; i < istate->cache_nr; i++)\n-\t\tprintf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? \"+\" : \"-\");\n+\t\tif (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID)\n+\t\t\tvalid++;\n+\n+\tprintf(\"  valid: %d\\n\", valid);\n+\tprintf(\"  invalid: %d\\n\", istate->cache_nr - valid);\n \n \treturn 0;\n }\n-- \n2.15.1.31.gddce0adfe\n\n"},{"id":"335696","messageId":"fb10a998d5a8250f26d3504a1d1f5ca6723160d7.1514948078.git.alexmv@dropbox.com","threadId":"47526","inReplyTo":"20180103030456.8181-1-alexmv@dropbox.com","subject":"[PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2018-01-03T03:04:53Z","receivedAt":"2018-01-03T03:05:56Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"dd9005a0a (\"fsmonitor: delay updating state until after split index is\nmerged\", 2017-10-27) began deferring the setting of the\nCE_FSMONITOR_VALID flag until later, such that do_read_index() does\nnot perform those steps.  This means that t/helper/test-dump-fsmonitor\nshowed all bits as always unset.\n\nLoad the index using read_index_from(), which is aware of split\nindexes and later fsmonitor ewah inflation, but ensure that we do not\nadd or remove it, by setting the value to \"keep\".\n\nSigned-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n config.c                       | 9 +++++++--\n t/helper/test-dump-fsmonitor.c | 4 +++-\n 2 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex e617c2018..7c6ed888e 100644\n--- a/config.c\n+++ b/config.c\n@@ -2174,8 +2174,13 @@ int git_config_get_fsmonitor(void)\n \tif (core_fsmonitor && !*core_fsmonitor)\n \t\tcore_fsmonitor = NULL;\n \n-\tif (core_fsmonitor)\n-\t\treturn 1;\n+\n+\tif (core_fsmonitor) {\n+\t\tif (!strcasecmp(core_fsmonitor, \"keep\"))\n+\t\t\treturn -1;\n+\t\telse\n+\t\t\treturn 1;\n+\t}\n \n \treturn 0;\n }\ndiff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\nindex ad452707e..48c4bab0b 100644\n--- a/t/helper/test-dump-fsmonitor.c\n+++ b/t/helper/test-dump-fsmonitor.c\n@@ -1,12 +1,14 @@\n #include \"cache.h\"\n+#include \"config.h\"\n \n int cmd_main(int ac, const char **av)\n {\n \tstruct index_state *istate = &the_index;\n \tint i;\n \n+\tgit_config_push_parameter(\"core.fsmonitor=keep\");\n \tsetup_git_directory();\n-\tif (do_read_index(istate, get_index_file(), 0) < 0)\n+\tif (read_index_from(istate, get_index_file()) < 0)\n \t\tdie(\"unable to read index file\");\n \tif (!istate->fsmonitor_last_update) {\n \t\tprintf(\"no fsmonitor\\n\");\n-- \n2.15.1.31.gddce0adfe\n\n"},{"id":"335876","messageId":"alpine.DEB.2.21.1.1801042326581.32@MININT-6BKU6QN.europe.corp.microsoft.com","threadId":"47526","inReplyTo":"9bb36cea369530b980f6542e3e0f24dc142a20a8.1514948078.git.alexmv@dropbox.com","subject":"Re: [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-01-04T22:27:58Z","receivedAt":"2018-01-04T22:28:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Tue, 2 Jan 2018, Alex Vandiver wrote:\n\n> These were inline'd when they were first introduced, presumably as an\n> optimization for cases when they were called in tight loops.  This\n> complicates using these functions, as untracked_cache_invalidate_path\n> is defined in dir.h.\n> \n> Leave the inline'ing up to the compiler's decision, for ease of use.\n\nAs a compromise, you could leave the rather simple mark_fsmonitor_valid()\nas inlined function. It should be by far the more-called function, anyway.\n\nCiao,\nJohannes\n"},{"id":"335877","messageId":"alpine.DEB.2.21.1.1801042331590.32@MININT-6BKU6QN.europe.corp.microsoft.com","threadId":"47526","inReplyTo":"36db77ccb5c025a73bf3f5841cd11607427ffdc0.1514948078.git.alexmv@dropbox.com","subject":"Re: [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-01-04T22:33:35Z","receivedAt":"2018-01-04T22:33:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Tue, 2 Jan 2018, Alex Vandiver wrote:\n\n> Rather than display one very long line, summarize the contents of that\n> line.  The tests do not currently rely on any content except the first\n> line (\"no fsmonitor\" / \"fsmonitor last update\").\n\nThe more interesting part would be the entries with outdated (\"invalid\")\ninformation. I thought that this information was pretty useful for\ndebugging. Maybe we could still keep at least that part, or at least\ntrigger outputting it via a command-line flag?\n\nCiao,\nJohannes\n"},{"id":"335883","messageId":"alpine.DEB.2.21.1.1801042335130.32@MININT-6BKU6QN.europe.corp.microsoft.com","threadId":"47526","inReplyTo":"121828fc14bc6f3096d16005feffb58bf68f070a.1514948078.git.alexmv@dropbox.com","subject":"Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-01-04T22:46:27Z","receivedAt":"2018-01-04T22:46:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Tue, 2 Jan 2018, Alex Vandiver wrote:\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 8104603a3..13ff00d81 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -95,6 +95,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>  \n>  \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n>  \n> +\tif (!(option & DIFF_SKIP_FSMONITOR))\n> +\t\trefresh_fsmonitor(&the_index);\n> +\n>  \tif (diff_unmerged_stage < 0)\n>  \t\tdiff_unmerged_stage = 2;\n\nI read over this hunk five times, and only now am I able to wrap my head\naround this: if we do *not* want to skip the fsmonitor data, we refresh\nthe fsmonitor data in the index.\n\nThat feels a bit like an unneeded double negation. Speaking for myself, I\nwould prefore `DIFF_IGNORE_FSMONITOR` instead, it would feel less like a\ndouble negation then. But I am not a native speaker, so I might be wrong.\n\n> +               if (ce->ce_flags & CE_FSMONITOR_VALID && !(option & DIFF_SKIP_FSMONITOR))\n> +                       continue;\n\nSince we do expect this to be called without the DIFF_SKIP_FSMONITOR flag,\nI guess it makes sense to order it this way.\n\nI still have troubles to understand why we ignore the fsmonitor data with\n`git add`, though... we want to add only modified files, right? I thought\nthat the fsmonitor data could help performance exactly there (I am\nthinking of a certain insanely large code base where a developer might\nwant to change only one or maybe 3 files out of an entire machine workshop\nof files, and with fsmonitor it should be a really fast operation because\nit should ignore all but those few files, right?)... Could you maybe try\nto help me understand that better?\n\nThanks,\nJohannes\n"},{"id":"335891","messageId":"alpine.DEB.2.21.1.1801042348500.32@MININT-6BKU6QN.europe.corp.microsoft.com","threadId":"47526","inReplyTo":"fb10a998d5a8250f26d3504a1d1f5ca6723160d7.1514948078.git.alexmv@dropbox.com","subject":"Re: [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-01-04T23:03:24Z","receivedAt":"2018-01-04T23:03:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Tue, 2 Jan 2018, Alex Vandiver wrote:\n\n> diff --git a/config.c b/config.c\n> index e617c2018..7c6ed888e 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2174,8 +2174,13 @@ int git_config_get_fsmonitor(void)\n>  \tif (core_fsmonitor && !*core_fsmonitor)\n>  \t\tcore_fsmonitor = NULL;\n>  \n> -\tif (core_fsmonitor)\n> -\t\treturn 1;\n> +\n> +\tif (core_fsmonitor) {\n> +\t\tif (!strcasecmp(core_fsmonitor, \"keep\"))\n> +\t\t\treturn -1;\n> +\t\telse\n> +\t\t\treturn 1;\n> +\t}\n\nIt took me a while to reason about this:\n\n- there is no existing code path that can return -1 from\n  git_config_get_fsmonitor(),\n\n- the callers in builtin/update-index.c (testing explicitly for 0 and 1)\n  do not matter because they only trigger warnings.\n\n- the remaining two callers are in fsmonitor.c:\n\n  - tweak_fsmonitor() (which handles -1 specifically), and\n\n  - inflate_fsmonitor_ewah(), which only tests whether\n    git_config_get_fsmonitor() returned a non-zero value, but that test is\n    inside a code block that is only triggered if the index has an\n    fsmonitor_dirty array, meaning: it already had fsmonitor enabled.\n    Therefore the test is legitimate.\n\nThis would take the next reader as much time, I would wager a bet. So\nmaybe you can include this information (or at least the information about\ninflate_fsmonitor_ewah()) in the commit message?\n\n> diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\n> index ad452707e..48c4bab0b 100644\n> --- a/t/helper/test-dump-fsmonitor.c\n> +++ b/t/helper/test-dump-fsmonitor.c\n> @@ -1,12 +1,14 @@\n>  #include \"cache.h\"\n> +#include \"config.h\"\n>  \n>  int cmd_main(int ac, const char **av)\n>  {\n>  \tstruct index_state *istate = &the_index;\n>  \tint i;\n>  \n> +\tgit_config_push_parameter(\"core.fsmonitor=keep\");\n\nThe alternative would be to use an environment variable. We already use\nGIT_FSMONITOR_TEST.\n\nHowever, I wonder why we need this. Do we really update the index anywhere\nin the tests, then *toggle* the core.fsmonitor setting, and *then* call\ntest-dump-fsmonitor?\n\nAnd if we do, can't we simply avoid it?\n\nCiao,\nJohannes\n"},{"id":"336023","messageId":"xmqqefn4aqt8.fsf@gitster.mtv.corp.google.com","threadId":"47526","inReplyTo":"alpine.DEB.2.21.1.1801042335130.32@MININT-6BKU6QN.europe.corp.microsoft.com","subject":"Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-05T22:22:59Z","receivedAt":"2018-01-05T22:23:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> diff --git a/diff-lib.c b/diff-lib.c\n>> index 8104603a3..13ff00d81 100644\n>> --- a/diff-lib.c\n>> +++ b/diff-lib.c\n>> @@ -95,6 +95,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>>  \n>>  \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n>>  \n>> +\tif (!(option & DIFF_SKIP_FSMONITOR))\n>> +\t\trefresh_fsmonitor(&the_index);\n>> +\n>>  \tif (diff_unmerged_stage < 0)\n>>  \t\tdiff_unmerged_stage = 2;\n>\n> I read over this hunk five times, and only now am I able to wrap my head\n> around this: if we do *not* want to skip the fsmonitor data, we refresh\n> the fsmonitor data in the index.\n>\n> That feels a bit like an unneeded double negation. Speaking for myself, I\n> would prefore `DIFF_IGNORE_FSMONITOR` instead, it would feel less like a\n> double negation then. But I am not a native speaker, so I might be wrong.\n\nI do find the logic a bit convoluted with double negative.\n"},{"id":"336187","messageId":"01ad47b4-aa5e-461a-270b-dd60032afbd1@gmail.com","threadId":"47526","inReplyTo":"alpine.DEB.2.21.1.1801042326581.32@MININT-6BKU6QN.europe.corp.microsoft.com","subject":"Re: [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-01-08T20:27:41Z","receivedAt":"2018-01-08T20:27:55Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 1/4/2018 5:27 PM, Johannes Schindelin wrote:\n> Hi Alex,\n> \n> On Tue, 2 Jan 2018, Alex Vandiver wrote:\n> \n>> These were inline'd when they were first introduced, presumably as an\n>> optimization for cases when they were called in tight loops.  This\n>> complicates using these functions, as untracked_cache_invalidate_path\n>> is defined in dir.h.\n>>\n>> Leave the inline'ing up to the compiler's decision, for ease of use.\n> \n\nI'm fine with these not being inline.  I was attempting to minimize the \nperformance impact of the fsmonitor code when it was not even turned on. \n  Inlineing these functions allowed it to be kept to a simple test but I \nsuspect (especially with modern optimizing compilers) that the overhead \nof calling a function to do that test is negligible.\n\n> As a compromise, you could leave the rather simple mark_fsmonitor_valid()\n> as inlined function. It should be by far the more-called function, anyway.\n> \n> Ciao,\n> Johannes\n> \n"},{"id":"336188","messageId":"af33fb18-fd31-727b-efcc-b3873c6e58f1@gmail.com","threadId":"47526","inReplyTo":"alpine.DEB.2.21.1.1801042331590.32@MININT-6BKU6QN.europe.corp.microsoft.com","subject":"Re: [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-01-08T20:33:24Z","receivedAt":"2018-01-08T20:33:32Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 1/4/2018 5:33 PM, Johannes Schindelin wrote:\n> Hi Alex,\n> \n> On Tue, 2 Jan 2018, Alex Vandiver wrote:\n> \n>> Rather than display one very long line, summarize the contents of that\n>> line.  The tests do not currently rely on any content except the first\n>> line (\"no fsmonitor\" / \"fsmonitor last update\").\n> \n> The more interesting part would be the entries with outdated (\"invalid\")\n> information. I thought that this information was pretty useful for\n> debugging. Maybe we could still keep at least that part, or at least\n> trigger outputting it via a command-line flag?\n> \n\nDuring the development and testing of fsmonitor, I found the '+-' to be \nhelpful (especially since it is in index order).  I could touch a file \nand verify that it showed up as invalid and that it was the file I \nexpected by its placement in the index.\n\nI'd hate to have to add options to a test program for more/less output. \nI do like your additions of the time since updated and the final counts. \n  I prefer more information rather than less in my test tools - how \nabout this?\n\n\ndiff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\nindex 5d61b0d621..8503da288d 100644\n--- a/t/helper/test-dump-fsmonitor.c\n+++ b/t/helper/test-dump-fsmonitor.c\n@@ -20,11 +20,13 @@ int cmd_main(int ac, const char **av)\n                (uintmax_t)istate->fsmonitor_last_update,\n                (now - istate->fsmonitor_last_update)/1.0e9);\n\n-       for (i = 0; i < istate->cache_nr; i++)\n+       for (i = 0; i < istate->cache_nr; i++) {\n+               printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) \n? \"+\" : \"-\");\n                 if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID)\n                         valid++;\n+       }\n\n-       printf(\"  valid: %d\\n\", valid);\n+       printf(\"\\n  valid: %d\\n\", valid);\n         printf(\"  invalid: %d\\n\", istate->cache_nr - valid);\n\n         return 0;\n\n\n> Ciao,\n> Johannes\n> \n"},{"id":"336199","messageId":"6f7b31f7-3532-a7b4-5846-d2994dcb8795@gmail.com","threadId":"47526","inReplyTo":"xmqqefn4aqt8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-01-08T20:58:17Z","receivedAt":"2018-01-08T20:58:24Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 1/5/2018 5:22 PM, Junio C Hamano wrote:\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n>>> diff --git a/diff-lib.c b/diff-lib.c\n>>> index 8104603a3..13ff00d81 100644\n>>> --- a/diff-lib.c\n>>> +++ b/diff-lib.c\n>>> @@ -95,6 +95,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>>>   \n>>>   \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n>>>   \n>>> +\tif (!(option & DIFF_SKIP_FSMONITOR))\n>>> +\t\trefresh_fsmonitor(&the_index);\n>>> +\n>>>   \tif (diff_unmerged_stage < 0)\n>>>   \t\tdiff_unmerged_stage = 2;\n>>\n>> I read over this hunk five times, and only now am I able to wrap my head\n>> around this: if we do *not* want to skip the fsmonitor data, we refresh\n>> the fsmonitor data in the index.\n>>\n>> That feels a bit like an unneeded double negation. Speaking for myself, I\n>> would prefore `DIFF_IGNORE_FSMONITOR` instead, it would feel less like a\n>> double negation then. But I am not a native speaker, so I might be wrong.\n> \n> I do find the logic a bit convoluted with double negative.\n> \n\nIt's great to see more use of the fsmonitor data.  Thanks for doing this!\n\nI agree with the sentiment that the logic as written is confusing.  I'll \nalso point out that DIFF_IGNORE_FSMONITOR would be more consistent with \nthe similar CE_MATCH_IGNORE_FSMONITOR flag and logic.\n\nI'm also confused why we would not want to use the fsmonitor data in the \n'add' case.  When would you ever need to add a file that had not been \nmodified?\n"}]}