From: Ben Peart Date: Mon, 18 Sep 2017 15:25:17 GMT Subject: Re: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor extension Message-ID: <2f3cdc85-f051-c0ae-b9db-fd13cac78aed@gmail.com> In-Reply-To: On 9/17/2017 12:47 AM, Junio C Hamano wrote: > Ben Peart writes: > >> +write_integration_script() { >> + write_script .git/hooks/fsmonitor-test<<-\EOF >> + if [ "$#" -ne 2 ]; then >> + echo "$0: exactly 2 arguments expected" >> + exit 2 >> + fi >> + if [ "$1" != 1 ]; then >> + echo -e "Unsupported core.fsmonitor hook version.\n" >&2 >> + exit 1 >> + fi > > In addition to "echo -e" thing pointed out earlier, these look > somewhat unusual in our shell scripts, relative to what > Documentation/CodingGuidelines tells us to do: I'm happy to make these changes. I understand the difficulty of creating a consistent coding style especially after the fact. Copy/paste is usually a developers best friend as it allows you to avoid a lot of errors by reusing existing tested code. One of the times it backfires is when that code doesn't match the current desired coding style. I only point these out to help lend some additional impetus to the effort to formalize the coding style and to provide tooling to handle what should mostly be a mechanical process. IMO, the goal should be to save the maintainer and contributors the cost of having to write up and respond to formatting feedback. :) Some stats on these same coding style errors in the current bash scripts: 298 instances of "[a-z]\(\).*\{" ie "function_name() {" (no space) 140 instances of "if \[ .* \]" (not using the preferred "test") 293 instances of "if .*; then" Wouldn't it be great not to have to write up style feedback for when these all get copy/pasted into new scripts? :) > > - We prefer a space between the function name and the parentheses, > and no space inside the parentheses. The opening "{" should also > be on the same line. > > (incorrect) > my_function(){ > ... > > (correct) > my_function () { > ... > > - We prefer "test" over "[ ... ]". > > - Do not write control structures on a single line with semicolon. > "then" should be on the next line for if statements, and "do" > should be on the next line for "while" and "for". > > (incorrect) > if test -f hello; then > do this > fi > > (correct) > if test -f hello > then > do this > fi > >> diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman >> new file mode 100755 >> index 0000000000..aaee5d1fe3 >> --- /dev/null >> +++ b/t/t7519/fsmonitor-watchman >> @@ -0,0 +1,128 @@ >> +#!/usr/bin/perl >> + >> +use strict; >> +use warnings; >> +use IPC::Open2; >> + ... >> + open (my $fh, ">", ".git/watchman-query.json"); >> + print $fh "[\"query\", \"$git_work_tree\", { \ >> + \"since\": $time, \ >> + \"fields\": [\"name\"], \ >> + \"expression\": [\"not\", [\"allof\", [\"since\", $time, \"cclock\"], [\"not\", \"exists\"]]] \ >> + }]"; >> + close $fh; >> + >> + print CHLD_IN "[\"query\", \"$git_work_tree\", { \ >> + \"since\": $time, \ >> + \"fields\": [\"name\"], \ >> + \"expression\": [\"not\", [\"allof\", [\"since\", $time, \"cclock\"], [\"not\", \"exists\"]]] \ >> + }]"; > > This look painful to read, write and maintain. IIRC, Perl supports > the < these "print" we see above easier? > I agree! I'm definitely *not* a perl developer so was unaware of this construct. A few minutes with stack overflow and now I can clean this up. >> +} >> \ No newline at end of file > > Oops. > > Thanks. >