threads / patch / 57964

patchfsmonitor: query watchman with right valid json

Subject: [PATCH] fsmonitor: query watchman with right valid json

## tl;dr

6 messages between Jun 7, 2022 and Jun 7, 2022. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Son Luong Ngoc· Jun 7, 2022, 07:54 UTC · lore

In rare circumstances where the current git index does not carry the last_update_token, the fsmonitor v2 hook will be invoked with an empty string which would caused the final rendered json to be invalid.

  ["query", "/path/to/my/git/repository/", {
          "since": ,
          "fields": ["name"],
          "expression": ["not", ["dirname", ".git"]]
  }]
Which will left user with the following error message
  > git status
  failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','
  Watchman: command returned no output.
  Falling back to scanning...
Hide the "since" field in json query when "last_update_token" is empty.
Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
---
 templates/hooks--fsmonitor-watchman.sample | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
Show changes to templates/hooks--fsmonitor-watchman.sample +13 −8
diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample
index 14ed0aa42d..b4ee86dfc4 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -79,6 +79,12 @@ sub watchman_query {
 	or die "open2() failed: $!\n" .
 	"Falling back to scanning...\n";
 
+	my $query = <<"	END";
+		["query", "$git_work_tree", {
+			"fields": ["name"],
+			"expression": ["not", ["dirname", ".git"]]
+		}]
+	END
 	# In the query expression below we're asking for names of files that
 	# changed since $last_update_token but not from the .git folder.
 	#
@@ -87,15 +93,14 @@ sub watchman_query {
 	# output to file names only. Then we're using the "expression" term to
 	# further constrain the results.
 	if (substr($last_update_token, 0, 1) eq "c") {
-		$last_update_token = "\"$last_update_token\"";
+		$query = <<"		END";
+			["query", "$git_work_tree", {
+				"since": "$last_update_token",
+				"fields": ["name"],
+				"expression": ["not", ["dirname", ".git"]]
+			}]
+		END
 	}
-	my $query = <<"	END";
-		["query", "$git_work_tree", {
-			"since": $last_update_token,
-			"fields": ["name"],
-			"expression": ["not", ["dirname", ".git"]]
-		}]
-	END
 
 	# Uncomment for debugging the watchman query
 	# open (my $fh, ">", ".git/watchman-query.json");

-- 
2.36.1.231.g6924ef9a07
Ævar Arnfjörð Bjarmason· Jun 7, 2022, 08:40 UTC · re: Son Luong Ngoc · lore

Re: [PATCH] fsmonitor: query watchman with right valid json

On Tue, Jun 07 2022, Son Luong Ngoc wrote:
Show 38 quoted lines
> In rare circumstances where the current git index does not carry the
> last_update_token, the fsmonitor v2 hook will be invoked with an
> empty string which would caused the final rendered json to be invalid.
>
>   ["query", "/path/to/my/git/repository/", {
>           "since": ,
>           "fields": ["name"],
>           "expression": ["not", ["dirname", ".git"]]
>   }]
>
> Which will left user with the following error message
>
>   > git status
>   failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','
>   Watchman: command returned no output.
>   Falling back to scanning...
>
> Hide the "since" field in json query when "last_update_token" is empty.
>
> Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
> ---
>  templates/hooks--fsmonitor-watchman.sample | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample
> index 14ed0aa42d..b4ee86dfc4 100755
> --- a/templates/hooks--fsmonitor-watchman.sample
> +++ b/templates/hooks--fsmonitor-watchman.sample
> @@ -79,6 +79,12 @@ sub watchman_query {
>  	or die "open2() failed: $!\n" .
>  	"Falling back to scanning...\n";
>  
> +	my $query = <<"	END";
> +		["query", "$git_work_tree", {
> +			"fields": ["name"],
> +			"expression": ["not", ["dirname", ".git"]]
> +		}]
> +	END
Wouldn't a more minimal & obvious patch here be....
Show 8 quoted lines
>  	# In the query expression below we're asking for names of files that
>  	# changed since $last_update_token but not from the .git folder.
>  	#
> @@ -87,15 +93,14 @@ sub watchman_query {
>  	# output to file names only. Then we're using the "expression" term to
>  	# further constrain the results.
>  	if (substr($last_update_token, 0, 1) eq "c") {
> -		$last_update_token = "\"$last_update_token\"";
To just change this to be:
	# same as now:
	$last_update_token = "\"$last_update_token\"";
        $last_update_line = qq["since": $last_update_token,];
Of course having declared the new $last_update_line variable earlier, then:
Show 11 quoted lines
> +		$query = <<"		END";
> +			["query", "$git_work_tree", {
> +				"since": "$last_update_token",
> +				"fields": ["name"],
> +				"expression": ["not", ["dirname", ".git"]]
> +			}]
> +		END
>  	}
> -	my $query = <<"	END";
> -		["query", "$git_work_tree", {
> -			"since": $last_update_token,
Just change this line to:
	$last_update_line

I.e. you don't need to duplicate the whole query just to omit/include a single line in it, or am I missing something?

(This suggestion *would* include a redundant line, but I'm assuming JSON/watchman deals with that just fine...).

Son Luong Ngoc· Jun 7, 2022, 10:56 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] fsmonitor: query watchman with right valid json

Hi Ævar,

On Tue, Jun 7, 2022 at 10:42 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 62 quoted lines
>
>
> On Tue, Jun 07 2022, Son Luong Ngoc wrote:
>
> > In rare circumstances where the current git index does not carry the
> > last_update_token, the fsmonitor v2 hook will be invoked with an
> > empty string which would caused the final rendered json to be invalid.
> >
> >   ["query", "/path/to/my/git/repository/", {
> >           "since": ,
> >           "fields": ["name"],
> >           "expression": ["not", ["dirname", ".git"]]
> >   }]
> >
> > Which will left user with the following error message
> >
> >   > git status
> >   failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','
> >   Watchman: command returned no output.
> >   Falling back to scanning...
> >
> > Hide the "since" field in json query when "last_update_token" is empty.
> >
> > Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
> > ---
> >  templates/hooks--fsmonitor-watchman.sample | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample
> > index 14ed0aa42d..b4ee86dfc4 100755
> > --- a/templates/hooks--fsmonitor-watchman.sample
> > +++ b/templates/hooks--fsmonitor-watchman.sample
> > @@ -79,6 +79,12 @@ sub watchman_query {
> >       or die "open2() failed: $!\n" .
> >       "Falling back to scanning...\n";
> >
> > +     my $query = <<" END";
> > +             ["query", "$git_work_tree", {
> > +                     "fields": ["name"],
> > +                     "expression": ["not", ["dirname", ".git"]]
> > +             }]
> > +     END
>
> Wouldn't a more minimal & obvious patch here be....
>
> >       # In the query expression below we're asking for names of files that
> >       # changed since $last_update_token but not from the .git folder.
> >       #
> > @@ -87,15 +93,14 @@ sub watchman_query {
> >       # output to file names only. Then we're using the "expression" term to
> >       # further constrain the results.
> >       if (substr($last_update_token, 0, 1) eq "c") {
> > -             $last_update_token = "\"$last_update_token\"";
>
> To just change this to be:
>
>         # same as now:
>         $last_update_token = "\"$last_update_token\"";
>         $last_update_line = qq["since": $last_update_token,];
>
> Of course having declared the new $last_update_line variable earlier, then:
>

Yup, I think this is a sensible suggestion. I will fixup and send a V2 shortly.

Show 21 quoted lines
> > +             $query = <<"            END";
> > +                     ["query", "$git_work_tree", {
> > +                             "since": "$last_update_token",
> > +                             "fields": ["name"],
> > +                             "expression": ["not", ["dirname", ".git"]]
> > +                     }]
> > +             END
> >       }
> > -     my $query = <<" END";
> > -             ["query", "$git_work_tree", {
> > -                     "since": $last_update_token,
>
> Just change this line to:
>
>         $last_update_line
>
> I.e. you don't need to duplicate the whole query just to omit/include a
> single line in it, or am I missing something?
>
> (This suggestion *would* include a redundant line, but I'm assuming
> JSON/watchman deals with that just fine...).

I think we can remove that redundant line by adding '\n' before $last_update_line. I will be including this into the next version.

Thanks, Son Luong.

Son Luong Ngoc· Jun 7, 2022, 11:14 UTC · re: Son Luong Ngoc · lore

[PATCH v2] fsmonitor: query watchman with right valid json

In rare circumstances where the current git index does not carry the last_update_token, the fsmonitor v2 hook will be invoked with an empty string which would caused the final rendered json to be invalid.

  ["query", "/path/to/my/git/repository/", {
          "since": ,
          "fields": ["name"],
          "expression": ["not", ["dirname", ".git"]]
  }]
Which will left user with the following error message
  > git status
  failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','
  Watchman: command returned no output.
  Falling back to scanning...
Hide the "since" field in json query when "last_update_token" is empty.
Co-authored-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
---
 templates/hooks--fsmonitor-watchman.sample | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to templates/hooks--fsmonitor-watchman.sample +3 −2
diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample
index 14ed0aa42d..23e856f5de 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -86,12 +86,13 @@ sub watchman_query {
 	# recency index to select candidate nodes and "fields" to limit the
 	# output to file names only. Then we're using the "expression" term to
 	# further constrain the results.
+	my $last_update_line = "";
 	if (substr($last_update_token, 0, 1) eq "c") {
 		$last_update_token = "\"$last_update_token\"";
+		$last_update_line = qq[\n"since": $last_update_token,];
 	}
 	my $query = <<"	END";
-		["query", "$git_work_tree", {
-			"since": $last_update_token,
+		["query", "$git_work_tree", {$last_update_line
 			"fields": ["name"],
 			"expression": ["not", ["dirname", ".git"]]
 		}]

-- 
2.36.1.476.g0c4daa206d
Ævar Arnfjörð Bjarmason· Jun 7, 2022, 14:39 UTC · re: Son Luong Ngoc · lore

Re: [PATCH v2] fsmonitor: query watchman with right valid json

On Tue, Jun 07 2022, Son Luong Ngoc wrote:
Show 21 quoted lines
> In rare circumstances where the current git index does not carry the
> last_update_token, the fsmonitor v2 hook will be invoked with an
> empty string which would caused the final rendered json to be invalid.
>
>   ["query", "/path/to/my/git/repository/", {
>           "since": ,
>           "fields": ["name"],
>           "expression": ["not", ["dirname", ".git"]]
>   }]
>
> Which will left user with the following error message
>
>   > git status
>   failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','
>   Watchman: command returned no output.
>   Falling back to scanning...
>
> Hide the "since" field in json query when "last_update_token" is empty.
>
> Co-authored-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
Thanks for the quick turnaround.
Show 16 quoted lines
>  templates/hooks--fsmonitor-watchman.sample | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample
> index 14ed0aa42d..23e856f5de 100755
> --- a/templates/hooks--fsmonitor-watchman.sample
> +++ b/templates/hooks--fsmonitor-watchman.sample
> @@ -86,12 +86,13 @@ sub watchman_query {
>  	# recency index to select candidate nodes and "fields" to limit the
>  	# output to file names only. Then we're using the "expression" term to
>  	# further constrain the results.
> +	my $last_update_line = "";
>  	if (substr($last_update_token, 0, 1) eq "c") {
>  		$last_update_token = "\"$last_update_token\"";
> +		$last_update_line = qq[\n"since": $last_update_token,];
>  	}
This LGTM, just a note...
>  	my $query = <<"	END";
> -		["query", "$git_work_tree", {
> -			"since": $last_update_token,
> +		["query", "$git_work_tree", {$last_update_line

...doesn't really need a re-roll, but doesn't this trade "we don't have too many \n" for not indenting the query properly anymore?

I think skipping both is fine, but between the two I think having indenting is better than having a redundant \n some of the time.

FWIW you could just add the variable on its own line, and then do this instead:

	(my $query = <<"        END") =~ s/(?<=\n)\t*\n//s; 
To post-hoc fix the extra \n in this case :)
But I think this is also fine as-is, thanks!
Junio C Hamano· Jun 7, 2022, 17:00 UTC · re: Son Luong Ngoc · lore

Re: [PATCH v2] fsmonitor: query watchman with right valid json

Son Luong Ngoc <sluongng@gmail.com> writes:
Show 11 quoted lines
> In rare circumstances where the current git index does not carry the
> last_update_token, the fsmonitor v2 hook will be invoked with an
> empty string which would caused the final rendered json to be invalid.
>
>   ["query", "/path/to/my/git/repository/", {
>           "since": ,
>           "fields": ["name"],
>           "expression": ["not", ["dirname", ".git"]]
>   }]
>
> Which will left user with the following error message
"left" -> "leave" (or "give")
Show 8 quoted lines
>   > git status
>   failed to parse command from stdin: line 2, column 13, position 67: unexpected token near ','
>   Watchman: command returned no output.
>   Falling back to scanning...
>
> Hide the "since" field in json query when "last_update_token" is empty.
>
> Co-authored-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
It looked more like Helped-by to me, but I dunno.
Show 25 quoted lines
> Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
> ---
>  templates/hooks--fsmonitor-watchman.sample | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample
> index 14ed0aa42d..23e856f5de 100755
> --- a/templates/hooks--fsmonitor-watchman.sample
> +++ b/templates/hooks--fsmonitor-watchman.sample
> @@ -86,12 +86,13 @@ sub watchman_query {
>  	# recency index to select candidate nodes and "fields" to limit the
>  	# output to file names only. Then we're using the "expression" term to
>  	# further constrain the results.
> +	my $last_update_line = "";
>  	if (substr($last_update_token, 0, 1) eq "c") {
>  		$last_update_token = "\"$last_update_token\"";
> +		$last_update_line = qq[\n"since": $last_update_token,];
>  	}
>  	my $query = <<"	END";
> -		["query", "$git_work_tree", {
> -			"since": $last_update_token,
> +		["query", "$git_work_tree", {$last_update_line
>  			"fields": ["name"],
>  			"expression": ["not", ["dirname", ".git"]]
>  		}]
OK.  Compared to v1, this looks much more reasonable.

This is totally unrelated to the "hide invalid since" topic, but I wonder if $git_work_tree needs a bit more careful quoting. It comes directly from get_working_dir() but can it contain say a double quote character, to make the resulting string in the variable $query not quite well formed?

← back to recent threads