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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 31, 2021, 07:31 UTC
Message-ID: <CAPig+cQMn6oc4Jh=gb1jNfArXJBYhPRaSzJJvvbvprit6_OC0g@mail.gmail.com>
In-Reply-To: <pull.854.git.1612021544723.gitgitgadget@gmail.com>

On Sat, Jan 30, 2021 at 10:51 AM Lance Ward via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 15 quoted lines

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

Show 7 quoted lines

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

Show 12 quoted lines

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

Show 10 quoted lines

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