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 <[email protected]>

---
 builtin/commit.c             |  7 ++++
 diff.c                       |  5 +++
 diff.h                       |  1 +
 t/t7527-status-color-pipe.sh | 69 ++++++++++++++++++++++++++++++++++++

builtin/commit.c +4 -0

@@ -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.c +4 -0

@@ -261,6 +261,11 @@ void init_diff_ui_defaults(void)
+void set_diff_color(int use_color)
+{
+       diff_use_color_default = use_color;
+}

Quoted 6 lines · Lance Ward via GitGitGadget

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.

Quoted 6 lines

Signed-off-by Lance Ward <[email protected]>

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

Quoted 15 lines

builtin/commit.c +4 -0

@@ -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.c +4 -0

@@ -261,6 +261,11 @@ void init_diff_ui_defaults(void)
+void set_diff_color(int use_color)
+{
+       diff_use_color_default = use_color;
+}

diff.h int parse_long_opt(const char *opt, const char **argv, · line 501

Quoted 1 line

diff.h +1 -0

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

t/t7527-status-color-pipe.sh line 5

Quoted 8 lines

t/t7527-status-color-pipe.sh +5 -0

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

t/t7527-status-color-pipe.sh line 29

Quoted 13 lines

t/t7527-status-color-pipe.sh +12 -0

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

t/t7527-status-color-pipe.sh line 69

Quoted 11 lines

t/t7527-status-color-pipe.sh +10 -0

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

Quoted 15 lines · Eric Sunshine

diff.c void init_diff_ui_defaults(void) · line 267

Quoted 5 lines · Lance Ward via GitGitGadget

diff.c +4 -0

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

Quoted 6 lines

    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.

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?

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

Quoted 23 lines · Lance Ward

Quoted 15 lines · Eric Sunshine

diff.c void init_diff_ui_defaults(void) · line 267

Quoted 5 lines · Lance Ward via GitGitGadget

diff.c +4 -0

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

Quoted 6 lines

    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.

Quoted 4 lines

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?

Quoted 10 lines

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?

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 2 lines

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

Quoted 4 lines · Eric Sunshine

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.

Quoted 6 lines

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.

Quoted 11 lines · Lance Ward

Quoted 4 lines · Eric Sunshine

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.

Quoted 9 lines

Quoted 6 lines

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