Re: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor extension
Ben Peart wrote:
> 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?
Agreed. Care to write patches for these? :) (I think three patches,
one for each issue, would do the trick.)
Thanks,
Jonathan
Re: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor extension
On 9/19/2017 3:32 PM, David Turner wrote:
I think my comment here might have gotten lost, and I don't want it to because
it's something I'm really worried about:
You have to update the test completely to ensure it passes. If you run
the test with the '-v' option you will see the cause of the failure:
t7519-status-fsmonitor.sh: line 27: dir3/untracked: No such file or
directory
To fix this, you will also need to update the 'setup' test to create the
directory for the new untracked file to get created into. Then you will
need to drop at least one file in it so that the directory is preserved
through the 'git reset --hard' Then you have to update the several 'cat
>expect' blocks to expect the new file.
In addition, the ability to avoid scanning for untracked files relies on
the untracked cache. If you don't have another file that git is aware
of in that directory then there won't be a cache entry and git will do
the required scan and detect the new untracked file (by design).
Here is a patch to the test that updates it to meet all these
requirements. I hope this helps.
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index c6df85af5e..29ae4e284f 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -24,12 +24,14 @@ dirty_repo () {
: >untracked &&
: >dir1/untracked &&
: >dir2/untracked &&
+ : >dir3/untracked &&
echo 1 >modified &&
echo 2 >dir1/modified &&
echo 3 >dir2/modified &&
echo 4 >new &&
echo 5 >dir1/new &&
echo 6 >dir2/new
+ echo 7 >dir3/new
}
write_integration_script () {
@@ -47,12 +49,14 @@ write_integration_script () {
printf "untracked\0"
printf "dir1/untracked\0"
printf "dir2/untracked\0"
+ printf "dir3/untracked\0"
printf "modified\0"
printf "dir1/modified\0"
printf "dir2/modified\0"
printf "new\0"
printf "dir1/new\0"
printf "dir2/new\0"
+ printf "dir3/new\0"
EOF
}
@@ -71,6 +75,8 @@ test_expect_success 'setup' '
mkdir dir2 &&
: >dir2/tracked &&
: >dir2/modified &&
+ mkdir dir3 &&
+ : >dir3/tracked &&
git -c core.fsmonitor= add . &&
git -c core.fsmonitor= commit -m initial &&
git config core.fsmonitor .git/hooks/fsmonitor-test &&
@@ -107,6 +113,7 @@ h dir1/modified
H dir1/tracked
h dir2/modified
H dir2/tracked
+H dir3/tracked
h modified
H tracked
EOF
@@ -126,6 +133,7 @@ H dir1/modified
H dir1/tracked
H dir2/modified
H dir2/tracked
+H dir3/tracked
H modified
H tracked
EOF
@@ -144,6 +152,7 @@ H dir1/modified
H dir1/tracked
H dir2/modified
H dir2/tracked
+H dir3/tracked
H modified
H tracked
EOF
@@ -164,6 +173,8 @@ H dir1/tracked
H dir2/modified
h dir2/new
H dir2/tracked
+h dir3/new
+H dir3/tracked
H modified
h new
H tracked
@@ -174,6 +185,7 @@ test_expect_success 'newly added files are marked
valid' '
git add new &&
git add dir1/new &&
git add dir2/new &&
+ git add dir3/new &&
git ls-files -f >actual &&
test_cmp expect actual
'
@@ -185,6 +197,8 @@ h dir1/tracked
H dir2/modified
h dir2/new
h dir2/tracked
+h dir3/new
+h dir3/tracked
H modified
h new
h tracked
@@ -203,6 +217,7 @@ H dir1/modified
h dir1/tracked
h dir2/modified
h dir2/tracked
+h dir3/tracked
h modified
h tracked
EOF
@@ -269,6 +284,7 @@ do
git add new &&
git add dir1/new &&
git add dir2/new &&
+ git add dir3/new &&
git status >actual &&
git -c core.fsmonitor= status >expect &&
test_i18ncmp expect actual
-Original Message-
From: David Turner
Sent: Friday, September 15, 2017 6:00 PM
To: 'Ben Peart'
Cc: ava...@gmail.com; christian.cou...@gmail.com; git@vger.kernel.org;
gits...@pobox.com; johannes.schinde...@gmx.de; pclo...@gmail.com;
p...@peff.net
Subject: RE: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor
extension
-Original Message-
+dirty_repo () {
+ : >untracked &&
+ : >dir1/untracked &&
+ : >dir2/untracked &&
+ echo 1 >modified &&
+ echo 2 >dir1/modified &&
+ echo 3 >dir2/modified &&
+ echo 4 >new &&
+ echo 5 >dir1/new &&
+ echo 6 >dir2/new
If I add an untracked file named dir3/untracked to dirty_repo (and
write_integration_script), then "status doesn't detect unreported
modifications", below, fails. Did I do something wrong, or does this turn up a
bug?
RE: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor extension
I think my comment here might have gotten lost, and I don't want it to because
it's something I'm really worried about:
> -Original Message-
> From: David Turner
> Sent: Friday, September 15, 2017 6:00 PM
> To: 'Ben Peart'
> Cc: ava...@gmail.com; christian.cou...@gmail.com; git@vger.kernel.org;
> gits...@pobox.com; johannes.schinde...@gmx.de; pclo...@gmail.com;
> p...@peff.net
> Subject: RE: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor
> extension
>
> > -Original Message-
> > +dirty_repo () {
> > + : >untracked &&
> > + : >dir1/untracked &&
> > + : >dir2/untracked &&
> > + echo 1 >modified &&
> > + echo 2 >dir1/modified &&
> > + echo 3 >dir2/modified &&
> > + echo 4 >new &&
> > + echo 5 >dir1/new &&
> > + echo 6 >dir2/new
>
> If I add an untracked file named dir3/untracked to dirty_repo (and
> write_integration_script), then "status doesn't detect unreported
> modifications", below, fails. Did I do something wrong, or does this turn up
> a
> bug?
Re: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor extension
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 00..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 <
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.
Re: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor extension
On 9/16/2017 11:27 AM, Torsten Bögershausen wrote: On 2017-09-15 21:20, Ben Peart wrote: +if [ "$1" != 1 ] +then + echo -e "Unsupported core.fsmonitor hook version.\n" >&2 + exit 1 +fi The echo -e not portable (It was detected by a tighter version of the lint script, which I have here, but not yet send to the list :-( This will do: echo "Unsupported core.fsmonitor hook version." >&2 Thanks, I'll fix these and the ones in the t/t7519 directory.
Re: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor extension
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:
- 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 00..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 < +}
> \ No newline at end of file
Oops.
Thanks.
Re: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor extension
On 2017-09-15 21:20, Ben Peart wrote: > +if [ "$1" != 1 ] > +then > + echo -e "Unsupported core.fsmonitor hook version.\n" >&2 > + exit 1 > +fi The echo -e not portable (It was detected by a tighter version of the lint script, which I have here, but not yet send to the list :-( This will do: echo "Unsupported core.fsmonitor hook version." >&2
RE: [PATCH v6 10/12] fsmonitor: add test cases for fsmonitor extension
> -Original Message-
> +dirty_repo () {
> + : >untracked &&
> + : >dir1/untracked &&
> + : >dir2/untracked &&
> + echo 1 >modified &&
> + echo 2 >dir1/modified &&
> + echo 3 >dir2/modified &&
> + echo 4 >new &&
> + echo 5 >dir1/new &&
> + echo 6 >dir2/new
If I add an untracked file named dir3/untracked to dirty_repo
(and write_integration_script), then "status doesn't detect
unreported modifications", below, fails. Did I do something
wrong, or does this turn up a bug?
> + test_expect_success "setup preloadIndex to $preload_val" '
> + git config core.preloadIndex $preload_val &&
> + if [ $preload_val -eq true ]
"-eq" is for numeric equality in POSIX shell. So this works if your
/bin/sh is bash but not if it's e.g. dash. This happens twice more
below. Use "=" instead.
