{"thread":{"id":"55076","subject":"[PATCH] status: learn --color for piping colored output","startedAt":"2021-01-30T15:50:50Z","lastAt":"2021-02-05T06:24:56Z","messageCount":8,"participants":["Lance Ward via GitGitGadget","Eric Sunshine","Lance Ward"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"415655","messageId":"pull.854.git.1612021544723.gitgitgadget@gmail.com","threadId":"55076","inReplyTo":null,"subject":"[PATCH] status: learn --color for piping colored output","fromName":"Lance Ward via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-30T15:45:44Z","receivedAt":"2021-01-30T15:50:50Z","isPatch":true,"sender":{"key":"ljward10@gmail.com","avatar":null},"body":"From: Lance Ward <ljward10@gmail.com>\n\nMany users like to pipe colored results of git status to other commands\nsuch as more or less, but by default colors are lost when piping without\nchanging the user's git configuration.  Many other commands such as diff,\nshow, log and grep have a --color option to easily override this behavior.\nThis allows the status command to have a similar --color option providing\na simpler mechanism for temporarily forcing piped colored output.\n\nSigned-off-by: Lance Ward <ljward10@gmail.com>\n---\n    status: learn --color for piping colored output\n    \n    Many users like to pipe colored results of git status to other commands\n    such as more or less, but by default colors are lost when piping without\n    changing the user's git configuration. Many other commands such as diff,\n    show, log and grep have a --color option to easily override this\n    behavior. This allows the status command to have a similar --color\n    option providing a simpler mechanism for temporarily forcing piped\n    colored output.\n    \n    Signed-off-by: Lance Ward ljward10@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-854%2Fljward10%2Flw-add-status-color-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-854/ljward10/lw-add-status-color-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/854\n\n builtin/commit.c             |  7 ++++\n diff.c                       |  5 +++\n diff.h                       |  1 +\n t/t7527-status-color-pipe.sh | 69 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 82 insertions(+)\n create mode 100755 t/t7527-status-color-pipe.sh\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 739110c5a7f..1579f7cc9ed 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1355,6 +1355,7 @@ static int git_status_config(const char *k, const char *v, void *cb)\n int cmd_status(int argc, const char **argv, const char *prefix)\n {\n \tstatic int no_renames = -1;\n+\tstatic int use_color = GIT_COLOR_AUTO;\n \tstatic const char *rename_score_arg = (const char *)-1;\n \tstatic struct wt_status s;\n \tunsigned int progress_flag = 0;\n@@ -1378,6 +1379,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\t\t    STATUS_FORMAT_LONG),\n \t\tOPT_BOOL('z', \"null\", &s.null_termination,\n \t\t\t N_(\"terminate entries with NUL\")),\n+\t\tOPT__COLOR(&use_color, N_(\"use colored output\")),\n \t\t{ OPTION_STRING, 'u', \"untracked-files\", &untracked_files_arg,\n \t\t  N_(\"mode\"),\n \t\t  N_(\"show untracked files, optional modes: all, normal, no. (Default: all)\"),\n@@ -1410,6 +1412,11 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \thandle_untracked_files_arg(&s);\n \thandle_ignored_arg(&s);\n \n+\tif (use_color != GIT_COLOR_AUTO) {\n+\t\ts.use_color=use_color;\n+\t\tset_diff_color(use_color);\n+\t}\n+\n \tif (s.show_ignored_mode == SHOW_MATCHING_IGNORED &&\n \t    s.show_untracked_files == SHOW_NO_UNTRACKED_FILES)\n \t\tdie(_(\"Unsupported combination of ignored and untracked-files arguments\"));\ndiff --git a/diff.c b/diff.c\nindex 69e3bc00ed8..fe7ffce6803 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -261,6 +261,11 @@ void init_diff_ui_defaults(void)\n \tdiff_detect_rename_default = DIFF_DETECT_RENAME;\n }\n \n+void set_diff_color(int use_color)\n+{\n+       diff_use_color_default = use_color;\n+}\n+\n int git_diff_heuristic_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"diff.indentheuristic\"))\ndiff --git a/diff.h b/diff.h\nindex 2ff2b1c7f2c..10196d9b040 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -501,6 +501,7 @@ int parse_long_opt(const char *opt, const char **argv,\n int git_diff_basic_config(const char *var, const char *value, void *cb);\n int git_diff_heuristic_config(const char *var, const char *value, void *cb);\n void init_diff_ui_defaults(void);\n+void set_diff_color(int use_color);\n int git_diff_ui_config(const char *var, const char *value, void *cb);\n #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n #define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts)\ndiff --git a/t/t7527-status-color-pipe.sh b/t/t7527-status-color-pipe.sh\nnew file mode 100755\nindex 00000000000..ee4ab2ea821\n--- /dev/null\n+++ b/t/t7527-status-color-pipe.sh\n@@ -0,0 +1,69 @@\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 >out &&\n+\ttest_i18ngrep \"original$\" out\n+'\n+\n+# Test new color option with never (expect same as above)\n+test_expect_success 'git status --color=never' '\n+\tgit status --color=never >out &&\n+\ttest_i18ngrep \"original$\" out\n+'\n+\n+# Test new color (default is always)\n+test_expect_success 'git status --color' '\n+\tgit status --color |\n+\ttest_decode_color >out &&\n+\ttest_i18ngrep \"original<RESET>$\" out\n+'\n+\n+# Test new color option with always\n+test_expect_success 'git status --color=always' '\n+\tgit status --color=always |\n+\ttest_decode_color >out &&\n+\ttest_i18ngrep \"original<RESET>$\" out\n+'\n+\n+# Test verbose (default)\n+test_expect_success 'git status -v' '\n+\tgit status -v |\n+\ttest_decode_color >out &&\n+\ttest_i18ngrep \"+1\" out\n+'\n+\n+# Test verbose --color=never\n+test_expect_success 'git status -v --color=never' '\n+\tgit status -v --color=never |\n+\ttest_decode_color >out &&\n+\ttest_i18ngrep \"+1\" out\n+'\n+\n+# Test verbose --color (default always)\n+test_expect_success 'git status -v --color' '\n+\tgit status -v --color |\n+\ttest_decode_color >out &&\n+\ttest_i18ngrep \"<CYAN>@@ -0,0 +1 @@<RESET>\" out &&\n+\ttest_i18ngrep \"<GREEN>+<RESET><GREEN>1<RESET>\" out\n+'\n+\n+test_done\n+# Test verbose --color=always\n+test_expect_success 'git status -v --color=always' '\n+\tgit status -v --color=always |\n+\ttest_decode_color >out &&\n+\ttest_i18ngrep \"<CYAN>@@ -0,0 +1 @@<RESET>\" out &&\n+\ttest_i18ngrep \"GREEN>+<RESET><GREEN>1<RESET>\" out\n+'\n+\n+test_done\n\nbase-commit: e6362826a0409539642a5738db61827e5978e2e4\n-- \ngitgitgadget\n"},{"id":"415687","messageId":"CAPig+cQMn6oc4Jh=gb1jNfArXJBYhPRaSzJJvvbvprit6_OC0g@mail.gmail.com","threadId":"55076","inReplyTo":"pull.854.git.1612021544723.gitgitgadget@gmail.com","subject":"Re: [PATCH] status: learn --color for piping colored output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-01-31T07:31:34Z","receivedAt":"2021-01-31T07:35:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jan 30, 2021 at 10:51 AM Lance Ward via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> Many users like to pipe colored results of git status to other commands\n> such as more or less, but by default colors are lost when piping without\n> changing the user's git configuration.  Many other commands such as diff,\n> show, log and grep have a --color option to easily override this behavior.\n> This allows the status command to have a similar --color option providing\n> a simpler mechanism for temporarily forcing piped colored output.\n\nThanks, makes sense.\n\n> Signed-off-by: Lance Ward <ljward10@gmail.com>\n> ---\n>  builtin/commit.c             |  7 ++++\n>  diff.c                       |  5 +++\n>  diff.h                       |  1 +\n>  t/t7527-status-color-pipe.sh | 69 ++++++++++++++++++++++++++++++++++++\n\nAs this is introducing a new --color option to `git status`, it should\nbe accompanied by an update to Documentation/git-status.txt.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> @@ -1410,6 +1412,11 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> +       if (use_color != GIT_COLOR_AUTO) {\n> +               s.use_color=use_color;\n> +               set_diff_color(use_color);\n> +       }\n> diff --git a/diff.c b/diff.c\n> @@ -261,6 +261,11 @@ void init_diff_ui_defaults(void)\n> +void set_diff_color(int use_color)\n> +{\n> +       diff_use_color_default = use_color;\n> +}\n> diff --git a/diff.h b/diff.h\n> @@ -501,6 +501,7 @@ int parse_long_opt(const char *opt, const char **argv,\n> +void set_diff_color(int use_color);\n\nThis new API for setting `diff_use_color_default` feels a bit too\nquick-and-dirty and assumes that the caller has intimate knowledge\nabout when it is safe/correct to call the new function. Did you\nconsider the alternate approach of having wt-status functionality set\nthe appropriate diff_options.use_color value at the time it drives the\ndiff machinery? For instance, as a test, I added:\n\n    rev.diffopt.use_color = s->use_color;\n\nto the functions wt-status.c:wt_status_collect_changes_worktree(),\nwt_status_collect_changes_index(), and wt_longstatus_print_verbose(),\nso that the `use_color` value from the `struct wt_status *` supplied\nby commit.c:cmd_status() is automatically applied to the diff options.\n\n(Note that this was just a quick test. I dug through the code just\nenough to locate these functions as the likely correct place to set\ndiff_options.use_color, but didn't spend any time verifying that all\nthree functions need to be patched like this. I also didn't verify\nthat my changes won't stomp on --porcelain's explicit disabling of\ncolor, which is something that ought to be checked. There's also some\ncustom `use_color` handling going on in wt_longstatus_print_verbose()\nof which to be aware.)\n\n> diff --git a/t/t7527-status-color-pipe.sh b/t/t7527-status-color-pipe.sh\n> @@ -0,0 +1,69 @@\n> +# Normal git status does not pipe colors\n> +test_expect_success 'git status' '\n> +       git status >out &&\n> +       test_i18ngrep \"original$\" out\n> +'\n\nNone of the text being checked by any of the tests being added by this\nfile is subject to localization, so use of test_i18ngrep() is\nunwarranted. Use plain old `grep` instead here and elsewhere.\n\n> +# Test new color option with never (expect same as above)\n> +test_expect_success 'git status --color=never' '\n> +       git status --color=never >out &&\n> +       test_i18ngrep \"original$\" out\n> +'\n> +\n> +# Test new color (default is always)\n> +test_expect_success 'git status --color' '\n> +       git status --color |\n> +       test_decode_color >out &&\n> +       test_i18ngrep \"original<RESET>$\" out\n> +'\n\nIf someone introduces a bug which causes the non-colored tests to\nincorrectly emit color codes, then it will be easier to debug the\nproblem if you also pass the output through test_decode_color() even\nfor the non-colored tests rather than only for the expect-colored\ntests. Thus, I'd recommend calling test_decode_color() in all the\ntests, even if you don't expect color to be emitted.\n\nAlso, these days, we don't normally place a Git command upstream in a\npipe since its exit code will be lost. Instead, we capture the output\nto a file:\n\n    git status --color >raw &&\n    test_decode_color <raw >out &&\n    ...\n\n> +test_done\n> +# Test verbose --color=always\n> +test_expect_success 'git status -v --color=always' '\n> +       git status -v --color=always |\n> +       test_decode_color >out &&\n> +       test_i18ngrep \"<CYAN>@@ -0,0 +1 @@<RESET>\" out &&\n> +       test_i18ngrep \"GREEN>+<RESET><GREEN>1<RESET>\" out\n> +'\n> +\n> +test_done\n\nYou have one too many calls to test_done() in this fragment. I think\nyou only want to keep the final test_done() and remove the one prior\nto the last test.\n"},{"id":"415689","messageId":"CACPHW2X2UGAVmNM+cHXs6dwVfZbgLFZ0iUGU89h04H5czAt1Ww@mail.gmail.com","threadId":"55076","inReplyTo":"CAPig+cQMn6oc4Jh=gb1jNfArXJBYhPRaSzJJvvbvprit6_OC0g@mail.gmail.com","subject":"Re: [PATCH] status: learn --color for piping colored output","fromName":"Lance Ward","fromEmail":"ljward10@gmail.com","sentAt":"2021-01-31T18:49:27Z","receivedAt":"2021-01-31T20:14:30Z","isPatch":true,"sender":{"key":"ljward10@gmail.com","avatar":null},"body":"Hi Eric,\n\nThank you for the comments!  I made all your suggested changes, except\nfor one which I wanted to provide more context on and ask a question\nabout before proceeding.  Please see my responses below.\n\nOn Sun, Jan 31, 2021 at 1:31 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Sat, Jan 30, 2021 at 10:51 AM Lance Ward via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> > Many users like to pipe colored results of git status to other commands\n> > such as more or less, but by default colors are lost when piping without\n> > changing the user's git configuration.  Many other commands such as diff,\n> > show, log and grep have a --color option to easily override this behavior.\n> > This allows the status command to have a similar --color option providing\n> > a simpler mechanism for temporarily forcing piped colored output.\n>\n> Thanks, makes sense.\n>\n> > Signed-off-by: Lance Ward <ljward10@gmail.com>\n> > ---\n> >  builtin/commit.c             |  7 ++++\n> >  diff.c                       |  5 +++\n> >  diff.h                       |  1 +\n> >  t/t7527-status-color-pipe.sh | 69 ++++++++++++++++++++++++++++++++++++\n>\n> As this is introducing a new --color option to `git status`, it should\n> be accompanied by an update to Documentation/git-status.txt.\n\nI've added the option to the documentation.\n\n>\n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > @@ -1410,6 +1412,11 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> > +       if (use_color != GIT_COLOR_AUTO) {\n> > +               s.use_color=use_color;\n> > +               set_diff_color(use_color);\n> > +       }\n> > diff --git a/diff.c b/diff.c\n> > @@ -261,6 +261,11 @@ void init_diff_ui_defaults(void)\n> > +void set_diff_color(int use_color)\n> > +{\n> > +       diff_use_color_default = use_color;\n> > +}\n> > diff --git a/diff.h b/diff.h\n> > @@ -501,6 +501,7 @@ int parse_long_opt(const char *opt, const char **argv,\n> > +void set_diff_color(int use_color);\n>\n> This new API for setting `diff_use_color_default` feels a bit too\n> quick-and-dirty and assumes that the caller has intimate knowledge\n> about when it is safe/correct to call the new function. Did you\n> consider the alternate approach of having wt-status functionality set\n> the appropriate diff_options.use_color value at the time it drives the\n> diff machinery? For instance, as a test, I added:\n>\n>     rev.diffopt.use_color = s->use_color;\n>\n> to the functions wt-status.c:wt_status_collect_changes_worktree(),\n> wt_status_collect_changes_index(), and wt_longstatus_print_verbose(),\n> so that the `use_color` value from the `struct wt_status *` supplied\n> by commit.c:cmd_status() is automatically applied to the diff options.\n>\n> (Note that this was just a quick test. I dug through the code just\n> enough to locate these functions as the likely correct place to set\n> diff_options.use_color, but didn't spend any time verifying that all\n> three functions need to be patched like this. I also didn't verify\n> that my changes won't stomp on --porcelain's explicit disabling of\n> color, which is something that ought to be checked. There's also some\n> custom `use_color` handling going on in wt_longstatus_print_verbose()\n> of which to be aware.)\n\nI can understand how this may look \"quick-and-dirty\", but it was actually\nmy intent to create a function which assumes that the caller has intimate\nknowledge about when it is safe/correct.  I will try to explain why.\n\nOriginally I tried to use what I thought would be a much more appropriate\nmethod which is to simply let the --color flag set things the same way\nas the config option status.color=always does, but I noticed it does\nnot work the same when piped.\n\nFor example the following produces full color output:\ngit -c status.color=always status -v\n\nHowever, running this colors only the status, not the diff:\ngit -c status.color=always status -v | cat\n\nRight now I can only get what I expect by running:\ngit -c status.color=always -c diff.color=always status -v | cat\n\nThis appeared to me to be inconsistent behaviour and lead me down\na path trying to figure out where the color was being disabled which\nmade me realize that the status command shares code paths with\nthe commit message and porcelain output.  I wanted to be very\ncareful not to do anything which might break either of these in some\nunforeseen way which is why I created the function.\n\nI changed the function name and added a comment to make it clear\nthat its usage is a special case which may have undesired\nconsequences if not used appropriately.\n\nIf you feel existing unit tests would mitigate any issues with commit\nmessages and porcelain output I can try going the route you\nsuggested instead?\n\nAlso if you agree the behavior of status.color should be the same for\nboth piped and not piped output I could add that to this patch as well?\n\n>\n> > diff --git a/t/t7527-status-color-pipe.sh b/t/t7527-status-color-pipe.sh\n> > @@ -0,0 +1,69 @@\n> > +# Normal git status does not pipe colors\n> > +test_expect_success 'git status' '\n> > +       git status >out &&\n> > +       test_i18ngrep \"original$\" out\n> > +'\n>\n> None of the text being checked by any of the tests being added by this\n> file is subject to localization, so use of test_i18ngrep() is\n> unwarranted. Use plain old `grep` instead here and elsewhere.\n\nI made these changes.\n\n>\n> > +# Test new color option with never (expect same as above)\n> > +test_expect_success 'git status --color=never' '\n> > +       git status --color=never >out &&\n> > +       test_i18ngrep \"original$\" out\n> > +'\n> > +\n> > +# Test new color (default is always)\n> > +test_expect_success 'git status --color' '\n> > +       git status --color |\n> > +       test_decode_color >out &&\n> > +       test_i18ngrep \"original<RESET>$\" out\n> > +'\n>\n> If someone introduces a bug which causes the non-colored tests to\n> incorrectly emit color codes, then it will be easier to debug the\n> problem if you also pass the output through test_decode_color() even\n> for the non-colored tests rather than only for the expect-colored\n> tests. Thus, I'd recommend calling test_decode_color() in all the\n> tests, even if you don't expect color to be emitted.\n>\n> Also, these days, we don't normally place a Git command upstream in a\n> pipe since its exit code will be lost. Instead, we capture the output\n> to a file:\n>\n>     git status --color >raw &&\n>     test_decode_color <raw >out &&\n>     ...\n\nI made both these changes also.\n\n>\n> > +test_done\n> > +# Test verbose --color=always\n> > +test_expect_success 'git status -v --color=always' '\n> > +       git status -v --color=always |\n> > +       test_decode_color >out &&\n> > +       test_i18ngrep \"<CYAN>@@ -0,0 +1 @@<RESET>\" out &&\n> > +       test_i18ngrep \"GREEN>+<RESET><GREEN>1<RESET>\" out\n> > +'\n> > +\n> > +test_done\n>\n> You have one too many calls to test_done() in this fragment. I think\n> you only want to keep the final test_done() and remove the one prior\n> to the last test.\n\nYou are correct, thanks for catching this.\n"},{"id":"415701","messageId":"CAPig+cS-hnwp2HjkkFPeJ4aibFHnJ0VZq0DSVgdWB0H_q5=oXw@mail.gmail.com","threadId":"55076","inReplyTo":"CACPHW2X2UGAVmNM+cHXs6dwVfZbgLFZ0iUGU89h04H5czAt1Ww@mail.gmail.com","subject":"Re: [PATCH] status: learn --color for piping colored output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-01-31T23:09:15Z","receivedAt":"2021-01-31T23:12:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[cc:+junio +peff +duy +dscho +rene]\n\nOn Sun, Jan 31, 2021 at 1:49 PM Lance Ward <ljward10@gmail.com> wrote:\n> On Sun, Jan 31, 2021 at 1:31 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Sat, Jan 30, 2021 at 10:51 AM Lance Ward via GitGitGadget\n> > <gitgitgadget@gmail.com> wrote:\n> > > diff --git a/diff.c b/diff.c\n> > > +void set_diff_color(int use_color)\n> > > +{\n> > > +       diff_use_color_default = use_color;\n> > > +}\n> >\n> > This new API for setting `diff_use_color_default` feels a bit too\n> > quick-and-dirty and assumes that the caller has intimate knowledge\n> > about when it is safe/correct to call the new function. Did you\n> > consider the alternate approach of having wt-status functionality set\n> > the appropriate diff_options.use_color value at the time it drives the\n> > diff machinery? For instance, as a test, I added:\n> >\n> >     rev.diffopt.use_color = s->use_color;\n> >\n> > to the functions wt-status.c:wt_status_collect_changes_worktree(),\n> > wt_status_collect_changes_index(), and wt_longstatus_print_verbose(),\n> > so that the `use_color` value from the `struct wt_status *` supplied\n> > by commit.c:cmd_status() is automatically applied to the diff options.\n>\n> Originally I tried to use what I thought would be a much more appropriate\n> method which is to simply let the --color flag set things the same way\n> as the config option status.color=always does, but I noticed it does\n> not work the same when piped.\n\nI'm not quite sure what you mean. How exactly did you originally\nimplement --config to accomplish this?\n\n> For example the following produces full color output:\n> git -c status.color=always status -v\n>\n> However, running this colors only the status, not the diff:\n> git -c status.color=always status -v | cat\n>\n> Right now I can only get what I expect by running:\n> git -c status.color=always -c diff.color=always status -v | cat\n>\n> This appeared to me to be inconsistent behaviour [...]\n\nAt an implementation level, this behavior makes sense, but I can see\nhow it might be confusing and unexpected from a user's viewpoint. I\nthink the approach I suggested of patching those wt-status.c functions\nto use:\n\n    rev.diffopt.use_color = s->use_color;\n\nwould fix this inconsistency, wouldn't it?\n\n> [...] and lead me down\n> a path trying to figure out where the color was being disabled which\n> made me realize that the status command shares code paths with\n> the commit message and porcelain output.  I wanted to be very\n> careful not to do anything which might break either of these in some\n> unforeseen way which is why I created the function.\n\nI see where you are coming from and understand the desire to isolate\nthis behavior change, however, I can't shake the feeling that this\nsledge-hammer approach is going in the wrong direction and that the\nfine-grained approach I suggested in my review is more desirable.\nHaving said that, I'm not particularly familiar with this area of the\ncode -- and had to spend some time digging through it to find those\nfunctions in wt-status.c to make the fine-grained approach work -- so\nit would be nice to hear from people who have spent a lot more time in\nthat area of the code (I Cc:'d them).\n\n> If you feel existing unit tests would mitigate any issues with commit\n> messages and porcelain output I can try going the route you\n> suggested instead?\n\nI doubt that anyone on this project feels that the unit tests have\nsufficient coverage to make any guarantees. However, for changes such\nas the one I proposed which might have unforeseen side-effects, Junio\ntends to let the changes \"cook\" for a while in his 'next' branch\nbefore promoting them to the 'master' branch so as to give time for\nunexpected fallout to be discovered.\n\n> Also if you agree the behavior of status.color should be the same for\n> both piped and not piped output I could add that to this patch as well?\n\nI'm not quite sure I understand your question. Are you asking if\n`color.status` should imply `color.diff`? If so, I haven't thought a\nlot about it, but I can see how the present behavior may have a high\nsurprise-factor for users, so it might be worthwhile.\n\nIn fact, I can envision this patch being re-rolled as a two-patch\nseries which (1) patches the wt-status.c functions to do\n`rev.diffopt.use_color = s->use_color` which should make\n`color.status` imply `color.diff`, and (2) adds a --color option to\n`git status` which sets `wt_status.use_color` (which would then be\nautomatically inherited by the diff machinery due to the first patch).\n\n(By the way, `status.color` is a deprecated synonym for `color.status`.)\n"},{"id":"415879","messageId":"CACPHW2VBEu=02HFhyrDes=6KceLtHzGDqBJVf2qAnD2s1f8VAg@mail.gmail.com","threadId":"55076","inReplyTo":"CAPig+cS-hnwp2HjkkFPeJ4aibFHnJ0VZq0DSVgdWB0H_q5=oXw@mail.gmail.com","subject":"Re: [PATCH] status: learn --color for piping colored output","fromName":"Lance Ward","fromEmail":"ljward10@gmail.com","sentAt":"2021-02-02T04:09:00Z","receivedAt":"2021-02-02T04:09:57Z","isPatch":true,"sender":{"key":"ljward10@gmail.com","avatar":null},"body":"Hi Eric,\n\nI've updated my patch per your request.\n\nOn Sun, Jan 31, 2021 at 5:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> [cc:+junio +peff +duy +dscho +rene]\n>\n> On Sun, Jan 31, 2021 at 1:49 PM Lance Ward <ljward10@gmail.com> wrote:\n> > On Sun, Jan 31, 2021 at 1:31 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > > On Sat, Jan 30, 2021 at 10:51 AM Lance Ward via GitGitGadget\n> > > <gitgitgadget@gmail.com> wrote:\n> > > > diff --git a/diff.c b/diff.c\n> > > > +void set_diff_color(int use_color)\n> > > > +{\n> > > > +       diff_use_color_default = use_color;\n> > > > +}\n> > >\n> > > This new API for setting `diff_use_color_default` feels a bit too\n> > > quick-and-dirty and assumes that the caller has intimate knowledge\n> > > about when it is safe/correct to call the new function. Did you\n> > > consider the alternate approach of having wt-status functionality set\n> > > the appropriate diff_options.use_color value at the time it drives the\n> > > diff machinery? For instance, as a test, I added:\n> > >\n> > >     rev.diffopt.use_color = s->use_color;\n> > >\n> > > to the functions wt-status.c:wt_status_collect_changes_worktree(),\n> > > wt_status_collect_changes_index(), and wt_longstatus_print_verbose(),\n> > > so that the `use_color` value from the `struct wt_status *` supplied\n> > > by commit.c:cmd_status() is automatically applied to the diff options.\n> >\n> > Originally I tried to use what I thought would be a much more appropriate\n> > method which is to simply let the --color flag set things the same way\n> > as the config option status.color=always does, but I noticed it does\n> > not work the same when piped.\n>\n> I'm not quite sure what you mean. How exactly did you originally\n> implement --config to accomplish this?\n>\n> > For example the following produces full color output:\n> > git -c status.color=always status -v\n> >\n> > However, running this colors only the status, not the diff:\n> > git -c status.color=always status -v | cat\n> >\n> > Right now I can only get what I expect by running:\n> > git -c status.color=always -c diff.color=always status -v | cat\n> >\n> > This appeared to me to be inconsistent behaviour [...]\n>\n> At an implementation level, this behavior makes sense, but I can see\n> how it might be confusing and unexpected from a user's viewpoint. I\n> think the approach I suggested of patching those wt-status.c functions\n> to use:\n>\n>     rev.diffopt.use_color = s->use_color;\n>\n> would fix this inconsistency, wouldn't it?\n\nYes, it did.\n\n>\n> > [...] and lead me down\n> > a path trying to figure out where the color was being disabled which\n> > made me realize that the status command shares code paths with\n> > the commit message and porcelain output.  I wanted to be very\n> > careful not to do anything which might break either of these in some\n> > unforeseen way which is why I created the function.\n>\n> I see where you are coming from and understand the desire to isolate\n> this behavior change, however, I can't shake the feeling that this\n> sledge-hammer approach is going in the wrong direction and that the\n> fine-grained approach I suggested in my review is more desirable.\n> Having said that, I'm not particularly familiar with this area of the\n> code -- and had to spend some time digging through it to find those\n> functions in wt-status.c to make the fine-grained approach work -- so\n> it would be nice to hear from people who have spent a lot more time in\n> that area of the code (I Cc:'d them).\n>\n\nI've made the change you requested and it resolves the issue.\nIt also fixed the inconsistency I mentioned.  I only needed\nto modify wt_longstatus_print_verbose to resolve the issue\nsince it is the only status path that had an issue with the\ngit status command.\n\n> > If you feel existing unit tests would mitigate any issues with commit\n> > messages and porcelain output I can try going the route you\n> > suggested instead?\n>\n> I doubt that anyone on this project feels that the unit tests have\n> sufficient coverage to make any guarantees. However, for changes such\n> as the one I proposed which might have unforeseen side-effects, Junio\n> tends to let the changes \"cook\" for a while in his 'next' branch\n> before promoting them to the 'master' branch so as to give time for\n> unexpected fallout to be discovered.\n>\n\nI did not have to patch all the functions you mentioned and think\nthe new change is cleaner and will not break anything else.\n\n> > Also if you agree the behavior of status.color should be the same for\n> > both piped and not piped output I could add that to this patch as well?\n>\n> I'm not quite sure I understand your question. Are you asking if\n> `color.status` should imply `color.diff`? If so, I haven't thought a\n> lot about it, but I can see how the present behavior may have a high\n> surprise-factor for users, so it might be worthwhile.\n\nYes, the inconsistency I mentioned was basically that:\ncolor.status implies color.diff when outputting to stdout,\nbut this is not true when outputting to a file.  My latest\nchange resolves this inconsistency.  Now color.status\nimplies color.diff when running git status regardless\nof where the output is going.\n\n>\n> In fact, I can envision this patch being re-rolled as a two-patch\n> series which (1) patches the wt-status.c functions to do\n> `rev.diffopt.use_color = s->use_color` which should make\n> `color.status` imply `color.diff`, and (2) adds a --color option to\n> `git status` which sets `wt_status.use_color` (which would then be\n> automatically inherited by the diff machinery due to the first patch).\n>\n> (By the way, `status.color` is a deprecated synonym for `color.status`.)\n\nRight now as it stands my patch resolves both of these, but\nif you'd like to make it two separate patches that would be fine.\n"},{"id":"416024","messageId":"CAPig+cSSU1P68dBomjRkO4jUqUnu+0ri5-3y0-H228-qONwhyw@mail.gmail.com","threadId":"55076","inReplyTo":"CACPHW2VBEu=02HFhyrDes=6KceLtHzGDqBJVf2qAnD2s1f8VAg@mail.gmail.com","subject":"Re: [PATCH] status: learn --color for piping colored output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-03T04:46:34Z","receivedAt":"2021-02-03T04:48:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 1, 2021 at 11:09 PM Lance Ward <ljward10@gmail.com> wrote:\n> On Sun, Jan 31, 2021 at 5:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > I think the approach I suggested of patching those wt-status.c functions\n> > to use:\n> >     rev.diffopt.use_color = s->use_color;\n> > would fix this inconsistency, wouldn't it?\n>\n> I've made the change you requested and it resolves the issue.\n> It also fixed the inconsistency I mentioned.  I only needed\n> to modify wt_longstatus_print_verbose to resolve the issue\n> since it is the only status path that had an issue with the\n> git status command.\n\nOkay, makes sense. As long as you insert the assignment somewhere\nabove the special case:\n\n    if (s->fp != stdout) {\n        rev.diffopt.use_color = 0;\n        wt_status_add_cut_line(s->fp);\n    }\n\nthen it should work correctly, I would think. However, it might be\neasier for people to grok the logic overall if you incorporate it into\nthat conditional, perhaps like this:\n\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\nOr this:\n\n    if (s->fp == stdout) {\n        rev.diffopt.use_color = s->use_color;\n    else {\n        rev.diffopt.use_color = 0;\n        wt_status_add_cut_line(s->fp);\n    }\n\nIt's subjective, of course, so use your best judgment.\n\n> > In fact, I can envision this patch being re-rolled as a two-patch\n> > series which (1) patches the wt-status.c functions to do\n> > `rev.diffopt.use_color = s->use_color` which should make\n> > `color.status` imply `color.diff`, and (2) adds a --color option to\n> > `git status` which sets `wt_status.use_color` (which would then be\n> > automatically inherited by the diff machinery due to the first patch).\n>\n> Right now as it stands my patch resolves both of these, but\n> if you'd like to make it two separate patches that would be fine.\n\nThe reason I was thinking of splitting these changes into two patches\nis that they have different purposes. You'd sell the first patch as a\nstraight up bug fix, and it's easier to formulate a proper \"sales\nspiel\" for that if it's not blurred with other changes. (This may be\nimportant because it could be slightly controversial if other people\ndon't consider the behavior as being buggy.) The second patch would be\nan easy sell as a straightforward and simple enhancement. Another\nreason for splitting them into two patches is that doing so would make\nit easier to revert the bug-fix patch separately if it turns out that\nthere are unforeseen negative side-effects.\n\nHaving said that, a well-crafted commit message may very well make it\neasy to sell the change(s) as a single patch. Again, use your best\njudgment.\n"},{"id":"416091","messageId":"CACPHW2WbY+c914vHV8cjfBC1GstF7PxfQFr-NF=EUhcF0YaCzQ@mail.gmail.com","threadId":"55076","inReplyTo":"CAPig+cSSU1P68dBomjRkO4jUqUnu+0ri5-3y0-H228-qONwhyw@mail.gmail.com","subject":"Re: [PATCH] status: learn --color for piping colored output","fromName":"Lance Ward","fromEmail":"ljward10@gmail.com","sentAt":"2021-02-03T21:08:26Z","receivedAt":"2021-02-03T21:09:34Z","isPatch":true,"sender":{"key":"ljward10@gmail.com","avatar":null},"body":"Hi Eric,\n\nI made all the changes you suggested and split it into two\nindependent changes:\n\nThis patch handles the --color option:\nhttps://github.com/git/git/pull/955\n\nThis patch fixes the coloring inconsistency:\nhttps://github.com/git/git/pull/954\n\nBoth have failed a CI test, but the reason has nothing to do with my\ncode.  CI/PR / static-analysis VM is trying to install the package\ncoccinelle, but is failing to install it with the following:\n\nE: Unable to locate package coccinelle\nError: Process completed with exit code 100.\n\nOn Tue, Feb 2, 2021 at 10:46 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Mon, Feb 1, 2021 at 11:09 PM Lance Ward <ljward10@gmail.com> wrote:\n> > On Sun, Jan 31, 2021 at 5:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > > I think the approach I suggested of patching those wt-status.c functions\n> > > to use:\n> > >     rev.diffopt.use_color = s->use_color;\n> > > would fix this inconsistency, wouldn't it?\n> >\n> > I've made the change you requested and it resolves the issue.\n> > It also fixed the inconsistency I mentioned.  I only needed\n> > to modify wt_longstatus_print_verbose to resolve the issue\n> > since it is the only status path that had an issue with the\n> > git status command.\n>\n> Okay, makes sense. As long as you insert the assignment somewhere\n> above the special case:\n>\n>     if (s->fp != stdout) {\n>         rev.diffopt.use_color = 0;\n>         wt_status_add_cut_line(s->fp);\n>     }\n>\n> then it should work correctly, I would think. However, it might be\n> easier for people to grok the logic overall if you incorporate it into\n> that conditional, perhaps like this:\n>\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>\n> Or this:\n>\n>     if (s->fp == stdout) {\n>         rev.diffopt.use_color = s->use_color;\n>     else {\n>         rev.diffopt.use_color = 0;\n>         wt_status_add_cut_line(s->fp);\n>     }\n>\n> It's subjective, of course, so use your best judgment.\n>\n\nI've made the change you suggested.\n\n> > > In fact, I can envision this patch being re-rolled as a two-patch\n> > > series which (1) patches the wt-status.c functions to do\n> > > `rev.diffopt.use_color = s->use_color` which should make\n> > > `color.status` imply `color.diff`, and (2) adds a --color option to\n> > > `git status` which sets `wt_status.use_color` (which would then be\n> > > automatically inherited by the diff machinery due to the first patch).\n> >\n> > Right now as it stands my patch resolves both of these, but\n> > if you'd like to make it two separate patches that would be fine.\n>\n> The reason I was thinking of splitting these changes into two patches\n> is that they have different purposes. You'd sell the first patch as a\n> straight up bug fix, and it's easier to formulate a proper \"sales\n> spiel\" for that if it's not blurred with other changes. (This may be\n> important because it could be slightly controversial if other people\n> don't consider the behavior as being buggy.) The second patch would be\n> an easy sell as a straightforward and simple enhancement. Another\n> reason for splitting them into two patches is that doing so would make\n> it easier to revert the bug-fix patch separately if it turns out that\n> there are unforeseen negative side-effects.\n>\n> Having said that, a well-crafted commit message may very well make it\n> easy to sell the change(s) as a single patch. Again, use your best\n> judgment.\n\nI've split it into two patches.\n"},{"id":"416242","messageId":"CAPig+cQMeK_vPuS-V3EAQUDHZaJ21VSgPV0m1CB_LZ8=9Bbgzw@mail.gmail.com","threadId":"55076","inReplyTo":"CACPHW2WbY+c914vHV8cjfBC1GstF7PxfQFr-NF=EUhcF0YaCzQ@mail.gmail.com","subject":"Re: [PATCH] status: learn --color for piping colored output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-05T06:23:22Z","receivedAt":"2021-02-05T06:24:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 3, 2021 at 4:08 PM Lance Ward <ljward10@gmail.com> wrote:\n> I made all the changes you suggested and split it into two\n> independent changes:\n>\n> This patch handles the --color option:\n> https://github.com/git/git/pull/955\n>\n> This patch fixes the coloring inconsistency:\n> https://github.com/git/git/pull/954\n\nThanks for being responsive and indulging this reviewer. The two-patch\napproach turned out to be concise and easy to digest. The bug fix --\nwhich effectively turned out to be a one-liner -- is an especially\nnice improvement over the earlier version, and the implementation of\nthe new --color option boiled down very nicely to a really minimal\namount of straightforward code.\n"}]}