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

[Eric Sunshine <sunshine@sunshineco.com>]

[cc:+junio +peff +duy +dscho +rene]

On Sun, Jan 31, 2021 at 1:49 PM Lance Ward <ljward10@gmail.com> wrote:
> On Sun, Jan 31, 2021 at 1:31 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Sat, Jan 30, 2021 at 10:51 AM Lance Ward via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > > diff --git a/diff.c b/diff.c
> > > +void set_diff_color(int use_color)
> > > +{
> > > +       diff_use_color_default = use_color;
> > > +}
> >
> > This new API for setting `diff_use_color_default` feels a bit too
> > quick-and-dirty and assumes that the caller has intimate knowledge
> > about when it is safe/correct to call the new function. Did you
> > consider the alternate approach of having wt-status functionality set
> > the appropriate diff_options.use_color value at the time it drives the
> > diff machinery? For instance, as a test, I added:
> >
> >     rev.diffopt.use_color = s->use_color;
> >
> > to the functions wt-status.c:wt_status_collect_changes_worktree(),
> > wt_status_collect_changes_index(), and wt_longstatus_print_verbose(),
> > so that the `use_color` value from the `struct wt_status *` supplied
> > by commit.c:cmd_status() is automatically applied to the diff options.
>
> Originally I tried to use what I thought would be a much more appropriate
> method which is to simply let the --color flag set things the same way
> as the config option status.color=always does, but I noticed it does
> not work the same when piped.

I'm not quite sure what you mean. How exactly did you originally
implement --config to accomplish this?

> For example the following produces full color output:
> git -c status.color=always status -v
>
> However, running this colors only the status, not the diff:
> git -c status.color=always status -v | cat
>
> Right now I can only get what I expect by running:
> git -c status.color=always -c diff.color=always status -v | cat
>
> This appeared to me to be inconsistent behaviour [...]

At an implementation level, this behavior makes sense, but I can see
how it might be confusing and unexpected from a user's viewpoint. 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?

> [...] and lead me down
> a path trying to figure out where the color was being disabled which
> made me realize that the status command shares code paths with
> the commit message and porcelain output.  I wanted to be very
> careful not to do anything which might break either of these in some
> unforeseen way which is why I created the function.

I see where you are coming from and understand the desire to isolate
this behavior change, however, I can't shake the feeling that this
sledge-hammer approach is going in the wrong direction and that the
fine-grained approach I suggested in my review is more desirable.
Having said that, I'm not particularly familiar with this area of the
code -- and had to spend some time digging through it to find those
functions in wt-status.c to make the fine-grained approach work -- so
it would be nice to hear from people who have spent a lot more time in
that area of the code (I Cc:'d them).

> If you feel existing unit tests would mitigate any issues with commit
> messages and porcelain output I can try going the route you
> suggested instead?

I doubt that anyone on this project feels that the unit tests have
sufficient coverage to make any guarantees. However, for changes such
as the one I proposed which might have unforeseen side-effects, Junio
tends to let the changes "cook" for a while in his 'next' branch
before promoting them to the 'master' branch so as to give time for
unexpected fallout to be discovered.

> Also if you agree the behavior of status.color should be the same for
> both piped and not piped output I could add that to this patch as well?

I'm not quite sure I understand your question. Are you asking if
`color.status` should imply `color.diff`? If so, I haven't thought a
lot about it, but I can see how the present behavior may have a high
surprise-factor for users, so it might be worthwhile.

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

(By the way, `status.color` is a deprecated synonym for `color.status`.)