On Sun, Mar 17 2019, Thomas Gummerer wrote:

On 03/17, Ævar Arnfjörð Bjarmason wrote:

quoted

On Sun, Mar 17 2019, Jonathan Chang wrote:

quoted

Signed-off-by: Jonathan Chang <redacted>
---
 t/t0000-basic.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 47666b013e..3de13daabe 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh

@@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '
 	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) &&
 	test "z$commit0" = "z$parent" &&
 	git show --pretty=raw $commit2 >actual &&
-	numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
-	test $numparent = 1
+	sed -n -e "s/^parent //p" -e "/^author /q" actual >parents &&
+	test_line_count = 1 parents
 '

 test_expect_success 'update-index D/F conflict' '

@@ -1146,8 +1146,7 @@ test_expect_success 'update-index D/F conflict' '
 	mv tmp path2 &&
 	git update-index --add --replace path2 path0/file2 &&
 	git ls-files path0 >actual &&
-	numpath0=$(wc -l <actual) &&
-	test $numpath0 = 1
+	test_line_count = 1 actual
 '

...of course reading this series in sequence I find that 50% of my
suggestions for 2/3 are then done in this patch... :)

Indeed.  I think doing this in a separate patch is a good idea, as it
makes the previous patch slightly easier to review imho.  But I think
something to take away from this for Jonathan would be that this
should have been described in the commit message of the previous
commit.  Maybe something like

    This commit doesn't make any additional simplifications, such as
    using the test_line_count function for counting the lines in the
    output.  These simplifications will be made in a subsequent commit.

in addition to the existing commit message would have helped save a
bit of review effort.

FWIW I chuck this up to just my blindness / expedience in reading the
thing.

No objections to changing this, but I don't think it's the fault of a
commit message if someone reading it doesn't get an explanation for a
future unrelated improvement.

The times when a commit should have such an explanation are cases like
e.g. introducing a function that's not used yet to make a subsequent
commit smaller, or other such cases where the change is incomplete in
some way.