Re: [PATCH 0/1] Colorize some errors on stderr
On Fri, Feb 16 2018, Johannes Schindelin wrote: > This is an RFC because it tries to introduce a fundamental new color feature: > Coloring messages *on stderr*. I missed this the first time around, and don't have anything to add that others haven't covered. Just wanted to say I'd love to get this into git, it's a great UI improvement.
Re: [PATCH 0/1] Colorize some errors on stderr
Hi Junio, [and welcome back, at least I hope you only read this after a good and relaxing vacation] On Fri, 16 Feb 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Now, what would be possible solutions for this? > > > > - introduce `int fd` in `want_color()` (and callees) so that we can make > > a distinction whether we want to detect whether stdout or stderr is > > connected > > to a tty > > > > - introduce a separate `want_color_stderr()` (we still would need to decide > > whether we want a config setting for this) > > Between the above two, there probably aren't so big a difference, but > in order to avoid disrupting existing callers of want_color() while > possibly sharing as much code between the old and new callers, > perhaps: > > extern int want_color_fd(int fd, int colorbool); > #define want_color(colorbool) want_color_fd(1, (colorbool)) > #define want_color_stderr(colorbool) want_color_fd(2, (colorbool)) I made it so. Note that I also had to change the check_auto_color() function, and while want_color_fd() can have a "private" record of previous results, check_auto_color() needs to use the global color_stdout_is_tty (so that builtin/config.c can edit it, for use in `git config --colorbool [stdout-is-tty]`). > We should honor configuration at two levels, just like the colors on > stdout, i.e. color in which individual items are painted (e.g. > color.diff.filename, color.advice.hint) and whether we use colors in > UI at all (e.g. color.ui). This is how v2 does it. Thanks for your suggestions, Dscho
Re: [PATCH 0/1] Colorize some errors on stderr
Johannes Schindelin writes: > Now, what would be possible solutions for this? > > - introduce `int fd` in `want_color()` (and callees) so that we can make > a distinction whether we want to detect whether stdout or stderr is > connected > to a tty > > - introduce a separate `want_color_stderr()` (we still would need to decide > whether we want a config setting for this) Between the above two, there probably aren't so big a difference, but in order to avoid disrupting existing callers of want_color() while possibly sharing as much code between the old and new callers, perhaps: extern int want_color_fd(int fd, int colorbool); #define want_color(colorbool) want_color_fd(1, (colorbool)) #define want_color_stderr(colorbool) want_color_fd(2, (colorbool)) We should honor configuration at two levels, just like the colors on stdout, i.e. color in which individual items are painted (e.g. color.diff.filename, color.advice.hint) and whether we use colors in UI at all (e.g. color.ui). I do not think it is necessary or even at the right granularity to allow settings like "do color stdout but do not color errors". > - not color stderr, ever This is my personal preference, but that does not and should not carry too much weight ;-)
[PATCH 0/1] Colorize some errors on stderr
This is an RFC because it tries to introduce a fundamental new color feature: Coloring messages *on stderr*. So far, pretty much everything in color.[ch] assumed that you want to color only stuff on stdout. However, in this case, a user (who became a contributor!) wanted some messages that are printed to stderr and were missed by his colleagues to be colored. The contribution comes via Pull Request from the Git for Windows project: https://github.com/git-for-windows/git/pull/1429 Now, what would be possible solutions for this? - introduce `int fd` in `want_color()` (and callees) so that we can make a distinction whether we want to detect whether stdout or stderr is connected to a tty - introduce a separate `want_color_stderr()` (we still would need to decide whether we want a config setting for this) - not color stderr, ever Also, I did not have too much time to dig into the question how to test this in Git's test suite. Do we already have tests that generate fake server-side errors onto which I could piggy-back a new test case? Thoughts? Suggestions? Help? Ryan Dammrose (1): Colorize push errors advice.c | 42 +++-- builtin/push.c | 38 + transport.c| 59 +- 3 files changed, 136 insertions(+), 3 deletions(-) base-commit: b2e45c695d09f6a31ce09347ae0a5d2cdfe9dd4e Published-As: https://github.com/dscho/git/releases/tag/colorize-push-errors-v1 Fetch-It-Via: git fetch https://github.com/dscho/git colorize-push-errors-v1 -- 2.16.1.windows.4
