Re: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor extension

From: Ben Peart <peartben@gmail.com>
Date: Sep 18, 2017, 15:25 UTC
Message-ID: <2f3cdc85-f051-c0ae-b9db-fd13cac78aed@gmail.com>
In-Reply-To: <xmqqy3peszho.fsf@gitster.mtv.corp.google.com>

On 9/17/2017 12:47 AM, Junio C Hamano wrote:

Show 16 quoted lines

> Ben Peart <benpeart@microsoft.com> 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.

<soapbox>

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? :)

</soapbox>

Show 60 quoted lines

> 
>   - 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 <<HERE document syntax quite similar to shell; would it make
> 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.

Show 7 quoted lines

>> +}
>> \ No newline at end of file
> 
> Oops.
> 
> Thanks.
>