Matthew DeVore wrote:
> Subject: t/*: fix pipe placement and remove \'s
>
> Where ever there was code in the tests like this:
>
> foo \
> | bar
Language nits:
- s/Where ever/Wherever/
- Git's commit messages use the present tense to describe the existing
previous state of the codebase, as though reporting a bug.
Maybe something like
tests: standardize pipe placement
Instead of using a line-continuation and pipe on the second
line, take advantage of the shell's implicit line continuation
after a pipe character. So for example, instead of
some long line \
| next line
use
some long line |
next line
At this point, it would be useful to say something about rationale ---
for example,
This better matches the coding style documented in
Documentation/CodingGuidelines and used in shell scripts
elsewhere in Git.
Except: is this documented in Documentation/CodingGuidelines? Or,
better, is there a linter that we can run in the test-lint target of
t/Makefile to ensure we keep sticking to this style?
[...]
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -57,8 +57,8 @@ then
> echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
> --passphrase-fd 0 --pinentry-mode loopback \
> --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
> - gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K \
> - | grep fingerprint: | cut -d" " -f4 | tr -d '\n' > \
> + gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K |
> + grep fingerprint: | cut -d" " -f4 | tr -d '\n' > \
> ${GNUPGHOME}/trustlist.txt &&
I think this would be more readable with one item from the pipeline
per line:
gpgsm --homedir ... |
grep ... |
cut ... |
tr ... >... &&
[...]
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -218,8 +218,8 @@ test_expect_success "--batch-check for a non-existent
> hash" '
> test "0000000000000000000000000000000000000042 missing
> 0000000000000000000000000000000000000084 missing" = \
> "$( ( echo 0000000000000000000000000000000000000042;
> - echo_without_newline 0000000000000000000000000000000000000084; ) \
> - | git cat-file --batch-check)"
> + echo_without_newline 0000000000000000000000000000000000000084; ) |
> + git cat-file --batch-check)"
This test is problematic in a lot of ways. Most importantly, it ignores
the exist status from git cat-file.
So it should say something like:
cat >expect <<-\EOF &&
foobar42 missing
foobar84 missing
EOF
printf "foobar42\nfoobar84" |
git cat-file --batch-check >actual &&
test_cmp expect actual
If we want to restrict to the pipeline style fixes, we could do
test "..." = "$(
{ # Couldn't resist changing the ( to {!
echo ... && # Couldn't resist changing the ; to &&!
echo_without_newline ...
} |
git cat-file --batch-check
)"
but unless there's a linter that we're helping support, it's probably
better to skip this file and use a dedicated patch to modernize its
style more generally.
[...]
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1770,8 +1770,9 @@ test_expect_success '--show-origin stdin with file
> include' '
> cat >expect <<-EOF &&
> file:$INCLUDE_DIR/stdin.include include
> EOF
> - echo "[include]path=\"$INCLUDE_DIR\"/stdin.include" \
> - | git config --show-origin --includes --file - user.stdin
> >output &&
> + echo "[include]path=\"$INCLUDE_DIR\"/stdin.include" |
> + git config --show-origin --includes --file - user.stdin >output &&
Okay.
[...]
> --- a/t/t5317-pack-objects-filter-objects.sh
> +++ b/t/t5317-pack-objects-filter-objects.sh
> @@ -20,17 +20,20 @@ test_expect_success 'setup r1' '
> '
>
> test_expect_success 'verify blob count in normal packfile' '
> - git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
> - | awk -f print_2.awk \
> - | sort >expected &&
> + git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
> + awk -f print_2.awk |
> + sort >expected &&
This loses the exit status from git, so we should make it write to a
temporary file instead (as a separate patch).
[...]
> - git -C r1 verify-pack -v ../all.pack \
> - | grep blob \
> - | awk -f print_1.awk \
> - | sort >observed &&
> +
> + git -C r1 verify-pack -v ../all.pack |
Likewise (and likewise for the rest in this file).
[...]
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -50,8 +50,9 @@ pull_to_client () {
> case "$heads" in *B*)
> git update-ref refs/heads/B "$BTIP";;
> esac &&
> - git symbolic-ref HEAD refs/heads/$(echo $heads \
> - | sed -e "s/^\(.\).*$/\1/") &&
> +
> + git symbolic-ref HEAD refs/heads/$(echo $heads |
> + sed -e "s/^\(.\).*$/\1/") &&
It would be better to use a temporary variable. If we're just
changing line wrapping, then this would be
git symbolic-ref HAD refs/heads/$(
echo $heads |
sed ...
) &&
[...]
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -34,10 +34,12 @@ test_expect_success 'setup bare clone for server' '
> # confirm partial clone was registered in the local config.
> test_expect_success 'do partial clone 1' '
> git clone --no-checkout --filter=blob:none "file://$(pwd)/srv.bare" pc1
> &&
> - git -C pc1 rev-list HEAD --quiet --objects --missing=print \
> - | awk -f print_1.awk \
> - | sed "s/?//" \
> - | sort >observed.oids &&
> +
> + git -C pc1 rev-list HEAD --quiet --objects --missing=print |
Also needs to write to a temporary to avoid losing the exist status
(and likewise for the rest of this file).
[...]
> --- a/t/t6112-rev-list-filters-objects.sh
> +++ b/t/t6112-rev-list-filters-objects.sh
> @@ -20,24 +20,28 @@ test_expect_success 'setup r1' '
> '
>
> test_expect_success 'verify blob:none omits all 5 blobs' '
> - git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
> - | awk -f print_2.awk \
> - | sort >expected &&
> - git -C r1 rev-list HEAD --quiet --objects --filter-print-omitted
> --filter=blob:none \
> - | awk -f print_1.awk \
> - | sed "s/~//" \
> - | sort >observed &&
> + git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
> + awk -f print_2.awk |
> + sort >expected &&
Likewise.
[...]
> --- a/t/t9101-git-svn-props.sh
> +++ b/t/t9101-git-svn-props.sh
> @@ -193,8 +193,8 @@ test_expect_success 'test propget' "
> git svn propget svn:ignore . | cmp - prop.expect &&
> cd deeply &&
> git svn propget svn:ignore . | cmp - ../prop.expect &&
> - git svn propget svn:entry:committed-rev nested/directory/.keep \
> - | cmp - ../prop2.expect &&
> + git svn propget svn:entry:committed-rev nested/directory/.keep |
> + cmp - ../prop2.expect &&
Likewise.
Thanks and hope that helps,
Jonathan
