Re: [PATCH] status: learn --color for piping colored output

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Feb 1, 2021 at 11:09 PM Lance Ward <ljward10@gmail.com> wrote:
> On Sun, Jan 31, 2021 at 5:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > I think the approach I suggested of patching those wt-status.c functions
> > to use:
> >     rev.diffopt.use_color = s->use_color;
> > would fix this inconsistency, wouldn't it?
>
> I've made the change you requested and it resolves the issue.
> It also fixed the inconsistency I mentioned.  I only needed
> to modify wt_longstatus_print_verbose to resolve the issue
> since it is the only status path that had an issue with the
> git status command.

Okay, makes sense. As long as you insert the assignment somewhere
above the special case:

    if (s->fp != stdout) {
        rev.diffopt.use_color = 0;
        wt_status_add_cut_line(s->fp);
    }

then it should work correctly, I would think. However, it might be
easier for people to grok the logic overall if you incorporate it into
that conditional, perhaps like this:

    if (s->fp != stdout) {
        rev.diffopt.use_color = 0;
        wt_status_add_cut_line(s->fp);
    } else {
        rev.diffopt.use_color = s->use_color;
    }

Or this:

    if (s->fp == stdout) {
        rev.diffopt.use_color = s->use_color;
    else {
        rev.diffopt.use_color = 0;
        wt_status_add_cut_line(s->fp);
    }

It's subjective, of course, so use your best judgment.

> > In fact, I can envision this patch being re-rolled as a two-patch
> > series which (1) patches the wt-status.c functions to do
> > `rev.diffopt.use_color = s->use_color` which should make
> > `color.status` imply `color.diff`, and (2) adds a --color option to
> > `git status` which sets `wt_status.use_color` (which would then be
> > automatically inherited by the diff machinery due to the first patch).
>
> Right now as it stands my patch resolves both of these, but
> if you'd like to make it two separate patches that would be fine.

The reason I was thinking of splitting these changes into two patches
is that they have different purposes. You'd sell the first patch as a
straight up bug fix, and it's easier to formulate a proper "sales
spiel" for that if it's not blurred with other changes. (This may be
important because it could be slightly controversial if other people
don't consider the behavior as being buggy.) The second patch would be
an easy sell as a straightforward and simple enhancement. Another
reason for splitting them into two patches is that doing so would make
it easier to revert the bug-fix patch separately if it turns out that
there are unforeseen negative side-effects.

Having said that, a well-crafted commit message may very well make it
easy to sell the change(s) as a single patch. Again, use your best
judgment.