Re: [GSoC][PATCH v3 3/3] t0000: use test_line_count instead of wc -l

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@gmail.com>
>> > ---
>> >  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.