Re: [PATCH] status: fix verbose status coloring inconsistency

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 5, 2021, 07:17 UTC
Message-ID: <CAPig+cRos+wGuwqbjuAV11kM5U0TPbBO9F4o4VJzQ4eZyxArMA@mail.gmail.com>
In-Reply-To: <xmqqa6skbw9u.fsf@gitster.c.googlers.com>

On Wed, Feb 3, 2021 at 5:51 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> "Lance Ward via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >  t/t7527-status-color-pipe.sh | 55 ++++++++++++++++++++++++++++++++++++
>
> Don't we already have test that checks "status" output including its
> coloring already? I'd rather see us adding to existing test script,
> than allocating a new number for a small subset of features being
> tested for a command.  After all, test numbers are limited resource.

t7508 seems like a good place for these tests.

Show 7 quoted lines

> > +test_expect_success 'git status' '
> > +     git status >raw &&
> > +     test_decode_color <raw >out &&
> > +     grep "original$" out
> > +'
>
> Not "new file: *original$" or something less false-positive prone?

The "new file:" message is localized, so this `grep` will need to become `test_i18ngrep` again (as it was in the original submission) if this approach is adopted, which is fine.

Show 9 quoted lines

> > +test_expect_success 'git -c color.status=never status' '
> > +     git -c color.status=never status >raw &&
> > +     test_decode_color <raw >out &&
> > +     grep "original$" out
> > +'
>
> Would it make sense to have tests for color.status=true, I wonder.
> It requires tty to actually "see" the colors output but sending
> the output to tty is the normal use case, so we should care...

I wondered if `color.status=true` might already be tested by t7508 but apparently it only checks `auto`.

Show 8 quoted lines

> > +test_expect_success 'git -c color.status=always status -v' '
> > +     git -c color.status=always status -v >raw &&
> > +     test_decode_color <raw >out &&
> > +     grep "<CYAN>@@ -0,0 +1 @@<RESET>" out &&
> > +     grep "GREEN>+<RESET><GREEN>1<RESET>" out
> > +'
>
> Are we forcing the standard palette?

As this is a stand-alone test script with a well-controlled initial configuration, I expect it would be using the default palette. t7508 does assign a custom palette for `color.status` but not, apparently, for `color.diff`, so it presumably should be okay there, as well.