{"thread":{"id":"55099","subject":"[PATCH] status: fix verbose status coloring inconsistency","startedAt":"2021-02-03T21:34:52Z","lastAt":"2021-02-05T07:18:15Z","messageCount":5,"participants":["Lance Ward via GitGitGadget","Junio C Hamano","Lance Ward","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"416096","messageId":"pull.954.git.git.1612388043875.gitgitgadget@gmail.com","threadId":"55099","inReplyTo":null,"subject":"[PATCH] status: fix verbose status coloring inconsistency","fromName":"Lance Ward via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-03T21:34:03Z","receivedAt":"2021-02-03T21:34:52Z","isPatch":true,"sender":{"key":"ljward10@gmail.com","avatar":null},"body":"From: Lance Ward \n\nCurrently setting color.status=always results in a colored diff when\ngoing stdout, but an uncolored diff when going to other files or piping\nto other commands such as less or more. This patch fixes this and now\ncolor.status=always implies color.diff=always regardless of the output\nlocation.\n\nSigned-off-by: Lance Ward \n---\n status: fix verbose status coloring inconsistency\n \n Currently setting color.status=always results in a colored diff when\n going stdout, but an uncolored diff when going to other files or piping\n to other commands such as less or more. This patch fixes this and now\n color.status=always implies color.diff=always regardless of the output\n location.\n \n Signed-off-by: Lance Ward ljward10@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-954%2Fljward10%2Flw-fix-status-coloring-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-954/ljward10/lw-fix-status-coloring-v1\nPull-Request: https://github.com/git/git/pull/954\n\n t/t7527-status-color-pipe.sh | 55 ++++++++++++++++++++++++++++++++++++\n wt-status.c | 2 ++\n 2 files changed, 57 insertions(+)\n create mode 100755 t/t7527-status-color-pipe.sh\n\ndiff --git a/t/t7527-status-color-pipe.sh b/t/t7527-status-color-pipe.sh\nnew file mode 100755\nindex 00000000000..88df03ae066\n--- /dev/null\n+++ b/t/t7527-status-color-pipe.sh\n@@ -0,0 +1,55 @@\n+#!/bin/sh\n+\n+test_description='git status color option'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo 1 >original &&\n+\tgit add .\n+'\n+\n+# Normal git status does not pipe colors\n+test_expect_success 'git status' '\n+\tgit status >raw &&\n+\ttest_decode_color out &&\n+\tgrep \"original$\" out\n+'\n+\n+# Test color.status=never (expect same as above)\n+test_expect_success 'git -c color.status=never status' '\n+\tgit -c color.status=never status >raw &&\n+\ttest_decode_color out &&\n+\tgrep \"original$\" out\n+'\n+\n+# Test color.status=always\n+test_expect_success 'git -c color.status=always status' '\n+\tgit -c color.status=always status >raw &&\n+\ttest_decode_color out &&\n+\tgrep \"original$\" out\n+'\n+\n+# Test verbose (default)\n+test_expect_success 'git status -v' '\n+\tgit status -v >raw &&\n+\ttest_decode_color out &&\n+\tgrep \"+1\" out\n+'\n+\n+# Test verbose color.status=never\n+test_expect_success 'git -c color.status=never status -v' '\n+\tgit -c color.status=never status -v >raw &&\n+\ttest_decode_color out &&\n+\tgrep \"+1\" out\n+'\n+\n+# Test verbose color.status=always\n+test_expect_success 'git -c color.status=always status -v' '\n+\tgit -c color.status=always status -v >raw &&\n+\ttest_decode_color out &&\n+\tgrep \"@@ -0,0 +1 @@\" out &&\n+\tgrep \"GREEN>+1\" out\n+'\n+\n+test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 0c8287a023e..1e9c899a7b2 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1064,6 +1064,8 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n \tif (s->fp != stdout) {\n \t\trev.diffopt.use_color = 0;\n \t\twt_status_add_cut_line(s->fp);\n+\t} else {\n+\t\trev.diffopt.use_color = s->use_color;\n \t}\n \tif (s->verbose > 1 && s->committable) {\n \t\t/* print_updated() printed a header, so do we */\n\nbase-commit: e6362826a0409539642a5738db61827e5978e2e4\n-- \ngitgitgadget\n"},{"id":"416111","messageId":"xmqqa6skbw9u.fsf@gitster.c.googlers.com","threadId":"55099","inReplyTo":"pull.954.git.git.1612388043875.gitgitgadget@gmail.com","subject":"Re: [PATCH] status: fix verbose status coloring inconsistency","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-03T22:51:09Z","receivedAt":"2021-02-03T22:51:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lance Ward via GitGitGadget\" writes:\n\n> From: Lance Ward \n>\n> Currently setting color.status=always results in a colored diff when\n\nOur log message begins with the description of the current status,\nso \"Currently\" is not something you need to say.\n\nWhat command with what options? \"git status\" does not even show\n\"diff\" at least by default. \n\n ... goes, experiments, and guesses what the poster means ...\n\nPerhaps you meant something like this:\n\n status: honor color.status=always when sending diff output to non tty\n\n \"git status --verbose\" shows the patch in color by default (in\n addition to the list of added, modified, and/or deleted paths)\n when the output goes to a tty. With color.status configuration\n set to 'always' and sending the output to a non-tty, the list of\n paths are colored as expected, but the patch output loses colors.\n\nAnd then after the description of the current status, you give\norders to a patch monkey to fix the code \"to be like so\".\n\n This is because the code did not pass the settings read from the\n configuration and the command line to the underlying machinery\n that generates the patch output. Fix it to do so.\n\n> going stdout, but an uncolored diff when going to other files or piping\n> to other commands such as less or more. This patch fixes this and now\n> color.status=always implies color.diff=always regardless of the output\n> location.\n>\n> Signed-off-by: Lance Ward \n> ---\n\nEric may deserve Helped-by: mention.\n\n> t/t7527-status-color-pipe.sh | 55 ++++++++++++++++++++++++++++++++++++\n\nDon't we already have test that checks \"status\" output including its\ncoloring already? I'd rather see us adding to existing test script,\nthan allocating a new number for a small subset of features being\ntested for a command. After all, test numbers are limited resource.\n\n> +test_expect_success setup '\n> +\techo 1 >original &&\n> +\tgit add .\n> +'\n> +\n> +# Normal git status does not pipe colors\n\nWhat does \"pipe colors\" mean? \"color its output on a non-tty\", you mean?\n\n> +test_expect_success 'git status' '\n> +\tgit status >raw &&\n> +\ttest_decode_color out &&\n> +\tgrep \"original$\" out\n> +'\n\nNot \"new file: *original$\" or something less false-positive prone?\n\n> +# Test color.status=never (expect same as above)\n> +test_expect_success 'git -c color.status=never status' '\n> +\tgit -c color.status=never status >raw &&\n> +\ttest_decode_color out &&\n> +\tgrep \"original$\" out\n> +'\n> +\n\nWould it make sense to have tests for color.status=true, I wonder.\nIt requires tty to actually \"see\" the colors output but sending\nthe output to tty is the normal use case, so we should care...\n\n> +# Test color.status=always\n> +test_expect_success 'git -c color.status=always status' '\n> +\tgit -c color.status=always status >raw &&\n> +\ttest_decode_color out &&\n> +\tgrep \"original$\" out\n> +'\n\nOK. I understand that this passes without the patch below.\n\n> +# Test verbose (default)\n> +test_expect_success 'git status -v' '\n> +\tgit status -v >raw &&\n> +\ttest_decode_color out &&\n> +\tgrep \"+1\" out\n> +'\n\nI think you meant to catch the new contents \"1\" stored in the file\nwhose name is \"original\", but this will hit the hunk header, no?\n\n @@ -0,0 +1 @@\n +1\n\nIOW, the grep patterns throughout this patch may be a bit too loose\nand prone to false hits.\n\n> +# Test verbose color.status=never\n> +test_expect_success 'git -c color.status=never status -v' '\n> +\tgit -c color.status=never status -v >raw &&\n> +\ttest_decode_color out &&\n> +\tgrep \"+1\" out\n> +'\n> +\n> +# Test verbose color.status=always\n> +test_expect_success 'git -c color.status=always status -v' '\n> +\tgit -c color.status=always status -v >raw &&\n> +\ttest_decode_color out &&\n> +\tgrep \"@@ -0,0 +1 @@\" out &&\n> +\tgrep \"GREEN>+1\" out\n> +'\n\nIs the missing open bra \"<\" before GREEN intended?\n\nAre we forcing the standard palette?\n\n> +test_done\n> diff --git a/wt-status.c b/wt-status.c\n> index 0c8287a023e..1e9c899a7b2 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1064,6 +1064,8 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n> \tif (s->fp != stdout) {\n> \t\trev.diffopt.use_color = 0;\n> \t\twt_status_add_cut_line(s->fp);\n> +\t} else {\n> +\t\trev.diffopt.use_color = s->use_color;\n> \t}\n> \tif (s->verbose > 1 && s->committable) {\n> \t\t/* print_updated() printed a header, so do we */\n>\n> base-commit: e6362826a0409539642a5738db61827e5978e2e4\n\nThanks.\n"},{"id":"416122","messageId":"CACPHW2WxXKaDjMY3KdxAQAi82V3_crN763bGQa4aWcJttZ+_cQ@mail.gmail.com","threadId":"55099","inReplyTo":"xmqqa6skbw9u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] status: fix verbose status coloring inconsistency","fromName":"Lance Ward","fromEmail":"ljward10@gmail.com","sentAt":"2021-02-04T00:44:03Z","receivedAt":"2021-02-04T00:45:43Z","isPatch":true,"sender":{"key":"ljward10@gmail.com","avatar":null},"body":"On Wed, Feb 3, 2021 at 4:51 PM Junio C Hamano wrote:\n>\n> \"Lance Ward via GitGitGadget\" writes:\n>\n> > From: Lance Ward \n> >\n> > Currently setting color.status=always results in a colored diff when\n>\n> Our log message begins with the description of the current status,\n> so \"Currently\" is not something you need to say.\n>\n> What command with what options? \"git status\" does not even show\n> \"diff\" at least by default.\n>\n> ... goes, experiments, and guesses what the poster means ...\n\nI'm disappointed by your tone...\n\nI'll go ahead and close my pull requests, if someone else wants\nto pick them up that's fine with me.\n\n>\n> Perhaps you meant something like this:\n>\n> status: honor color.status=always when sending diff output to non tty\n>\n> \"git status --verbose\" shows the patch in color by default (in\n> addition to the list of added, modified, and/or deleted paths)\n> when the output goes to a tty. With color.status configuration\n> set to 'always' and sending the output to a non-tty, the list of\n> paths are colored as expected, but the patch output loses colors.\n>\n> And then after the description of the current status, you give\n> orders to a patch monkey to fix the code \"to be like so\".\n>\n> This is because the code did not pass the settings read from the\n> configuration and the command line to the underlying machinery\n> that generates the patch output. Fix it to do so.\n>\n> > going stdout, but an uncolored diff when going to other files or piping\n> > to other commands such as less or more. This patch fixes this and now\n> > color.status=always implies color.diff=always regardless of the output\n> > location.\n> >\n> > Signed-off-by: Lance Ward \n> > ---\n>\n> Eric may deserve Helped-by: mention.\n>\n> > t/t7527-status-color-pipe.sh | 55 ++++++++++++++++++++++++++++++++++++\n>\n> Don't we already have test that checks \"status\" output including its\n> coloring already? I'd rather see us adding to existing test script,\n> than allocating a new number for a small subset of features being\n> tested for a command. After all, test numbers are limited resource.\n>\n> > +test_expect_success setup '\n> > + echo 1 >original &&\n> > + git add .\n> > +'\n> > +\n> > +# Normal git status does not pipe colors\n>\n> What does \"pipe colors\" mean? \"color its output on a non-tty\", you mean?\n>\n> > +test_expect_success 'git status' '\n> > + git status >raw &&\n> > + test_decode_color out &&\n> > + grep \"original$\" out\n> > +'\n>\n> Not \"new file: *original$\" or something less false-positive prone?\n>\n> > +# Test color.status=never (expect same as above)\n> > +test_expect_success 'git -c color.status=never status' '\n> > + git -c color.status=never status >raw &&\n> > + test_decode_color out &&\n> > + grep \"original$\" out\n> > +'\n> > +\n>\n> Would it make sense to have tests for color.status=true, I wonder.\n> It requires tty to actually \"see\" the colors output but sending\n> the output to tty is the normal use case, so we should care...\n>\n> > +# Test color.status=always\n> > +test_expect_success 'git -c color.status=always status' '\n> > + git -c color.status=always status >raw &&\n> > + test_decode_color out &&\n> > + grep \"original$\" out\n> > +'\n>\n> OK. I understand that this passes without the patch below.\n>\n> > +# Test verbose (default)\n> > +test_expect_success 'git status -v' '\n> > + git status -v >raw &&\n> > + test_decode_color out &&\n> > + grep \"+1\" out\n> > +'\n>\n> I think you meant to catch the new contents \"1\" stored in the file\n> whose name is \"original\", but this will hit the hunk header, no?\n>\n> @@ -0,0 +1 @@\n> +1\n>\n> IOW, the grep patterns throughout this patch may be a bit too loose\n> and prone to false hits.\n>\n> > +# Test verbose color.status=never\n> > +test_expect_success 'git -c color.status=never status -v' '\n> > + git -c color.status=never status -v >raw &&\n> > + test_decode_color out &&\n> > + grep \"+1\" out\n> > +'\n> > +\n> > +# Test verbose color.status=always\n> > +test_expect_success 'git -c color.status=always status -v' '\n> > + git -c color.status=always status -v >raw &&\n> > + test_decode_color out &&\n> > + grep \"@@ -0,0 +1 @@\" out &&\n> > + grep \"GREEN>+1\" out\n> > +'\n>\n> Is the missing open bra \"<\" before GREEN intended?\n>\n> Are we forcing the standard palette?\n>\n> > +test_done\n> > diff --git a/wt-status.c b/wt-status.c\n> > index 0c8287a023e..1e9c899a7b2 100644\n> > --- a/wt-status.c\n> > +++ b/wt-status.c\n> > @@ -1064,6 +1064,8 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n> > if (s->fp != stdout) {\n> > rev.diffopt.use_color = 0;\n> > wt_status_add_cut_line(s->fp);\n> > + } else {\n> > + rev.diffopt.use_color = s->use_color;\n> > }\n> > if (s->verbose > 1 && s->committable) {\n> > /* print_updated() printed a header, so do we */\n> >\n> > base-commit: e6362826a0409539642a5738db61827e5978e2e4\n>\n> Thanks.\n"},{"id":"416244","messageId":"CAPig+cSqhopq1ibWFYuWwV=fET+MYdyusvwGdcvj-pdy6LXbGQ@mail.gmail.com","threadId":"55099","inReplyTo":"CACPHW2WxXKaDjMY3KdxAQAi82V3_crN763bGQa4aWcJttZ+_cQ@mail.gmail.com","subject":"Re: [PATCH] status: fix verbose status coloring inconsistency","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-05T07:06:12Z","receivedAt":"2021-02-05T07:07:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 3, 2021 at 7:44 PM Lance Ward wrote:\n> On Wed, Feb 3, 2021 at 4:51 PM Junio C Hamano wrote:\n> > Our log message begins with the description of the current status,\n> > so \"Currently\" is not something you need to say.\n>\n> I'm disappointed by your tone...\n>\n> I'll go ahead and close my pull requests, if someone else wants\n> to pick them up that's fine with me.\n\nIt is, unfortunately, easy to misinterpret a reviewer's neutral tone\nas being negative or as an attempt to shame the author. But be assured\nthat the goal of reviewers on this project is to help the patch author\nget the submission into proper shape for acceptance, and when Junio\ntakes the time to write such a comprehensive review, he does so\nbecause he sees promise in both the patches and in the author of the\npatches. A review as extensive as this one is intended to get the\nnewcomer up to speed quickly with local project conventions (such as\nhow commit messages are written) and to help land the patches with as\nfew revisions as possible since both submitter and reviewer time is\nvaluable.\n"},{"id":"416245","messageId":"CAPig+cRos+wGuwqbjuAV11kM5U0TPbBO9F4o4VJzQ4eZyxArMA@mail.gmail.com","threadId":"55099","inReplyTo":"xmqqa6skbw9u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] status: fix verbose status coloring inconsistency","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-05T07:17:04Z","receivedAt":"2021-02-05T07:18:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 3, 2021 at 5:51 PM Junio C Hamano wrote:\n> \"Lance Ward via GitGitGadget\" writes:\n> > t/t7527-status-color-pipe.sh | 55 ++++++++++++++++++++++++++++++++++++\n>\n> Don't we already have test that checks \"status\" output including its\n> coloring already? I'd rather see us adding to existing test script,\n> than allocating a new number for a small subset of features being\n> tested for a command. After all, test numbers are limited resource.\n\nt7508 seems like a good place for these tests.\n\n> > +test_expect_success 'git status' '\n> > + git status >raw &&\n> > + test_decode_color out &&\n> > + grep \"original$\" out\n> > +'\n>\n> Not \"new file: *original$\" or something less false-positive prone?\n\nThe \"new file:\" message is localized, so this `grep` will need to\nbecome `test_i18ngrep` again (as it was in the original submission) if\nthis approach is adopted, which is fine.\n\n> > +test_expect_success 'git -c color.status=never status' '\n> > + git -c color.status=never status >raw &&\n> > + test_decode_color out &&\n> > + grep \"original$\" out\n> > +'\n>\n> Would it make sense to have tests for color.status=true, I wonder.\n> It requires tty to actually \"see\" the colors output but sending\n> the output to tty is the normal use case, so we should care...\n\nI wondered if `color.status=true` might already be tested by t7508 but\napparently it only checks `auto`.\n\n> > +test_expect_success 'git -c color.status=always status -v' '\n> > + git -c color.status=always status -v >raw &&\n> > + test_decode_color out &&\n> > + grep \"@@ -0,0 +1 @@\" out &&\n> > + grep \"GREEN>+1\" out\n> > +'\n>\n> Are we forcing the standard palette?\n\nAs this is a stand-alone test script with a well-controlled initial\nconfiguration, I expect it would be using the default palette. t7508\ndoes assign a custom palette for `color.status` but not, apparently,\nfor `color.diff`, so it presumably should be okay there, as well.\n"}]}