Re: [PATCH 1/8] t1091: use check_files to reduce boilerplate
From: Junio C Hamano <hidden>
Date: 2020-01-16 21:40:47
"Derrick Stolee via GitGitGadget" [off-list ref] writes:
quoted hunk
From: Derrick Stolee <redacted> When testing the sparse-checkout feature, we need to compare the contents of the working-directory against some expected output. Using here-docs was useful in the beginning, but became repetetive as the test script grew. Create a check_files helper to make the tests simpler and easier to extend. It also reduces instances of bad here-doc whitespace. Signed-off-by: Derrick Stolee <redacted> --- t/t1091-sparse-checkout-builtin.sh | 215 ++++++++++------------------- 1 file changed, 71 insertions(+), 144 deletions(-)diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index ff7f8f7a1f..20caefe155 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh@@ -12,6 +12,13 @@ list_files() { (cd "$1" && printf '%s\n' *) } +check_files() { + DIR=$1 + printf "%s\n" $2 >expect && + list_files $DIR >actual &&
It is unclear if the script is being deliberate or sloppy.
It turns out that not quoting $2 is deliberate (i.e. it wants to
pass more than one words in $2, have them split at $IFS and show
each of them on a separate line), at the same time not quoting $DIR
is simply sloppy.
And it is totally unnecessary to confuse readers like this.
Unless you plan to extend this helper further, I think this would be
much less burdensome to the readers:
check_files () {
list_files "$1" >actual &&
shift &&
printf "%s\n" "$@" >expect &&
test_cmp expect actual
}
This ...
test_cmp expect repo/.git/info/sparse-checkout && - list_files repo >dir && - cat >expect <<-EOF && - a - folder1 - folder2 - EOF - test_cmp expect dir + check_files repo "a folder1 folder2"
... is a kind of change that the log message advertises, which is a very nice rewrite. And ...
test_expect_success 'clone --sparse' ' git clone --sparse repo clone && git -C clone sparse-checkout list >actual && - cat >expect <<-EOF && - /* - !/*/ + cat >expect <<-\EOF && + /* + !/*/ EOF
... this is a style-fix that is another nice rewrite but in a different category. I wonder if they should be done in separate commits. Other than that, makes sense. Thanks.