{"thread":{"id":"47463","subject":"[PATCH 0/6] Minor fsmonitor bugfixes, use with `git diff`","startedAt":"2017-12-19T00:29:41Z","lastAt":"2017-12-21T21:50:04Z","messageCount":16,"participants":["Alex Vandiver","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"335006","messageId":"20171219002858.22214-1-alexmv@dropbox.com","threadId":"47463","inReplyTo":null,"subject":"[PATCH 0/6] Minor fsmonitor bugfixes, use with `git diff`","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-12-19T00:28:52Z","receivedAt":"2017-12-19T00:29:41Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"This cleans up some mistakes I introduced in my previous series, and\nswitches `git diff` to use the fsmonitor data.\n\nI've noticed that `git checkout HEAD` drops the fsmonitor data, which\nsurprises me -- the following patch \"fixes\" that but broke tests, so\nthere's something I clearly don't understand yet going on here:\n\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1262,6 +1262,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n        o->result.timestamp.nsec = o->src_index->timestamp.nsec;\n        o->result.version = o->src_index->version;\n        o->result.split_index = o->src_index->split_index;\n+       o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n        if (o->result.split_index)\n                o->result.split_index->refcount++;\n        hashcpy(o->result.sha1, o->src_index->sha1);\n\n\n - Alex\n\n"},{"id":"335007","messageId":"95804e03dec9bd9d1a28ab92ed4356c37950468f.1513642743.git.alexmv@dropbox.com","threadId":"47463","inReplyTo":"20171219002858.22214-1-alexmv@dropbox.com","subject":"[PATCH 1/6] Fix comments to agree with argument name","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-12-19T00:28:53Z","receivedAt":"2017-12-19T00:29:43Z","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.626.gc4617b774\n\n"},{"id":"335008","messageId":"c8cf261d9d620d8123e8bfa5aa952fa55685a8db.1513642743.git.alexmv@dropbox.com","threadId":"47463","inReplyTo":"20171219002858.22214-1-alexmv@dropbox.com","subject":"[PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-12-19T00:28:54Z","receivedAt":"2017-12-19T00:29:45Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"This missing include is currently hidden by dint of the fact that\ndir.h is already included by all things that currently include\nfsmonitor.h\n\nSigned-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n fsmonitor.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex cd3cc0ccf..5f68ca4d2 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -1,5 +1,6 @@\n #ifndef FSMONITOR_H\n #define FSMONITOR_H\n+#include \"dir.h\"\n \n extern struct trace_key trace_fsmonitor;\n \n-- \n2.15.1.626.gc4617b774\n\n"},{"id":"335009","messageId":"dab89f071d22a21b85dff5d31e9e9a8ceb6458e3.1513642743.git.alexmv@dropbox.com","threadId":"47463","inReplyTo":"20171219002858.22214-1-alexmv@dropbox.com","subject":"[PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-12-19T00:28:56Z","receivedAt":"2017-12-19T00:29:46Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"This makes it more readable when used for debugging from the\ncommandline.\n\nSigned-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n t/helper/test-dump-fsmonitor.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\nindex 53b19b39b..4e56929f7 100644\n--- a/t/helper/test-dump-fsmonitor.c\n+++ b/t/helper/test-dump-fsmonitor.c\n@@ -19,5 +19,6 @@ int cmd_main(int ac, const char **av)\n \tfor (i = 0; i < istate->cache_nr; i++)\n \t\tprintf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? \"+\" : \"-\");\n \n+\tprintf(\"\\n\");\n \treturn 0;\n }\n-- \n2.15.1.626.gc4617b774\n\n"},{"id":"335010","messageId":"e3246bad10b891d8e3f751b6ed368a9e3f37c425.1513642743.git.alexmv@dropbox.com","threadId":"47463","inReplyTo":"20171219002858.22214-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":"2017-12-19T00:28:55Z","receivedAt":"2017-12-19T00:29:48Z","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\nSplit out the code which inflates the ewah into CE_FSMONITOR_VALID\nbits, and call that from t/helper/test-dump-fsmonitor.  We cannot\nsimply switch the code to call read_index_from or the more specific\ntweak_fsmonitor, because the latter would modify the extension state\nby calling add_fsmonitor.\n\nSigned-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n fsmonitor.c                    | 9 ++++++++-\n fsmonitor.h                    | 6 ++++++\n t/helper/test-dump-fsmonitor.c | 2 ++\n 3 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 0af7c4edb..7011dff15 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -227,7 +227,7 @@ void remove_fsmonitor(struct index_state *istate)\n \t}\n }\n \n-void tweak_fsmonitor(struct index_state *istate)\n+void inflate_fsmonitor_ewah(struct index_state *istate)\n {\n \tint i;\n \tint fsmonitor_enabled = git_config_get_fsmonitor();\n@@ -250,6 +250,13 @@ void tweak_fsmonitor(struct index_state *istate)\n \t\tewah_free(istate->fsmonitor_dirty);\n \t\tistate->fsmonitor_dirty = NULL;\n \t}\n+}\n+\n+void tweak_fsmonitor(struct index_state *istate)\n+{\n+\tint fsmonitor_enabled = git_config_get_fsmonitor();\n+\n+\tinflate_fsmonitor_ewah(istate);\n \n \tswitch (fsmonitor_enabled) {\n \tcase -1: /* keep: do nothing */\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex 5f68ca4d2..619852d4b 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -28,6 +28,12 @@ extern void write_fsmonitor_extension(struct strbuf *sb, struct index_state *ist\n extern void add_fsmonitor(struct index_state *istate);\n extern void remove_fsmonitor(struct index_state *istate);\n \n+/*\n+ * Inflate the fsmonitor_dirty ewah into the CE_FSMONITOR_VALID bits.\n+ * Called by tweak_fsmonitor.\n+ */\n+extern void inflate_fsmonitor_ewah(struct index_state *istate);\n+\n /*\n  * Add/remove the fsmonitor index extension as necessary based on the current\n  * core.fsmonitor setting.\ndiff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\nindex ad452707e..53b19b39b 100644\n--- a/t/helper/test-dump-fsmonitor.c\n+++ b/t/helper/test-dump-fsmonitor.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"fsmonitor.h\"\n \n int cmd_main(int ac, const char **av)\n {\n@@ -8,6 +9,7 @@ int cmd_main(int ac, const char **av)\n \tsetup_git_directory();\n \tif (do_read_index(istate, get_index_file(), 0) < 0)\n \t\tdie(\"unable to read index file\");\n+\tinflate_fsmonitor_ewah(istate);\n \tif (!istate->fsmonitor_last_update) {\n \t\tprintf(\"no fsmonitor\\n\");\n \t\treturn 0;\n-- \n2.15.1.626.gc4617b774\n\n"},{"id":"335011","messageId":"564465f82fc637c10af1fa6531a143a029555580.1513642743.git.alexmv@dropbox.com","threadId":"47463","inReplyTo":"20171219002858.22214-1-alexmv@dropbox.com","subject":"[PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-12-19T00:28:58Z","receivedAt":"2017-12-19T00:29:53Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"With fsmonitor enabled, the first call to match_stat_with_submodule\ncalls refresh_fsmonitor, incurring the overhead of reading the list of\nupdated files -- but run_diff_files does not respect the\nCE_FSMONITOR_VALID flag.\n\nMake use of the fsmonitor extension to skip lstat() calls on files\nthat fsmonitor judged as unmodified.  Skip use of the fsmonitor\nextension when called by \"add\", as the format_callback in such cases\nexpects to be called even when the file is believed to be \"up to date\"\nwith the index.\n\nNotably, this change improves performance of the git shell prompt when\nGIT_PS1_SHOWDIRTYSTATE is set.\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 36a09624f..1060bc495 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -395,6 +395,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.626.gc4617b774\n\n"},{"id":"335012","messageId":"0e1b58795c3cfe2a6a64ae8ab0f87f46e5716da4.1513642743.git.alexmv@dropbox.com","threadId":"47463","inReplyTo":"20171219002858.22214-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":"2017-12-19T00:28:57Z","receivedAt":"2017-12-19T00:29:57Z","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.626.gc4617b774\n\n"},{"id":"335038","messageId":"xmqqtvwmv5fl.fsf@gitster.mtv.corp.google.com","threadId":"47463","inReplyTo":"c8cf261d9d620d8123e8bfa5aa952fa55685a8db.1513642743.git.alexmv@dropbox.com","subject":"Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-19T20:17:02Z","receivedAt":"2017-12-19T20:17:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Vandiver <alexmv@dropbox.com> writes:\n\n> Subject: Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path\n\nPerhaps\n\n\"Subject: fsmonitor.h: include dir.h\"\n\nBut I am not sure if this is a right direction to go in.  If a .C\nuser of fsmonitor needs (does not need) things from dir.h, that file\ncan (does not need to) include dir.h itself.\n\nI think this header does excessive \"static inline\" as premature\noptimization, so a better \"fix\" to your perceived problem may be to\nmake them not \"static inline\".\n\n> This missing include is currently hidden by dint of the fact that\n> dir.h is already included by all things that currently include\n> fsmonitor.h\n>\n> Signed-off-by: Alex Vandiver <alexmv@dropbox.com>\n> ---\n\n\n>  fsmonitor.h | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/fsmonitor.h b/fsmonitor.h\n> index cd3cc0ccf..5f68ca4d2 100644\n> --- a/fsmonitor.h\n> +++ b/fsmonitor.h\n> @@ -1,5 +1,6 @@\n>  #ifndef FSMONITOR_H\n>  #define FSMONITOR_H\n> +#include \"dir.h\"\n>  \n>  extern struct trace_key trace_fsmonitor;\n"},{"id":"335039","messageId":"xmqqpo7av5au.fsf@gitster.mtv.corp.google.com","threadId":"47463","inReplyTo":"0e1b58795c3cfe2a6a64ae8ab0f87f46e5716da4.1513642743.git.alexmv@dropbox.com","subject":"Re: [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-19T20:19:53Z","receivedAt":"2017-12-19T20:20:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Vandiver <alexmv@dropbox.com> writes:\n\n> These were mistakenly left in when the test was introduced, in\n> 1487372d3 (\"fsmonitor: store fsmonitor bitmap before splitting index\",\n> 2017-11-09)\n>\n> Signed-off-by: Alex Vandiver <alexmv@dropbox.com>\n> ---\n>  t/t7519-status-fsmonitor.sh | 2 --\n>  1 file changed, 2 deletions(-)\n>\n> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> index 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\nHmph, by default the standard output and standard error streams are\nnot shown in the test output, and it would help while debugging test\nfailures, so I am not sure if this is a good change.  With the\nprevious step [4/6], we can lose the \"echo\", of course, and I do not\nthink we need >&2 redirection there, either.\n"},{"id":"335040","messageId":"xmqqlghyv4wz.fsf@gitster.mtv.corp.google.com","threadId":"47463","inReplyTo":"dab89f071d22a21b85dff5d31e9e9a8ceb6458e3.1513642743.git.alexmv@dropbox.com","subject":"Re: [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-19T20:28:12Z","receivedAt":"2017-12-19T20:28:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Vandiver <alexmv@dropbox.com> writes:\n\n> Subject: Re: [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor\n\n\"Subject: fsmonitor: complete the last line of test-dump-fsmonitor output\"\n\nperhaps.\n\n> This makes it more readable when used for debugging from the\n> commandline.\n>\n> Signed-off-by: Alex Vandiver <alexmv@dropbox.com>\n> ---\n>  t/helper/test-dump-fsmonitor.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\n> index 53b19b39b..4e56929f7 100644\n> --- a/t/helper/test-dump-fsmonitor.c\n> +++ b/t/helper/test-dump-fsmonitor.c\n> @@ -19,5 +19,6 @@ int cmd_main(int ac, const char **av)\n>  \tfor (i = 0; i < istate->cache_nr; i++)\n>  \t\tprintf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? \"+\" : \"-\");\n>  \n> +\tprintf(\"\\n\");\n\nThat (and existing) uses of printf() all feel a bit overkill ;-)\nPerhaps putchar() would suffice.\n\nI am not sure if the above wants to become something like\n\n\tfor (i = 0; i < istate->cache_nr; i++) {\n        \tputchar(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID ? '+' : '-');\n\t\tquote_c_style(istate->cache[i]->name, NULL, stdout, 0);\n\t\tputchar('\\n');\n\t}\n\ninstead of \"a single long incomplete line\" in the first place.  Your\n\"fix\" merely turns it into \"a single long complete line\", which does\nnot quite feel big enough an improvement, at least to me.\n\n>  \treturn 0;\n>  }\n"},{"id":"335091","messageId":"alpine.DEB.2.21.1.1712201833420.406@MININT-6BKU6QN.europe.corp.microsoft.com","threadId":"47463","inReplyTo":"xmqqpo7av5au.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-20T17:35:36Z","receivedAt":"2017-12-20T17:35:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 19 Dec 2017, Junio C Hamano wrote:\n\n> Alex Vandiver <alexmv@dropbox.com> writes:\n> \n> > These were mistakenly left in when the test was introduced, in\n> > 1487372d3 (\"fsmonitor: store fsmonitor bitmap before splitting index\",\n> > 2017-11-09)\n> >\n> > Signed-off-by: Alex Vandiver <alexmv@dropbox.com>\n> > ---\n> >  t/t7519-status-fsmonitor.sh | 2 --\n> >  1 file changed, 2 deletions(-)\n> >\n> > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> > index 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> \n> Hmph, by default the standard output and standard error streams are\n> not shown in the test output, and it would help while debugging test\n> failures, so I am not sure if this is a good change.  With the\n> previous step [4/6], we can lose the \"echo\", of course, and I do not\n> think we need >&2 redirection there, either.\n\nI think you got it backwards. Sure, this helps debugging, but it hurts\nruntime of the entire test suite (which I might have happened to mention a\ncouple of times takes way too long on Windows, thanks to our choice of\ntest \"framework\").\n\nAnd in the bigger picture, I think that it is very, very easy to insert\nthose debugging statements when something breaks (we have to do that with\nother tests, anyways).\n\nSo I am in favor of this patch, and disagree with your assessment, Junio.\n\nCiao,\nDscho\n"},{"id":"335106","messageId":"alpine.DEB.2.10.1712201108190.10810@alexmv-linux","threadId":"47463","inReplyTo":"xmqqtvwmv5fl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-12-20T20:59:31Z","receivedAt":"2017-12-20T20:59:50Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"On Tue, 19 Dec 2017, Junio C Hamano wrote:\n> Alex Vandiver <alexmv@dropbox.com> writes:\n> \n> > Subject: Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path\n> \n> Perhaps\n> \n> \"Subject: fsmonitor.h: include dir.h\"\n\nCertainly more concise.\n\n> But I am not sure if this is a right direction to go in.  If a .C\n> user of fsmonitor needs (does not need) things from dir.h, that file\n> can (does not need to) include dir.h itself.\n\nHm; I was patterning based on existing .h files, which don't seem shy\nabout pulling in other .h files.\n\n> I think this header does excessive \"static inline\" as premature\n> optimization, so a better \"fix\" to your perceived problem may be to\n> make them not \"static inline\".\n\nYeah, quite possibly.  Ben, do you recall your rationale for inlining\nthem in 6a6da08f6 (\"fsmonitor: teach git to optionally utilize a file\nsystem monitor to speed up detecting new or changed files.\",\n2017-09-22) ?\n\n - Alex\n"},{"id":"335107","messageId":"alpine.DEB.2.10.1712201259520.10810@alexmv-linux","threadId":"47463","inReplyTo":"e3246bad10b891d8e3f751b6ed368a9e3f37c425.1513642743.git.alexmv@dropbox.com","subject":"Re: [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-12-20T21:12:56Z","receivedAt":"2017-12-20T21:13:17Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"On Mon, 18 Dec 2017, Alex Vandiver wrote:\n> dd9005a0a (\"fsmonitor: delay updating state until after split index is\n> merged\", 2017-10-27) began deferring the setting of the\n> CE_FSMONITOR_VALID flag until later, such that do_read_index() does\n> not perform those steps.  This means that t/helper/test-dump-fsmonitor\n> showed all bits as always unset.\n\nThis isn't the right fix, actually.  With split indexes, this puts us\nright back into \"only shows a few bits\" territory, because\ndo_read_index doesn't know about split indexes.\n\nWhich means we need a way to do the whole index load but _not_\nadd/remove the fsmonitor cache, even if the config says to do so.\n\nThe best I'm coming up with is the below -- but I'm not happy with\nit, because 'keep' is meaningless as a configuration value outside of\ntesting, since it's normally treated as an executable path.  This uses\nthe fact that fsmonitor.c currently has a:\n\n        switch (fsmonitor_enabled) {\n        case -1: /* keep: do nothing */\n                break;\n\n...despite get_config_get_fsmonitor() havong no way to return -1 at\npresent.\n\nIs this sort of testing generally done via environment variables,\nrather than magic config values?\n - Alex\n\n---------------------8<----------------\ndiff --git a/config.c b/config.c\nindex 6fb06c213..75fcf1a52 100644\n--- a/config.c\n+++ b/config.c\n@@ -2164,8 +2164,13 @@ int git_config_get_fsmonitor(void)\n        if (core_fsmonitor && !*core_fsmonitor)\n                core_fsmonitor = NULL;\n\n-       if (core_fsmonitor)\n-               return 1;\n+\n+       if (core_fsmonitor) {\n+               if (!strcasecmp(core_fsmonitor, \"keep\"))\n+                       return -1;\n+               else\n+                       return 1;\n+       }\n\n        return 0;\n }\ndiff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\nindex ad452707e..12e131530 100644\n--- a/t/helper/test-dump-fsmonitor.c\n+++ b/t/helper/test-dump-fsmonitor.c\n@@ -5,8 +5,9 @@ int cmd_main(int ac, const char **av)\n        struct index_state *istate = &the_index;\n        int i;\n\n+       git_config_push_parameter(\"core.fsmonitor=keep\");\n        setup_git_directory();\n-       if (do_read_index(istate, get_index_file(), 0) < 0)\n+       if (read_index_from(istate, get_index_file()) < 0)\n                die(\"unable to read index file\");\n        if (!istate->fsmonitor_last_update) {\n                printf(\"no fsmonitor\\n\");\n-----------------8<---------------------\n"},{"id":"335117","messageId":"alpine.DEB.2.10.1712201744270.15915@alexmv-linux","threadId":"47463","inReplyTo":"xmqqlghyv4wz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-12-21T01:55:09Z","receivedAt":"2017-12-21T01:55:30Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"\nOn Tue, 19 Dec 2017, Junio C Hamano wrote:\n> That (and existing) uses of printf() all feel a bit overkill ;-)\n> Perhaps putchar() would suffice.\n> \n> I am not sure if the above wants to become something like\n> \n> \tfor (i = 0; i < istate->cache_nr; i++) {\n>         \tputchar(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID ? '+' : '-');\n> \t\tquote_c_style(istate->cache[i]->name, NULL, stdout, 0);\n> \t\tputchar('\\n');\n> \t}\n> \n> instead of \"a single long incomplete line\" in the first place.  Your\n> \"fix\" merely turns it into \"a single long complete line\", which does\n> not quite feel big enough an improvement, at least to me.\n\nThe more user-digestable form like you describe already exists by way\nof `git ls-files -f`.  I am not sure it is worth replicating it.\n\nThe only current uses of this tool are in tests, which only examine\nthe first (\"no fsmonitor\" / \"fsmonitor last update ...\") line.  I find\nit useful as a brief summary view of the fsmonitor bits, but I suppose\nI'd also be happy with just presence/absence and a count of set/unset\nbits.\n\nBarring objections from Dscho or Ben, I'll reroll with a version that\nshows something like:\n\n    fsmonitor last update 1513821151547101894 (5 seconds ago)\n    5 files valid / 10 files invalid\n\n - Alex\n"},{"id":"335154","messageId":"xmqqfu83u522.fsf@gitster.mtv.corp.google.com","threadId":"47463","inReplyTo":"alpine.DEB.2.10.1712201108190.10810@alexmv-linux","subject":"Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-21T21:47:17Z","receivedAt":"2017-12-21T21:47:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Vandiver <alexmv@dropbox.com> writes:\n\n>> But I am not sure if this is a right direction to go in.  If a .C\n>> user of fsmonitor needs (does not need) things from dir.h, that file\n>> can (does not need to) include dir.h itself.\n>\n> Hm; I was patterning based on existing .h files, which don't seem shy\n> about pulling in other .h files.\n\nIIUC, existing X.h do pull in Y.h when X.h uses a structure or a\ntypedef defined in Y.h (but using pointer to such a structure or a\ntype does not count) defined in Y.h; in such a case, a user of X.h\nthat wants to use what is defined in X.h would not be able to use\nit without somehow knowing the shape of such a structure or type and\nwould be forced to pull in Y.h itself.  \"static inline\" falls into\nthe same category---as it stands, anybody that includes fsmonitor.h\nand wants to use one of these static inline functions would need to\nhave definitions from dir.h, which I agree is wrong and I understand\nthat you want to include dir.h there.\n\n"},{"id":"335155","messageId":"xmqqbmiru4xn.fsf@gitster.mtv.corp.google.com","threadId":"47463","inReplyTo":"alpine.DEB.2.10.1712201744270.15915@alexmv-linux","subject":"Re: [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-21T21:49:56Z","receivedAt":"2017-12-21T21:50:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Vandiver <alexmv@dropbox.com> writes:\n\n> The only current uses of this tool are in tests, which only examine\n> the first (\"no fsmonitor\" / \"fsmonitor last update ...\") line.  I find\n> it useful as a brief summary view of the fsmonitor bits, but I suppose\n> I'd also be happy with just presence/absence and a count of set/unset\n> bits.\n>\n> Barring objections from Dscho or Ben, I'll reroll with a version that\n> shows something like:\n>\n>     fsmonitor last update 1513821151547101894 (5 seconds ago)\n>     5 files valid / 10 files invalid\n\nAs I agree that this is test/debug only, I do not care too deeply\neither way, as long as it does not end with an incomplete line ;-)\n"}]}