git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Adam Spiers <git@adamspiers.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>, git list <git@vger.kernel.org>
Subject: Re: [PATCH v6 0/7] make test output coloring more intuitive
Date: Thu, 20 Dec 2012 23:28:57 +0000	[thread overview]
Message-ID: <CAOkDyE9tDYRYzojzNnjWsT7UygxMAurHqLSDGA66_LMPD2Wmnw@mail.gmail.com> (raw)
In-Reply-To: <7vy5gs4jiy.fsf@alter.siamese.dyndns.org>

On Thu, Dec 20, 2012 at 7:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>>> Good point, I forgot to check what it looked like with -v.  Since this
>>> series is already on v6, is there a more lightweight way of addressing
>>> this tiny tweak than sending v7?
>>
>> It is ultimately up to Junio, but I suspect he would be OK if you just
>> reposted patch 4/7 with the above squashed. Or even just said "I like
>> this, please squash it into patch 4 (change info messages from
>> yellow/brown to bold cyan).
>
> Surely; as long as the series is not in 'next', the change to be
> squashed is not too big and it is not too much work (and in this
> case it certainly is not).

OK.

> I actually wonder if "skipped test in bold blue" and "known breakage
> in bold yellow" should also lose the boldness.  Errors and warnings
> in bold are good, but I would say the degree of need for attention
> are more like this:
>
>         error (failed tests - you should look into it)
>         skip (skipped - perhaps you need more packages?)
>         warn (expected failure - you may want to look into fixing it someday)
>         info
>         pass
>
> The "expected_failure" cases painted in "warn" are all long-known
> failures; I do not think reminding about them in "bold" over and
> over will help encouraging the developers take a look at them.

As Peff already noted, on many (most?) X terminals "bold" colours are
just brighter colours, rather than a heavier typeface.  How bold they
look is therefore dependent on the colour scheme used by that
terminal.

> The "skipped" cases fall into two categories.  Either you already
> know you choose to not to care (e.g. I do not expect to use git-p4
> and decided not to install p4 anywhere, so I may have t98?? on
> GIT_SKIP_TESTS environment) or you haven't reached that point on a
> new system and haven't realized that you didn't install a package
> needed to run tests you care about (e.g. cvsserver tests would not
> run without Perl interface to SQLite).  For the former, the bold
> output is merely distracting; for the latter, bold _might_ help in
> this case.

Very good point.

> At least, I think
>
>         GIT_SKIP_TESTS=t98?? sh t9800-git-p4-basic.sh -v
>
> should paint "skipping test t9800 altogether" (emitted with "-v) and
> the last line "1..0 # SKIP skip all tests in t9800" both in the same
> "info" color.
>
> How about going further to reduce "bold" a bit more, like this?
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index aaf013e..2bbb81d 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -182,13 +182,13 @@ then
>                 error)
>                         tput bold; tput setaf 1;; # bold red
>                 skip)
> -                       tput bold; tput setaf 4;; # bold blue
> +                       tput setaf 4;; # bold blue

The comment still says "bold".

>                 warn)
> -                       tput bold; tput setaf 3;; # bold brown/yellow
> +                       tput setaf 3;; # bold brown/yellow

Ditto here ...

>                 pass)
>                         tput setaf 2;;            # green
>                 info)
> -                       tput bold; tput setaf 6;; # bold cyan
> +                       tput setaf 6;; # bold cyan

... and here.

>                 *)
>                         test -n "$quiet" && return;;
>                 esac
> @@ -589,7 +589,7 @@ for skp in $GIT_SKIP_TESTS
>  do
>         case "$this_test" in
>         $skp)
> -               say_color skip >&3 "skipping test $this_test altogether"
> +               say_color info >&3 "skipping test $this_test altogether"
>                 skip_all="skip all tests in $this_test"
>                 test_done
>         esac

Yes, I like this last hunk especially.

I have no objection in principle to a reduction in boldness.

However, I am beginning to get disheartened that at this rate, this
series will never land.  I already submitted v4 of the series which
already had non-bold blue.  I then received feedback indicating that
bold blue would be more suitable, so despite alarm bells beginning to
ring in my head, I submitted v5 with bold blue, declaring that that
would be my last version:

  http://article.gmane.org/gmane.comp.version-control.git/206042

A further concern about "info" messages not being blue prompted me
to attempt to canvass more opinions:

  http://article.gmane.org/gmane.comp.version-control.git/209321

I received none, so submitted v6 based on my best judgement.  Now we
are talking about a potential v7 going *back* to non-bold blue.  I can
submit v7 if you think it's worth it, but would that really be the end
of the discussion?  It's clear from the above that colour scheme
design by committee is about as good an idea as asking a bunch of kids
to reach consensus on their favourite colour ;-)

So if possible I'd be very happy for Junio to simply make an executive
decision (I don't care which way, as long as it fits the traffic
lights scheme and uses distinct hues of blue/cyan for the different
categories of skip/info messages), tweak the latest v6 series
accordingly, and then push so that we can all go back to more pressing
things ;-)

Hopefully that is a reasonable way forward?

Thanks,
Adam

  parent reply	other threads:[~2012-12-20 23:29 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-16 18:28 [PATCH v6 0/7] make test output coloring more intuitive Adam Spiers
2012-12-16 18:28 ` [PATCH v6 1/7] tests: test number comes first in 'not ok $count - $message' Adam Spiers
2012-12-16 18:28 ` [PATCH v6 2/7] tests: paint known breakages in bold yellow Adam Spiers
2012-12-16 18:28 ` [PATCH v6 3/7] tests: paint skipped tests in bold blue Adam Spiers
2012-12-16 18:28 ` [PATCH v6 4/7] tests: change info messages from yellow/brown to bold cyan Adam Spiers
2012-12-16 18:28 ` [PATCH v6 5/7] tests: refactor mechanics of testing in a sub test-lib Adam Spiers
2012-12-16 18:28 ` [PATCH v6 6/7] tests: test the test framework more thoroughly Adam Spiers
2012-12-16 18:28 ` [PATCH v6 7/7] tests: paint unexpectedly fixed known breakages in bold red Adam Spiers
2012-12-16 18:54 ` [PATCH v6 0/7] make test output coloring more intuitive Junio C Hamano
2012-12-16 19:01   ` Adam Spiers
2012-12-16 23:11     ` Junio C Hamano
2012-12-20 15:34     ` Jeff King
2012-12-20 15:44       ` Adam Spiers
2012-12-20 16:11         ` Jeff King
2012-12-20 18:08           ` Adam Spiers
2012-12-20 19:21           ` Junio C Hamano
2012-12-20 19:50             ` Jeff King
2012-12-20 23:28             ` Adam Spiers [this message]
2012-12-21  3:12               ` [PATCH v7 0/7] coloring test output after traffic signal Junio C Hamano
2012-12-21  8:15                 ` Jeff King
2012-12-21 10:00                   ` Adam Spiers
2012-12-21  3:12               ` [PATCH v7 1/7] tests: test number comes first in 'not ok $count - $message' Junio C Hamano
2012-12-21  3:12               ` [PATCH v7 2/7] tests: paint known breakages in yellow Junio C Hamano
2012-12-21  8:51                 ` Stefano Lattarini
2012-12-21 15:46                   ` Junio C Hamano
2012-12-21 16:59                     ` Stefano Lattarini
2012-12-21  3:12               ` [PATCH v7 3/7] tests: paint skipped tests in blue Junio C Hamano
2012-12-21  3:12               ` [PATCH v7 4/7] tests: change info messages from yellow/brown to cyan Junio C Hamano
2012-12-21  3:12               ` [PATCH v7 5/7] tests: refactor mechanics of testing in a sub test-lib Junio C Hamano
2012-12-21  3:12               ` [PATCH v7 6/7] tests: test the test framework more thoroughly Junio C Hamano
2012-12-21  3:12               ` [PATCH v7 7/7] tests: paint unexpectedly fixed known breakages in bold red Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOkDyE9tDYRYzojzNnjWsT7UygxMAurHqLSDGA66_LMPD2Wmnw@mail.gmail.com \
    --to=git@adamspiers.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).