On Sun, Mar 17 2019, Thomas Gummerer wrote: > On 03/17, Ævar Arnfjörð Bjarmason wrote: >> >> On Sun, Mar 17 2019, Jonathan Chang wrote: >> >> > Signed-off-by: Jonathan Chang <ttjtftx@xxxxxxxxx> >> > --- >> > 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.
