patchstatus: learn --color for piping colored output

> Many users like to pipe colored results of git status to other commands
> such as more or less, but by default colors are lost when piping without
> changing the user's git configuration.  Many other commands such as diff,
> show, log and grep have a --color option to easily override this behavior.
> This allows the status command to have a similar --color option providing
> a simpler mechanism for temporarily forcing piped colored output.

> Signed-off-by: Lance Ward <ljward10@gmail.com>
> ---
>  builtin/commit.c             |  7 ++++
>  diff.c                       |  5 +++
>  diff.h                       |  1 +
>  t/t7527-status-color-pipe.sh | 69 ++++++++++++++++++++++++++++++++++++

> diff --git a/builtin/commit.c b/builtin/commit.c
> @@ -1410,6 +1412,11 @@ int cmd_status(int argc, const char **argv, const char *prefix)
> +       if (use_color != GIT_COLOR_AUTO) {
> +               s.use_color=use_color;
> +               set_diff_color(use_color);
> +       }
> diff --git a/diff.c b/diff.c
> @@ -261,6 +261,11 @@ void init_diff_ui_defaults(void)
> +void set_diff_color(int use_color)
> +{
> +       diff_use_color_default = use_color;
> +}
> diff --git a/diff.h b/diff.h
> @@ -501,6 +501,7 @@ int parse_long_opt(const char *opt, const char **argv,
> +void set_diff_color(int use_color);

> diff --git a/t/t7527-status-color-pipe.sh b/t/t7527-status-color-pipe.sh
> @@ -0,0 +1,69 @@
> +# Normal git status does not pipe colors
> +test_expect_success 'git status' '
> +       git status >out &&
> +       test_i18ngrep "original$" out
> +'

> +# Test new color option with never (expect same as above)
> +test_expect_success 'git status --color=never' '
> +       git status --color=never >out &&
> +       test_i18ngrep "original$" out
> +'
> +
> +# Test new color (default is always)
> +test_expect_success 'git status --color' '
> +       git status --color |
> +       test_decode_color >out &&
> +       test_i18ngrep "original<RESET>$" out
> +'

> +test_done
> +# Test verbose --color=always
> +test_expect_success 'git status -v --color=always' '
> +       git status -v --color=always |
> +       test_decode_color >out &&
> +       test_i18ngrep "<CYAN>@@ -0,0 +1 @@<RESET>" out &&
> +       test_i18ngrep "GREEN>+<RESET><GREEN>1<RESET>" out
> +'
> +
> +test_done

>
> On Sat, Jan 30, 2021 at 10:51 AM Lance Ward via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Many users like to pipe colored results of git status to other commands
> > such as more or less, but by default colors are lost when piping without
> > changing the user's git configuration.  Many other commands such as diff,
> > show, log and grep have a --color option to easily override this behavior.
> > This allows the status command to have a similar --color option providing
> > a simpler mechanism for temporarily forcing piped colored output.
>
> Thanks, makes sense.
>
> > Signed-off-by: Lance Ward <ljward10@gmail.com>
> > ---
> >  builtin/commit.c             |  7 ++++
> >  diff.c                       |  5 +++
> >  diff.h                       |  1 +
> >  t/t7527-status-color-pipe.sh | 69 ++++++++++++++++++++++++++++++++++++
>
> As this is introducing a new --color option to `git status`, it should
> be accompanied by an update to Documentation/git-status.txt.

>
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > @@ -1410,6 +1412,11 @@ int cmd_status(int argc, const char **argv, const char *prefix)
> > +       if (use_color != GIT_COLOR_AUTO) {
> > +               s.use_color=use_color;
> > +               set_diff_color(use_color);
> > +       }
> > diff --git a/diff.c b/diff.c
> > @@ -261,6 +261,11 @@ void init_diff_ui_defaults(void)
> > +void set_diff_color(int use_color)
> > +{
> > +       diff_use_color_default = use_color;
> > +}
> > diff --git a/diff.h b/diff.h
> > @@ -501,6 +501,7 @@ int parse_long_opt(const char *opt, const char **argv,
> > +void set_diff_color(int 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.
>
> (Note that this was just a quick test. I dug through the code just
> enough to locate these functions as the likely correct place to set
> diff_options.use_color, but didn't spend any time verifying that all
> three functions need to be patched like this. I also didn't verify
> that my changes won't stomp on --porcelain's explicit disabling of
> color, which is something that ought to be checked. There's also some
> custom `use_color` handling going on in wt_longstatus_print_verbose()
> of which to be aware.)

>
> > diff --git a/t/t7527-status-color-pipe.sh b/t/t7527-status-color-pipe.sh
> > @@ -0,0 +1,69 @@
> > +# Normal git status does not pipe colors
> > +test_expect_success 'git status' '
> > +       git status >out &&
> > +       test_i18ngrep "original$" out
> > +'
>
> None of the text being checked by any of the tests being added by this
> file is subject to localization, so use of test_i18ngrep() is
> unwarranted. Use plain old `grep` instead here and elsewhere.

>
> > +# Test new color option with never (expect same as above)
> > +test_expect_success 'git status --color=never' '
> > +       git status --color=never >out &&
> > +       test_i18ngrep "original$" out
> > +'
> > +
> > +# Test new color (default is always)
> > +test_expect_success 'git status --color' '
> > +       git status --color |
> > +       test_decode_color >out &&
> > +       test_i18ngrep "original<RESET>$" out
> > +'
>
> If someone introduces a bug which causes the non-colored tests to
> incorrectly emit color codes, then it will be easier to debug the
> problem if you also pass the output through test_decode_color() even
> for the non-colored tests rather than only for the expect-colored
> tests. Thus, I'd recommend calling test_decode_color() in all the
> tests, even if you don't expect color to be emitted.
>
> Also, these days, we don't normally place a Git command upstream in a
> pipe since its exit code will be lost. Instead, we capture the output
> to a file:
>
>     git status --color >raw &&
>     test_decode_color <raw >out &&
>     ...

>
> > +test_done
> > +# Test verbose --color=always
> > +test_expect_success 'git status -v --color=always' '
> > +       git status -v --color=always |
> > +       test_decode_color >out &&
> > +       test_i18ngrep "<CYAN>@@ -0,0 +1 @@<RESET>" out &&
> > +       test_i18ngrep "GREEN>+<RESET><GREEN>1<RESET>" out
> > +'
> > +
> > +test_done
>
> You have one too many calls to test_done() in this fragment. I think
> you only want to keep the final test_done() and remove the one prior
> to the last test.

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

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

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

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

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

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

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

> 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