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

[Eric Sunshine <sunshine@sunshineco.com>]

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.