status: fix verbose status coloring inconsistency
PATCH 1/1 status: fix verbose status coloring inconsistency
From: Lance Ward <[email protected]>
Currently setting color.status=always results in a colored diff when going stdout, but an uncolored diff when going to other files or piping to other commands such as less or more. This patch fixes this and now color.status=always implies color.diff=always regardless of the output location.
---
status: fix verbose status coloring inconsistency
Currently setting color.status=always results in a colored diff when going stdout, but an uncolored diff when going to other files or piping to other commands such as less or more. This patch fixes this and now color.status=always implies color.diff=always regardless of the output location.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-954%2Fljward10%2Flw-fix-status-coloring-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-954/ljward10/lw-fix-status-coloring-v1
Pull-Request: https://github.com/git/git/pull/954
t/t7527-status-color-pipe.sh | 55 ++++++++++++++++++++++++++++++++++++ wt-status.c | 2 ++ 2 files changed, 57 insertions(+) create mode 100755 t/t7527-status-color-pipe.sh
t/t7527-status-color-pipe.sh new file +55 -0
@@ -0,0 +1,55 @@ +#!/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 color.status=never (expect same as above) +test_expect_success 'git -c color.status=never status' ' + git -c color.status=never status >raw && + test_decode_color <raw >out && + grep "original$" out +' + +# Test color.status=always +test_expect_success 'git -c color.status=always status' ' + git -c color.status=always status >raw && + test_decode_color <raw >out && + grep "original<RESET>$" out +' + +# Test verbose (default) +test_expect_success 'git status -v' ' + git status -v >raw && + test_decode_color <raw >out && + grep "+1" out +' + +# Test verbose color.status=never +test_expect_success 'git -c color.status=never status -v' ' + git -c color.status=never status -v >raw && + test_decode_color <raw >out && + grep "+1" out +' + +# Test verbose color.status=always +test_expect_success 'git -c color.status=always status -v' ' + git -c color.status=always status -v >raw && + test_decode_color <raw >out && + grep "<CYAN>@@ -0,0 +1 @@<RESET>" out && + grep "GREEN>+<RESET><GREEN>1<RESET>" out +' + +test_done
wt-status.c +2 -0
@@ -1064,6 +1064,8 @@ static void wt_longstatus_print_verbose(struct wt_status *s)if (s->fp != stdout) { rev.diffopt.use_color =0 ;wt_status_add_cut_line (s->fp); + }else { + rev.diffopt.use_color = s->use_color; }if (s->verbose >1 && s->committable) {/* print_updated() printed a header, so do we */
base-commit: e6362826a0409539642a5738db61827e5978e2e4
Signature · 1 line
gitgitgadget
Quoted 3 lines · Lance Ward via GitGitGadget
From: Lance Ward <[email protected]>
Currently setting color.status=always results in a colored diff when
Our log message begins with the description of the current status, so "Currently" is not something you need to say.
What command with what options? "git status" does not even show "diff" at least by default.
... goes, experiments, and guesses what the poster means ...
Perhaps you meant something like this:
status: honor color.status=always when sending diff output to non tty
"git status --verbose" shows the patch in color by default (in addition to the list of added, modified, and/or deleted paths) when the output goes to a tty. With color.status configuration set to 'always' and sending the output to a non-tty, the list of paths are colored as expected, but the patch output loses colors.
And then after the description of the current status, you give orders to a patch monkey to fix the code "to be like so".
This is because the code did not pass the settings read from the configuration and the command line to the underlying machinery that generates the patch output. Fix it to do so.
Quoted 7 lines
going stdout, but an uncolored diff when going to other files or piping to other commands such as less or more. This patch fixes this and now color.status=always implies color.diff=always regardless of the output location.
Signed-off-by Lance Ward <[email protected]>---
Eric may deserve Helped-by: mention.
Quoted 1 line
t/t7527-status-color-pipe.sh | 55 ++++++++++++++++++++++++++++++++++++
Don't we already have test that checks "status" output including its coloring already? I'd rather see us adding to existing test script, than allocating a new number for a small subset of features being tested for a command. After all, test numbers are limited resource.
t/t7527-status-color-pipe.sh line 12
Quoted 6 lines
t/t7527-status-color-pipe.sh new file +6
-0+test_expect_success setup ' + echo 1 >original && + git add . +' + +# Normal git status does not pipe colors
What does "pipe colors" mean? "color its output on a non-tty", you mean?
t/t7527-status-color-pipe.sh line 17
Quoted 5 lines
t/t7527-status-color-pipe.sh new file +5
-0+test_expect_success 'git status' ' + git status >raw && + test_decode_color <raw >out && + grep "original$" out +'
Not "new file: *original$" or something less false-positive prone?
t/t7527-status-color-pipe.sh line 25
Quoted 7 lines
t/t7527-status-color-pipe.sh new file +7
-0+# Test color.status=never (expect same as above) +test_expect_success 'git -c color.status=never status' ' + git -c color.status=never status >raw && + test_decode_color <raw >out && + grep "original$" out +' +
Would it make sense to have tests for color.status=true, I wonder. It requires tty to actually "see" the colors output but sending the output to tty is the normal use case, so we should care...
t/t7527-status-color-pipe.sh line 31
Quoted 6 lines
t/t7527-status-color-pipe.sh new file +6
-0+# Test color.status=always +test_expect_success 'git -c color.status=always status' ' + git -c color.status=always status >raw && + test_decode_color <raw >out && + grep "original<RESET>$" out +'
OK. I understand that this passes without the patch below.
t/t7527-status-color-pipe.sh line 38
Quoted 6 lines
t/t7527-status-color-pipe.sh new file +6
-0+# Test verbose (default) +test_expect_success 'git status -v' ' + git status -v >raw && + test_decode_color <raw >out && + grep "+1" out +'
I think you meant to catch the new contents "1" stored in the file whose name is "original", but this will hit the hunk header, no?
@@ -0,0 +1 @@
+1IOW, the grep patterns throughout this patch may be a bit too loose and prone to false hits.
Quoted 4 lines
t/t7527-status-color-pipe.sh new file +4
-0+# Test verbose color.status=never +test_expect_success 'git -c color.status=never status -v' ' + git -c color.status=never status -v >raw && + test_decode_color <raw >out &&
t/t7527-status-color-pipe.sh line 53
Quoted 10 lines
t/t7527-status-color-pipe.sh new file +10
-0+ grep "+1" out +' + +# Test verbose color.status=always +test_expect_success 'git -c color.status=always status -v' ' + git -c color.status=always status -v >raw && + test_decode_color <raw >out && + grep "<CYAN>@@ -0,0 +1 @@<RESET>" out && + grep "GREEN>+<RESET><GREEN>1<RESET>" out +'
Is the missing open bra "<" before GREEN intended?
Are we forcing the standard palette?
Quoted 15 lines
t/t7527-status-color-pipe.sh new file +1
-0+test_donewt-status.c +2
-0@@ -1064,6 +1064,8 @@ static void wt_longstatus_print_verbose(struct wt_status *s)if (s->fp != stdout) { rev.diffopt.use_color =0 ;wt_status_add_cut_line (s->fp); + }else { + rev.diffopt.use_color = s->use_color; }if (s->verbose >1 && s->committable) {/* print_updated() printed a header, so do we */
Quoted 1 line
base-commit:
e6362826a0409539642a5738db61827e5978e2e4
Thanks.
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Lance Ward via GitGitGadget
From: Lance Ward <[email protected]>
Currently setting color.status=always results in a colored diff when
Quoted 7 lines
Our log message begins with the description of the current status, so "Currently" is not something you need to say.
What command with what options? "git status" does not even show "diff" at least by default.
... goes, experiments, and guesses what the poster means ...
I'm disappointed by your tone...
I'll go ahead and close my pull requests, if someone else wants to pick them up that's fine with me.
Quoted 126 lines
Perhaps you meant something like this:
status: honor color.status=always when sending diff output to non tty
"git status --verbose" shows the patch in color by default (in addition to the list of added, modified, and/or deleted paths) when the output goes to a tty. With color.status configuration set to 'always' and sending the output to a non-tty, the list of paths are colored as expected, but the patch output loses colors.
And then after the description of the current status, you give orders to a patch monkey to fix the code "to be like so".
This is because the code did not pass the settings read from the configuration and the command line to the underlying machinery that generates the patch output. Fix it to do so.
Quoted 7 lines
going stdout, but an uncolored diff when going to other files or piping to other commands such as less or more. This patch fixes this and now color.status=always implies color.diff=always regardless of the output location.
Signed-off-by Lance Ward <[email protected]>---
Eric may deserve Helped-by: mention.
Quoted 1 line
t/t7527-status-color-pipe.sh | 55 ++++++++++++++++++++++++++++++++++++Don't we already have test that checks "status" output including its coloring already? I'd rather see us adding to existing test script, than allocating a new number for a small subset of features being tested for a command. After all, test numbers are limited resource.
t/t7527-status-color-pipe.sh line 12
Quoted 6 lines
t/t7527-status-color-pipe.sh new file +6
-0+test_expect_success setup ' + echo 1 >original && + git add . +' + +# Normal git status does not pipe colorsWhat does "pipe colors" mean? "color its output on a non-tty", you mean?
t/t7527-status-color-pipe.sh line 17
Quoted 5 lines
t/t7527-status-color-pipe.sh new file +5
-0+test_expect_success 'git status' ' + git status >raw && + test_decode_color <raw >out && + grep "original$" out +'Not "new file: *original$" or something less false-positive prone?
t/t7527-status-color-pipe.sh line 25
Quoted 7 lines
t/t7527-status-color-pipe.sh new file +7
-0+# Test color.status=never (expect same as above) +test_expect_success 'git -c color.status=never status' ' + git -c color.status=never status >raw && + test_decode_color <raw >out && + grep "original$" out +' +Would it make sense to have tests for color.status=true, I wonder. It requires tty to actually "see" the colors output but sending the output to tty is the normal use case, so we should care...
t/t7527-status-color-pipe.sh line 31
Quoted 6 lines
t/t7527-status-color-pipe.sh new file +6
-0+# Test color.status=always +test_expect_success 'git -c color.status=always status' ' + git -c color.status=always status >raw && + test_decode_color <raw >out && + grep "original<RESET>$" out +'OK. I understand that this passes without the patch below.
t/t7527-status-color-pipe.sh line 38
Quoted 6 lines
t/t7527-status-color-pipe.sh new file +6
-0+# Test verbose (default) +test_expect_success 'git status -v' ' + git status -v >raw && + test_decode_color <raw >out && + grep "+1" out +'I think you meant to catch the new contents "1" stored in the file whose name is "original", but this will hit the hunk header, no?
@@ -0,0 +1 @@ +1IOW, the grep patterns throughout this patch may be a bit too loose and prone to false hits.
Quoted 4 lines
t/t7527-status-color-pipe.sh new file +4
-0+# Test verbose color.status=never +test_expect_success 'git -c color.status=never status -v' ' + git -c color.status=never status -v >raw && + test_decode_color <raw >out &&t/t7527-status-color-pipe.sh line 53
Quoted 10 lines
t/t7527-status-color-pipe.sh new file +10
-0+ grep "+1" out +' + +# Test verbose color.status=always +test_expect_success 'git -c color.status=always status -v' ' + git -c color.status=always status -v >raw && + test_decode_color <raw >out && + grep "<CYAN>@@ -0,0 +1 @@<RESET>" out && + grep "GREEN>+<RESET><GREEN>1<RESET>" out +'Is the missing open bra "<" before GREEN intended?
Are we forcing the standard palette?
Quoted 15 lines
t/t7527-status-color-pipe.sh new file +1
-0+test_donewt-status.c +2
-0@@ -1064,6 +1064,8 @@ static void wt_longstatus_print_verbose(struct wt_status *s)if (s->fp != stdout) { rev.diffopt.use_color =0 ;wt_status_add_cut_line (s->fp); + }else { + rev.diffopt.use_color = s->use_color; }if (s->verbose >1 && s->committable) {/* print_updated() printed a header, so do we */ Quoted 1 line
base-commit:
e6362826a0409539642a5738db61827e5978e2e4Thanks.
Quoted 8 lines · Lance Ward
Quoted 2 lines · Junio C Hamano
Our log message begins with the description of the current status, so "Currently" is not something you need to say.
I'm disappointed by your tone...
I'll go ahead and close my pull requests, if someone else wants to pick them up that's fine with me.
It is, unfortunately, easy to misinterpret a reviewer's neutral tone as being negative or as an attempt to shame the author. But be assured that the goal of reviewers on this project is to help the patch author get the submission into proper shape for acceptance, and when Junio takes the time to write such a comprehensive review, he does so because he sees promise in both the patches and in the author of the patches. A review as extensive as this one is intended to get the newcomer up to speed quickly with local project conventions (such as how commit messages are written) and to help land the patches with as few revisions as possible since both submitter and reviewer time is valuable.
Quoted 7 lines · Junio C Hamano
Quoted 1 line · Lance Ward via GitGitGadget
t/t7527-status-color-pipe.sh | 55 ++++++++++++++++++++++++++++++++++++Don't we already have test that checks "status" output including its coloring already? I'd rather see us adding to existing test script, than allocating a new number for a small subset of features being tested for a command. After all, test numbers are limited resource.
t7508 seems like a good place for these tests.
Quoted 7 lines
t/t7527-status-color-pipe.sh line 17
Quoted 5 lines
t/t7527-status-color-pipe.sh new file +5
-0+test_expect_success 'git status' ' + git status >raw && + test_decode_color <raw >out && + grep "original$" out +'Not "new file: *original$" or something less false-positive prone?
The "new file:" message is localized, so this grep will need to become test_i18ngrep again (as it was in the original submission) if this approach is adopted, which is fine.
Quoted 9 lines
t/t7527-status-color-pipe.sh line 24
Quoted 5 lines
t/t7527-status-color-pipe.sh new file +5
-0+test_expect_success 'git -c color.status=never status' ' + git -c color.status=never status >raw && + test_decode_color <raw >out && + grep "original$" out +'Would it make sense to have tests for color.status=true, I wonder. It requires tty to actually "see" the colors output but sending the output to tty is the normal use case, so we should care...
I wondered if color.status=true might already be tested by t7508 but apparently it only checks auto.
Quoted 8 lines
t/t7527-status-color-pipe.sh line 53
Quoted 6 lines
t/t7527-status-color-pipe.sh new file +6
-0+test_expect_success 'git -c color.status=always status -v' ' + git -c color.status=always status -v >raw && + test_decode_color <raw >out && + grep "<CYAN>@@ -0,0 +1 @@<RESET>" out && + grep "GREEN>+<RESET><GREEN>1<RESET>" out +'Are we forcing the standard palette?
As this is a stand-alone test script with a well-controlled initial configuration, I expect it would be using the default palette. t7508 does assign a custom palette for color.status but not, apparently, for color.diff, so it presumably should be okay there, as well.