{"thread":{"id":"57964","subject":"[PATCH] fsmonitor: query watchman with right valid json","startedAt":"2022-06-07T07:55:01Z","lastAt":"2022-06-07T17:00:28Z","messageCount":6,"participants":["Son Luong Ngoc","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"456771","messageId":"20220607075425.39510-1-sluongng@gmail.com","threadId":"57964","inReplyTo":null,"subject":"[PATCH] fsmonitor: query watchman with right valid json","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2022-06-07T07:54:25Z","receivedAt":"2022-06-07T07:55:01Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"In rare circumstances where the current git index does not carry the\nlast_update_token, the fsmonitor v2 hook will be invoked with an\nempty string which would caused the final rendered json to be invalid.\n\n  [\"query\", \"/path/to/my/git/repository/\", {\n          \"since\": ,\n          \"fields\": [\"name\"],\n          \"expression\": [\"not\", [\"dirname\", \".git\"]]\n  }]\n\nWhich will left user with the following error message\n\n  > git status\n  failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','\n  Watchman: command returned no output.\n  Falling back to scanning...\n\nHide the \"since\" field in json query when \"last_update_token\" is empty.\n\nSigned-off-by: Son Luong Ngoc <sluongng@gmail.com>\n---\n templates/hooks--fsmonitor-watchman.sample | 21 +++++++++++++--------\n 1 file changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample\nindex 14ed0aa42d..b4ee86dfc4 100755\n--- a/templates/hooks--fsmonitor-watchman.sample\n+++ b/templates/hooks--fsmonitor-watchman.sample\n@@ -79,6 +79,12 @@ sub watchman_query {\n \tor die \"open2() failed: $!\\n\" .\n \t\"Falling back to scanning...\\n\";\n \n+\tmy $query = <<\"\tEND\";\n+\t\t[\"query\", \"$git_work_tree\", {\n+\t\t\t\"fields\": [\"name\"],\n+\t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n+\t\t}]\n+\tEND\n \t# In the query expression below we're asking for names of files that\n \t# changed since $last_update_token but not from the .git folder.\n \t#\n@@ -87,15 +93,14 @@ sub watchman_query {\n \t# output to file names only. Then we're using the \"expression\" term to\n \t# further constrain the results.\n \tif (substr($last_update_token, 0, 1) eq \"c\") {\n-\t\t$last_update_token = \"\\\"$last_update_token\\\"\";\n+\t\t$query = <<\"\t\tEND\";\n+\t\t\t[\"query\", \"$git_work_tree\", {\n+\t\t\t\t\"since\": \"$last_update_token\",\n+\t\t\t\t\"fields\": [\"name\"],\n+\t\t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n+\t\t\t}]\n+\t\tEND\n \t}\n-\tmy $query = <<\"\tEND\";\n-\t\t[\"query\", \"$git_work_tree\", {\n-\t\t\t\"since\": $last_update_token,\n-\t\t\t\"fields\": [\"name\"],\n-\t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n-\t\t}]\n-\tEND\n \n \t# Uncomment for debugging the watchman query\n \t# open (my $fh, \">\", \".git/watchman-query.json\");\n-- \n2.36.1.231.g6924ef9a07\n\n"},{"id":"456774","messageId":"220607.86ilpcvp89.gmgdl@evledraar.gmail.com","threadId":"57964","inReplyTo":"20220607075425.39510-1-sluongng@gmail.com","subject":"Re: [PATCH] fsmonitor: query watchman with right valid json","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-07T08:40:10Z","receivedAt":"2022-06-07T08:42:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 07 2022, Son Luong Ngoc wrote:\n\n> In rare circumstances where the current git index does not carry the\n> last_update_token, the fsmonitor v2 hook will be invoked with an\n> empty string which would caused the final rendered json to be invalid.\n>\n>   [\"query\", \"/path/to/my/git/repository/\", {\n>           \"since\": ,\n>           \"fields\": [\"name\"],\n>           \"expression\": [\"not\", [\"dirname\", \".git\"]]\n>   }]\n>\n> Which will left user with the following error message\n>\n>   > git status\n>   failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','\n>   Watchman: command returned no output.\n>   Falling back to scanning...\n>\n> Hide the \"since\" field in json query when \"last_update_token\" is empty.\n>\n> Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>\n> ---\n>  templates/hooks--fsmonitor-watchman.sample | 21 +++++++++++++--------\n>  1 file changed, 13 insertions(+), 8 deletions(-)\n>\n> diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample\n> index 14ed0aa42d..b4ee86dfc4 100755\n> --- a/templates/hooks--fsmonitor-watchman.sample\n> +++ b/templates/hooks--fsmonitor-watchman.sample\n> @@ -79,6 +79,12 @@ sub watchman_query {\n>  \tor die \"open2() failed: $!\\n\" .\n>  \t\"Falling back to scanning...\\n\";\n>  \n> +\tmy $query = <<\"\tEND\";\n> +\t\t[\"query\", \"$git_work_tree\", {\n> +\t\t\t\"fields\": [\"name\"],\n> +\t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n> +\t\t}]\n> +\tEND\n\nWouldn't a more minimal & obvious patch here be....\n\n>  \t# In the query expression below we're asking for names of files that\n>  \t# changed since $last_update_token but not from the .git folder.\n>  \t#\n> @@ -87,15 +93,14 @@ sub watchman_query {\n>  \t# output to file names only. Then we're using the \"expression\" term to\n>  \t# further constrain the results.\n>  \tif (substr($last_update_token, 0, 1) eq \"c\") {\n> -\t\t$last_update_token = \"\\\"$last_update_token\\\"\";\n\nTo just change this to be:\n\n\t# same as now:\n\t$last_update_token = \"\\\"$last_update_token\\\"\";\n        $last_update_line = qq[\"since\": $last_update_token,];\n\nOf course having declared the new $last_update_line variable earlier, then:\n\n> +\t\t$query = <<\"\t\tEND\";\n> +\t\t\t[\"query\", \"$git_work_tree\", {\n> +\t\t\t\t\"since\": \"$last_update_token\",\n> +\t\t\t\t\"fields\": [\"name\"],\n> +\t\t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n> +\t\t\t}]\n> +\t\tEND\n>  \t}\n> -\tmy $query = <<\"\tEND\";\n> -\t\t[\"query\", \"$git_work_tree\", {\n> -\t\t\t\"since\": $last_update_token,\n\nJust change this line to:\n\n\t$last_update_line\n\nI.e. you don't need to duplicate the whole query just to omit/include a\nsingle line in it, or am I missing something?\n\n(This suggestion *would* include a redundant line, but I'm assuming\nJSON/watchman deals with that just fine...).\n"},{"id":"456783","messageId":"CAL3xRKev_KHvAFuviG7RxsxA_786K4QY5F08a8D23M9MLM81+g@mail.gmail.com","threadId":"57964","inReplyTo":"220607.86ilpcvp89.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] fsmonitor: query watchman with right valid json","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2022-06-07T10:56:05Z","receivedAt":"2022-06-07T11:01:43Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Hi Ævar,\n\nOn Tue, Jun 7, 2022 at 10:42 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Tue, Jun 07 2022, Son Luong Ngoc wrote:\n>\n> > In rare circumstances where the current git index does not carry the\n> > last_update_token, the fsmonitor v2 hook will be invoked with an\n> > empty string which would caused the final rendered json to be invalid.\n> >\n> >   [\"query\", \"/path/to/my/git/repository/\", {\n> >           \"since\": ,\n> >           \"fields\": [\"name\"],\n> >           \"expression\": [\"not\", [\"dirname\", \".git\"]]\n> >   }]\n> >\n> > Which will left user with the following error message\n> >\n> >   > git status\n> >   failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','\n> >   Watchman: command returned no output.\n> >   Falling back to scanning...\n> >\n> > Hide the \"since\" field in json query when \"last_update_token\" is empty.\n> >\n> > Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>\n> > ---\n> >  templates/hooks--fsmonitor-watchman.sample | 21 +++++++++++++--------\n> >  1 file changed, 13 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample\n> > index 14ed0aa42d..b4ee86dfc4 100755\n> > --- a/templates/hooks--fsmonitor-watchman.sample\n> > +++ b/templates/hooks--fsmonitor-watchman.sample\n> > @@ -79,6 +79,12 @@ sub watchman_query {\n> >       or die \"open2() failed: $!\\n\" .\n> >       \"Falling back to scanning...\\n\";\n> >\n> > +     my $query = <<\" END\";\n> > +             [\"query\", \"$git_work_tree\", {\n> > +                     \"fields\": [\"name\"],\n> > +                     \"expression\": [\"not\", [\"dirname\", \".git\"]]\n> > +             }]\n> > +     END\n>\n> Wouldn't a more minimal & obvious patch here be....\n>\n> >       # In the query expression below we're asking for names of files that\n> >       # changed since $last_update_token but not from the .git folder.\n> >       #\n> > @@ -87,15 +93,14 @@ sub watchman_query {\n> >       # output to file names only. Then we're using the \"expression\" term to\n> >       # further constrain the results.\n> >       if (substr($last_update_token, 0, 1) eq \"c\") {\n> > -             $last_update_token = \"\\\"$last_update_token\\\"\";\n>\n> To just change this to be:\n>\n>         # same as now:\n>         $last_update_token = \"\\\"$last_update_token\\\"\";\n>         $last_update_line = qq[\"since\": $last_update_token,];\n>\n> Of course having declared the new $last_update_line variable earlier, then:\n>\n\nYup, I think this is a sensible suggestion.\nI will fixup and send a V2 shortly.\n\n> > +             $query = <<\"            END\";\n> > +                     [\"query\", \"$git_work_tree\", {\n> > +                             \"since\": \"$last_update_token\",\n> > +                             \"fields\": [\"name\"],\n> > +                             \"expression\": [\"not\", [\"dirname\", \".git\"]]\n> > +                     }]\n> > +             END\n> >       }\n> > -     my $query = <<\" END\";\n> > -             [\"query\", \"$git_work_tree\", {\n> > -                     \"since\": $last_update_token,\n>\n> Just change this line to:\n>\n>         $last_update_line\n>\n> I.e. you don't need to duplicate the whole query just to omit/include a\n> single line in it, or am I missing something?\n>\n> (This suggestion *would* include a redundant line, but I'm assuming\n> JSON/watchman deals with that just fine...).\n\nI think we can remove that redundant line by adding '\\n' before\n$last_update_line.\nI will be including this into the next version.\n\nThanks,\nSon Luong.\n"},{"id":"456784","messageId":"20220607111419.15753-1-sluongng@gmail.com","threadId":"57964","inReplyTo":"CAL3xRKev_KHvAFuviG7RxsxA_786K4QY5F08a8D23M9MLM81+g@mail.gmail.com","subject":"[PATCH v2] fsmonitor: query watchman with right valid json","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2022-06-07T11:14:19Z","receivedAt":"2022-06-07T11:14:47Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"In rare circumstances where the current git index does not carry the\nlast_update_token, the fsmonitor v2 hook will be invoked with an\nempty string which would caused the final rendered json to be invalid.\n\n  [\"query\", \"/path/to/my/git/repository/\", {\n          \"since\": ,\n          \"fields\": [\"name\"],\n          \"expression\": [\"not\", [\"dirname\", \".git\"]]\n  }]\n\nWhich will left user with the following error message\n\n  > git status\n  failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','\n  Watchman: command returned no output.\n  Falling back to scanning...\n\nHide the \"since\" field in json query when \"last_update_token\" is empty.\n\nCo-authored-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Son Luong Ngoc <sluongng@gmail.com>\n---\n templates/hooks--fsmonitor-watchman.sample | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample\nindex 14ed0aa42d..23e856f5de 100755\n--- a/templates/hooks--fsmonitor-watchman.sample\n+++ b/templates/hooks--fsmonitor-watchman.sample\n@@ -86,12 +86,13 @@ sub watchman_query {\n \t# recency index to select candidate nodes and \"fields\" to limit the\n \t# output to file names only. Then we're using the \"expression\" term to\n \t# further constrain the results.\n+\tmy $last_update_line = \"\";\n \tif (substr($last_update_token, 0, 1) eq \"c\") {\n \t\t$last_update_token = \"\\\"$last_update_token\\\"\";\n+\t\t$last_update_line = qq[\\n\"since\": $last_update_token,];\n \t}\n \tmy $query = <<\"\tEND\";\n-\t\t[\"query\", \"$git_work_tree\", {\n-\t\t\t\"since\": $last_update_token,\n+\t\t[\"query\", \"$git_work_tree\", {$last_update_line\n \t\t\t\"fields\": [\"name\"],\n \t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n \t\t}]\n-- \n2.36.1.476.g0c4daa206d\n\n"},{"id":"456787","messageId":"220607.867d5sv8bq.gmgdl@evledraar.gmail.com","threadId":"57964","inReplyTo":"20220607111419.15753-1-sluongng@gmail.com","subject":"Re: [PATCH v2] fsmonitor: query watchman with right valid json","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-07T14:39:58Z","receivedAt":"2022-06-07T14:48:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 07 2022, Son Luong Ngoc wrote:\n\n> In rare circumstances where the current git index does not carry the\n> last_update_token, the fsmonitor v2 hook will be invoked with an\n> empty string which would caused the final rendered json to be invalid.\n>\n>   [\"query\", \"/path/to/my/git/repository/\", {\n>           \"since\": ,\n>           \"fields\": [\"name\"],\n>           \"expression\": [\"not\", [\"dirname\", \".git\"]]\n>   }]\n>\n> Which will left user with the following error message\n>\n>   > git status\n>   failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','\n>   Watchman: command returned no output.\n>   Falling back to scanning...\n>\n> Hide the \"since\" field in json query when \"last_update_token\" is empty.\n>\n> Co-authored-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>\n\nThanks for the quick turnaround.\n\n>  templates/hooks--fsmonitor-watchman.sample | 5 +++--\n>  1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample\n> index 14ed0aa42d..23e856f5de 100755\n> --- a/templates/hooks--fsmonitor-watchman.sample\n> +++ b/templates/hooks--fsmonitor-watchman.sample\n> @@ -86,12 +86,13 @@ sub watchman_query {\n>  \t# recency index to select candidate nodes and \"fields\" to limit the\n>  \t# output to file names only. Then we're using the \"expression\" term to\n>  \t# further constrain the results.\n> +\tmy $last_update_line = \"\";\n>  \tif (substr($last_update_token, 0, 1) eq \"c\") {\n>  \t\t$last_update_token = \"\\\"$last_update_token\\\"\";\n> +\t\t$last_update_line = qq[\\n\"since\": $last_update_token,];\n>  \t}\n\nThis LGTM, just a note...\n\n>  \tmy $query = <<\"\tEND\";\n> -\t\t[\"query\", \"$git_work_tree\", {\n> -\t\t\t\"since\": $last_update_token,\n> +\t\t[\"query\", \"$git_work_tree\", {$last_update_line\n\n...doesn't really need a re-roll, but doesn't this trade \"we don't have\ntoo many \\n\" for not indenting the query properly anymore?\n\nI think skipping both is fine, but between the two I think having\nindenting is better than having a redundant \\n some of the time.\n\nFWIW you could just add the variable on its own line, and then do this\ninstead:\n\n\t(my $query = <<\"        END\") =~ s/(?<=\\n)\\t*\\n//s; \n\nTo post-hoc fix the extra \\n in this case :)\n\nBut I think this is also fine as-is, thanks!\n"},{"id":"456794","messageId":"xmqqmteoz9wd.fsf@gitster.g","threadId":"57964","inReplyTo":"20220607111419.15753-1-sluongng@gmail.com","subject":"Re: [PATCH v2] fsmonitor: query watchman with right valid json","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-07T17:00:18Z","receivedAt":"2022-06-07T17:00:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Son Luong Ngoc <sluongng@gmail.com> writes:\n\n> In rare circumstances where the current git index does not carry the\n> last_update_token, the fsmonitor v2 hook will be invoked with an\n> empty string which would caused the final rendered json to be invalid.\n>\n>   [\"query\", \"/path/to/my/git/repository/\", {\n>           \"since\": ,\n>           \"fields\": [\"name\"],\n>           \"expression\": [\"not\", [\"dirname\", \".git\"]]\n>   }]\n>\n> Which will left user with the following error message\n\n\"left\" -> \"leave\" (or \"give\")\n\n>   > git status\n>   failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','\n>   Watchman: command returned no output.\n>   Falling back to scanning...\n>\n> Hide the \"since\" field in json query when \"last_update_token\" is empty.\n>\n> Co-authored-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nIt looked more like Helped-by to me, but I dunno.\n\n> Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>\n> ---\n>  templates/hooks--fsmonitor-watchman.sample | 5 +++--\n>  1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample\n> index 14ed0aa42d..23e856f5de 100755\n> --- a/templates/hooks--fsmonitor-watchman.sample\n> +++ b/templates/hooks--fsmonitor-watchman.sample\n> @@ -86,12 +86,13 @@ sub watchman_query {\n>  \t# recency index to select candidate nodes and \"fields\" to limit the\n>  \t# output to file names only. Then we're using the \"expression\" term to\n>  \t# further constrain the results.\n> +\tmy $last_update_line = \"\";\n>  \tif (substr($last_update_token, 0, 1) eq \"c\") {\n>  \t\t$last_update_token = \"\\\"$last_update_token\\\"\";\n> +\t\t$last_update_line = qq[\\n\"since\": $last_update_token,];\n>  \t}\n>  \tmy $query = <<\"\tEND\";\n> -\t\t[\"query\", \"$git_work_tree\", {\n> -\t\t\t\"since\": $last_update_token,\n> +\t\t[\"query\", \"$git_work_tree\", {$last_update_line\n>  \t\t\t\"fields\": [\"name\"],\n>  \t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n>  \t\t}]\n\nOK.  Compared to v1, this looks much more reasonable.\n\nThis is totally unrelated to the \"hide invalid since\" topic, but I\nwonder if $git_work_tree needs a bit more careful quoting.  It comes\ndirectly from get_working_dir() but can it contain say a double quote\ncharacter, to make the resulting string in the variable $query not\nquite well formed?\n"}]}