Re: [PATCH] check: don't use multi-line coloring for the report

[Stefano Lattarini]

Hi Bert, and thanks again for your patch.

I have some minor observations and objections below (please do not take
them as a belittling of your work; they are either constructive criticism,
or requests for cosmetic changes mandated by the GNU coding standards).
I hope you have time and will to address them; if not, don't worry, I can
do that for you.

On Friday 17 June 2011, Bert Wesarg wrote:
> less -R can't handle multi-line coloring as it is done for the check reports
> of the serial and parallel testsuite, because of performance reasons. Thus,
> color each line of the check report by its own.
> ---
>
It's customary for the ChangeLog entry and the git commit message to be equal;
in this particular case, they should both contain both your explanation above
(expressing the reasons and motivations for your patch) and the detailed
description of changes to individual files (that you've already written
into the ChangeLog).

>  ChangeLog       |    9 +++++++++
>  THANKS          |    1 +
>  lib/am/check.am |   40 +++++++++++++++++++++-------------------
>  3 files changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index e468ef3..492afe5 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,12 @@
> +2011-06-17  Bert Wesarg <address@hidden>
> +
Here you should add a "  (tiny change)" specification after your e-mail
address.  Again, this is not to belittle your work, but only to indicate
that your change is small enough that it doesn't require you to sign
a copyright assignement with the FSF in order to have your patch accepted
into the Automake official repository.

> +     check: don't use multi-line coloring for the report
> +     * lib/am/check.am (am__text_box): Accept colors for lines.
> +     [%?PARALLEL_TESTS%] $(TEST_SUITE_LOG): Let am__text_box handle the line
> +     coloring. [!%?PARALLEL_TESTS%] $(check-TESTS): Color each report line
>
There should be a line break here, after "coloring.".  Typo maybe?

> +     by its own.
> +     * THANKS: Update.
> +
>  2011-06-13  Stefano Lattarini  <address@hidden>
>  
>       tests: optimize tests on primary/prefix mismatch for speed
> diff --git a/THANKS b/THANKS
> index c3cc55c..4f8950a 100644
> --- a/THANKS
> +++ b/THANKS
> @@ -37,6 +37,7 @@ Benoit Sigoure              address@hidden
>  Bernard Giroud               address@hidden
>  Bernard Urban                address@hidden
>  Bernd Jendrissek     address@hidden
> +Bert Wesarg          address@hidden
>  Bill Currie          address@hidden
>  Bill Davidson                address@hidden
>  Bill Fenner          address@hidden
> diff --git a/lib/am/check.am b/lib/am/check.am
> index 97ecb68..1e9348c 100644
> --- a/lib/am/check.am
> +++ b/lib/am/check.am
> @@ -75,15 +75,17 @@ am__rst_title   = sed 's/.*/   &   
> /;h;s/./=/g;p;x;p;g;p;s/.*//'
>  am__rst_section = sed 'p;s/./=/g;p;g'
>  
>  # Put stdin (possibly several lines separated by ".  ") in a box.
> -am__text_box = $(AWK) '{                             \
> -  n = split($$0, lines, "\\.  "); max = 0;           \
> -  for (i = 1; i <= n; ++i)                           \
> -    if (max < length(lines[i]))                              \
> -      max = length(lines[i]);                                \
> -  for (i = 0; i < max; ++i) line = line "=";         \
> -  print line;                                                \
> -  for (i = 1; i <= n; ++i) if (lines[i]) print lines[i];\
> -  print line;                                                \
> +# Prefix each line by col and terminate each with std, for coloring.
>
I'd also like a comment here that briefly explan that mult-line coloring is
problematic with "less -R"; what about this?

 +# Prefix each line by col and terminate each with std, for coloring.
 +# Multi line coloring is problematic with "less -R", so we really need
 +# to color each line individually.

> +am__text_box = $(AWK) '{                     \
> +  n = split($$0, lines, "\\.  "); max = 0;   \
> +  for (i = 1; i <= n; ++i)                   \
> +    if (max < length(lines[i]))                      \
> +      max = length(lines[i]);                        \
> +  for (i = 0; i < max; ++i) line = line "="; \
> +  print col line std;                                \
> +  for (i = 1; i <= n; ++i)                   \
> +    if (lines[i]) print col lines[i] std;    \
> +  print col line std;                                \
>  }'
>  
>  # Solaris 10 'make', and several other traditional 'make' implementations,
> @@ -216,12 +218,11 @@ $(TEST_SUITE_LOG): $(TEST_LOGS)
>       test x"$$VERBOSE" = x || $$exit || cat $(TEST_SUITE_LOG);       \
>       $(am__tty_colors);                                              \
>       if $$exit; then                                                 \
> -       echo $(ECHO_N) "$$grn$(ECHO_C)";                              \
> +       col="$$grn";                                                  \
>        else                                                           \
> -       echo $(ECHO_N) "$$red$(ECHO_C)";                              \
> +       col="$$red";                                                  \
>       fi;                                                             \
> -     echo "$$msg" | $(am__text_box);                                 \
> -     echo $(ECHO_N) "$$std$(ECHO_C)";                                \
> +     echo "$$msg" | $(am__text_box) "col=$$col" "std=$$std";         \
>       $$exit
>  
>  RECHECK_LOGS = $(TEST_LOGS)
> @@ -401,14 +402,15 @@ check-TESTS: $(TESTS)
>         fi; \
>         dashes=`echo "$$dashes" | sed s/./=/g`; \
>         if test "$$failed" -eq 0; then \
> -         echo "$$grn$$dashes"; \
> +         col="$$grn"; \
>         else \
> -         echo "$$red$$dashes"; \
> +         col="$$red"; \
>         fi; \
> -       echo "$$banner"; \
> -       test -z "$$skipped" || echo "$$skipped"; \
> -       test -z "$$report" || echo "$$report"; \
> -       echo "$$dashes$$std"; \

Again, I'd like a comment before this; repetition is OK in this case IMHO,
so you could simply go with:

 ## Multi line coloring is problematic with "less -R", so we really need
 ## to color each line individually.

> +       echo "$${col}$$dashes$${std}"; \
> +       echo "$${col}$$banner$${std}"; \
> +       test -z "$$skipped" || echo "$${col}$$skipped$${std}"; \
> +       test -z "$$report" || echo "$${col}$$report$${std}"; \
> +       echo "$${col}$$dashes$${std}"; \
>         test "$$failed" -eq 0; \
>       else :; fi
>

Hmm, I see that you've forgotten to regenerate the Automake's own Makefile.in
files (not your fault, the automake rebuild rules are pretty weak in this
regard).  You should be able to regenerate them by running:
  $ ./boostrap && make
from the top-level directory of the already-configure Automake tree.  You
should see that `tests/Makefile.in' and `lib/Automake/tests/Makefile.in'
have been updated accordingly to the changes you've done to `check.am'.

Finally, a question; have you re-run the automake testsuite (or at least
the tests that check the code modified by your patch)?  If not, don't
worry, I can do that for you.  If yes, I'll save myself the trouble ;-)

Thanks,
  Stefano