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

[Lance Ward <ljward10@gmail.com>]

Hi Eric,

I made all the changes you suggested and split it into two
independent changes:

This patch handles the --color option:
https://github.com/git/git/pull/955

This patch fixes the coloring inconsistency:
https://github.com/git/git/pull/954

Both have failed a CI test, but the reason has nothing to do with my
code.  CI/PR / static-analysis VM is trying to install the package
coccinelle, but is failing to install it with the following:

E: Unable to locate package coccinelle
Error: Process completed with exit code 100.

On Tue, Feb 2, 2021 at 10:46 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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.
>

I've made the change you suggested.

> > > 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.

I've split it into two patches.