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

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

[Lance Ward via GitGitGadget]

From: Lance Ward <ljward10@gmail.com>

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>
---
    status: 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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-854%2Fljward10%2Flw-add-status-color-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-854/ljward10/lw-add-status-color-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/854

 builtin/commit.c             |  7 ++++
 diff.c                       |  5 +++
 diff.h                       |  1 +
 t/t7527-status-color-pipe.sh | 69 ++++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+)
 create mode 100755 t/t7527-status-color-pipe.sh

diff --git a/builtin/commit.c b/builtin/commit.c
index 739110c5a7f..1579f7cc9ed 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1355,6 +1355,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
 int cmd_status(int argc, const char **argv, const char *prefix)
 {
 	static int no_renames = -1;
+	static int use_color = GIT_COLOR_AUTO;
 	static const char *rename_score_arg = (const char *)-1;
 	static struct wt_status s;
 	unsigned int progress_flag = 0;
@@ -1378,6 +1379,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			    STATUS_FORMAT_LONG),
 		OPT_BOOL('z', "null", &s.null_termination,
 			 N_("terminate entries with NUL")),
+		OPT__COLOR(&use_color, N_("use colored output")),
 		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg,
 		  N_("mode"),
 		  N_("show untracked files, optional modes: all, normal, no. (Default: all)"),
@@ -1410,6 +1412,11 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	handle_untracked_files_arg(&s);
 	handle_ignored_arg(&s);
 
+	if (use_color != GIT_COLOR_AUTO) {
+		s.use_color=use_color;
+		set_diff_color(use_color);
+	}
+
 	if (s.show_ignored_mode == SHOW_MATCHING_IGNORED &&
 	    s.show_untracked_files == SHOW_NO_UNTRACKED_FILES)
 		die(_("Unsupported combination of ignored and untracked-files arguments"));
diff --git a/diff.c b/diff.c
index 69e3bc00ed8..fe7ffce6803 100644
--- a/diff.c
+++ b/diff.c
@@ -261,6 +261,11 @@ void init_diff_ui_defaults(void)
 	diff_detect_rename_default = DIFF_DETECT_RENAME;
 }
 
+void set_diff_color(int use_color)
+{
+       diff_use_color_default = use_color;
+}
+
 int git_diff_heuristic_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "diff.indentheuristic"))
diff --git a/diff.h b/diff.h
index 2ff2b1c7f2c..10196d9b040 100644
--- a/diff.h
+++ b/diff.h
@@ -501,6 +501,7 @@ int parse_long_opt(const char *opt, const char **argv,
 int git_diff_basic_config(const char *var, const char *value, void *cb);
 int git_diff_heuristic_config(const char *var, const char *value, void *cb);
 void init_diff_ui_defaults(void);
+void set_diff_color(int use_color);
 int git_diff_ui_config(const char *var, const char *value, void *cb);
 #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
 #define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts)
diff --git a/t/t7527-status-color-pipe.sh b/t/t7527-status-color-pipe.sh
new file mode 100755
index 00000000000..ee4ab2ea821
--- /dev/null
+++ b/t/t7527-status-color-pipe.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='git status color option'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo 1 >original &&
+	git add .
+'
+
+# 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 new color option with always
+test_expect_success 'git status --color=always' '
+	git status --color=always |
+	test_decode_color >out &&
+	test_i18ngrep "original<RESET>$" out
+'
+
+# Test verbose (default)
+test_expect_success 'git status -v' '
+	git status -v |
+	test_decode_color >out &&
+	test_i18ngrep "+1" out
+'
+
+# Test verbose --color=never
+test_expect_success 'git status -v --color=never' '
+	git status -v --color=never |
+	test_decode_color >out &&
+	test_i18ngrep "+1" out
+'
+
+# Test verbose --color (default always)
+test_expect_success 'git status -v --color' '
+	git status -v --color |
+	test_decode_color >out &&
+	test_i18ngrep "<CYAN>@@ -0,0 +1 @@<RESET>" out &&
+	test_i18ngrep "<GREEN>+<RESET><GREEN>1<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

base-commit: e6362826a0409539642a5738db61827e5978e2e4
-- 
gitgitgadget

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

[Eric Sunshine]

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.

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

[Lance Ward]

Hi Eric,

Thank you for the comments!  I made all your suggested changes, except
for one which I wanted to provide more context on and ask a question
about before proceeding.  Please see my responses below.

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

I've added the option to the documentation.

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

I can understand how this may look "quick-and-dirty", but it was actually
my intent to create a function which assumes that the caller has intimate
knowledge about when it is safe/correct.  I will try to explain why.

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.

I changed the function name and added a comment to make it clear
that its usage is a special case which may have undesired
consequences if not used appropriately.

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?

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?

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

I made these changes.

>
> > +# 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 &&
>     ...

I made both these changes also.

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

You are correct, thanks for catching this.

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

[Eric Sunshine]

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

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

[Lance Ward]

Hi Eric,

I've updated my patch per your request.

On Sun, Jan 31, 2021 at 5:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> [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?

Yes, it did.

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

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.

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

I did not have to patch all the functions you mentioned and think
the new change is cleaner and will not break anything else.

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

Yes, the inconsistency I mentioned was basically that:
color.status implies color.diff when outputting to stdout,
but this is not true when outputting to a file.  My latest
change resolves this inconsistency.  Now color.status
implies color.diff when running git status regardless
of where the output is going.

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

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.

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

[Eric Sunshine]

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.

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

[Lance Ward]

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.

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

[Eric Sunshine]

On Wed, Feb 3, 2021 at 4:08 PM Lance Ward <ljward10@gmail.com> wrote:
> 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

Thanks for being responsive and indulging this reviewer. The two-patch
approach turned out to be concise and easy to digest. The bug fix --
which effectively turned out to be a one-liner -- is an especially
nice improvement over the earlier version, and the implementation of
the new --color option boiled down very nicely to a really minimal
amount of straightforward code.

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

[Lance Ward via GitGitGadget]

From: Lance Ward <ljward10@gmail.com>

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>
---
    status: learn --color command line option
    
    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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-955%2Fljward10%2Flw-add-status-color-option-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-955/ljward10/lw-add-status-color-option-v1
Pull-Request: https://github.com/git/git/pull/955

 Documentation/git-status.txt   |  9 ++++++++
 builtin/commit.c               |  6 +++++
 t/t7528-status-color-option.sh | 40 ++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+)
 create mode 100755 t/t7528-status-color-option.sh

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index c0764e850a44..d5655d051841 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -32,6 +32,15 @@ OPTIONS
 --branch::
 	Show the branch and tracking info even in short-format.
 
+--color[=<when>]::
+	Color status output, overriding configuration file setting.
+	The value must be always (the default), never, or auto.
+
+--no-color::
+	Turn off color ouput, even when the configuration file gives
+	the default to color output.
+	Same as `--color=never`.
+
 --show-stash::
 	Show the number of entries currently stashed away.
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 739110c5a7f6..7f0a9d07a0ee 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1355,6 +1355,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
 int cmd_status(int argc, const char **argv, const char *prefix)
 {
 	static int no_renames = -1;
+	static int use_color = GIT_COLOR_AUTO;
 	static const char *rename_score_arg = (const char *)-1;
 	static struct wt_status s;
 	unsigned int progress_flag = 0;
@@ -1378,6 +1379,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			    STATUS_FORMAT_LONG),
 		OPT_BOOL('z', "null", &s.null_termination,
 			 N_("terminate entries with NUL")),
+		OPT__COLOR(&use_color, N_("use colored output")),
 		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg,
 		  N_("mode"),
 		  N_("show untracked files, optional modes: all, normal, no. (Default: all)"),
@@ -1410,6 +1412,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	handle_untracked_files_arg(&s);
 	handle_ignored_arg(&s);
 
+	if (use_color != GIT_COLOR_AUTO) {
+		s.use_color=use_color;
+	}
+
 	if (s.show_ignored_mode == SHOW_MATCHING_IGNORED &&
 	    s.show_untracked_files == SHOW_NO_UNTRACKED_FILES)
 		die(_("Unsupported combination of ignored and untracked-files arguments"));
diff --git a/t/t7528-status-color-option.sh b/t/t7528-status-color-option.sh
new file mode 100755
index 000000000000..d14ea03f92c3
--- /dev/null
+++ b/t/t7528-status-color-option.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='git status color option'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo 1 >original &&
+	git add .
+'
+
+# Normal git status does not pipe colors
+test_expect_success 'git status' '
+	git status >raw &&
+	test_decode_color <raw >out &&
+	grep "original$" out
+'
+
+# Test new color option with never (expect same as above)
+test_expect_success 'git status --color=never' '
+	git status --color=never >raw &&
+	test_decode_color <raw >out &&
+	grep "original$" out
+'
+
+# Test new color (default is always)
+test_expect_success 'git status --color' '
+	git status --color >raw &&
+	test_decode_color <raw >out &&
+	grep "original<RESET>$" out
+'
+
+# Test new color option with always
+test_expect_success 'git status --color=always' '
+	git status --color=always >raw &&
+	test_decode_color <raw >out &&
+	grep "original<RESET>$" out
+'
+
+test_done

base-commit: e6362826a0409539642a5738db61827e5978e2e4
-- 
gitgitgadget