Re: [PATCH v2 1/4] t4014: clean up style
From: Eric Sunshine <hidden>
Date: 2019-08-20 02:41:37
On Mon, Aug 19, 2019 at 7:53 PM Denton Liu [off-list ref] wrote:
In Git's tests, there is typically no space between the redirection operator and the filename. Remove these spaces. Since output is silenced when running without `-v` and debugging output is useful with `-v`, remove redirections to /dev/null. Change here-docs from `<<\EOF` to `<<-\EOF` so that they can be indented along with the rest of the test case. Convert all instances of `cnt=$(... | wc -l) && test $cnt = N` into uses of `test_line_count()`. For style, move the ending sq of test cases onto its own line whenever they do not conform. Rename output files from "expected" to "expect" to conform with the usual convention. Finally, refactor to remove Git commands upstream of pipe as well as Git commands that are in a non-variable-assignment subshell (e.g. `echo "base-commit: $(git rev-parse HEAD)"`. This way, if an invocation of a Git command fails, the return code won't be lost. Keep upstream non-Git commands since we have to assume a base level of sanity. Signed-off-by: Denton Liu <redacted> --- This patch is getting a little unwieldy. Perhaps we could split it into several smaller patches? Unfortunately, I'm not really sure where a logical place to split it would be.
The bullet points in the commit message, each of which is a distinct change, give a strong hint as to how the commit could be split into smaller pieces. It would make repeated review easier (though I'm not sure it's worth re-rolling just for that).
quoted hunk ↗ jump to hunk
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh@@ -62,26 +63,24 @@ test_expect_success setup ' test_expect_success "format-patch --ignore-if-in-upstream" ' git format-patch --stdout \ --ignore-if-in-upstream master..side >patch1 && - cnt=$(grep "^From " patch1 | wc -l) && - test $cnt = 2 - + grep "^From " patch1 >from1 && + test_line_count = 2 from1 '
Here you've removed the blank line following the body before the closing quote, which brings the formatting more in line with current style, however, you could do the same with the blank line before the body. Ditto for other tests. Another style fix would be to change the double quotes in the test title to single quotes (here and in other tests).
quoted hunk ↗ jump to hunk
@@ -357,7 +369,7 @@ test_expect_success 'reroll count (-v)' ' check_threading () { expect="$1" && shift && - (git format-patch --stdout "$@"; echo $? > status.out) | + git format-patch --stdout "$@" >patch && # Prints everything between the Message-ID and In-Reply-To, # and replaces all Message-ID-lookalikes by a sequence number perl -ne '@@ -372,12 +384,11 @@ check_threading () { print; } print "---\n" if /^From /i; - ' > actual && - test 0 = "$(cat status.out)" && + ' <patch >actual && test_cmp "$expect" actual }
If you do break this patch into smaller pieces, this might deserve its own patch (or not) because it requires a bit of extra reasoning by the reviewer.
quoted hunk ↗ jump to hunk
@@ -852,20 +866,21 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' ' -git_version="$(git --version | sed "s/.* //")" - signature() { + git_version="$(git --version >version && sed "s/.* //" version)" && printf "%s\n%s\n\n" "-- " "${1:-$git_version}" }
Windows folks are not going to like you for this change since process creation is so expensive on that platform, and the point of setting that variable globally was to avoid repeated invocation (especially since the output of "git --version" won't change). This function appears to be invoked only three times presently, but that number could increase in the future as new tests are added. Consequently, it might be best to drop this change.