* 'git log' escape symbols shown as ESC[33 and ESC[m @ 2014-01-17 0:34 Yuri 2014-01-17 1:47 ` Jeff King 0 siblings, 1 reply; 39+ messages in thread From: Yuri @ 2014-01-17 0:34 UTC (permalink / raw) To: git When I run 'git log' on FreeBSD-9.2, I get output like this: ESC[33mcommit 398e78c62fd507a317de7c2abb8e25c9fac7ac9eESC[m Merge: 5fb8f6e d2138ba ... ESC is white on black background. Why ESC[33m aren't expanded by the terminal? Is this because git prints an unsupported sequence? Hex of what git writes to terminal is here: 0x0000 1b5b 3333 6d63 6f6d 6d69 7420 6636 6432 6136 3032 3965 6661 6439 6635 6334 3161 6261 |.[33mcommit f6d2a6029efad9f5c41aba| 0x0022 3961 3830 6131 3032 3138 6332 6333 3465 6662 1b5b 6d0a 4d65 7267 653a 2033 3938 6537 |9a80a10218c2c34efb.[m.Merge: 398e7| I think it tries to print the line in yellow (color code 33), and prints the wrong sequence. The correct sequence would be: \033[1;33mString Goes Here\033[0m It misses "1;" in the beginning, and "0" in the end, this is why the sequence is not interpreted. Why does it print a wrong sequence? Is this because this is some kind of linuxism that doesn't work on FreeBSD maybe? Also, there are the termcap functions that allow to determine what does the actual terminal supports. You should first check for cap bits corresponding to the features you expect, if you expect something uncommon. Yuri ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 0:34 'git log' escape symbols shown as ESC[33 and ESC[m Yuri @ 2014-01-17 1:47 ` Jeff King 2014-01-17 2:02 ` Yuri ` (2 more replies) 0 siblings, 3 replies; 39+ messages in thread From: Jeff King @ 2014-01-17 1:47 UTC (permalink / raw) To: Yuri; +Cc: git On Thu, Jan 16, 2014 at 04:34:01PM -0800, Yuri wrote: > When I run 'git log' on FreeBSD-9.2, I get output like this: > ESC[33mcommit 398e78c62fd507a317de7c2abb8e25c9fac7ac9eESC[m > Merge: 5fb8f6e d2138ba > ... > > ESC is white on black background. > > Why ESC[33m aren't expanded by the terminal? Is this because git > prints an unsupported sequence? Are you using "less" as your pager (it is the default in git unless you have set your PAGER environment variable)? If so, do you have the "R" option set to pass through ANSI codes? Git will set this automatically in your "LESS" variable if you do not already have such a variable (but it will not touch it if you already have it set, and are missing "R"). > Hex of what git writes to terminal is here: > 0x0000 1b5b 3333 6d63 6f6d 6d69 7420 6636 6432 6136 3032 3965 6661 > 6439 6635 6334 3161 6261 |.[33mcommit f6d2a6029efad9f5c41aba| > 0x0022 3961 3830 6131 3032 3138 6332 6333 3465 6662 1b5b 6d0a 4d65 > 7267 653a 2033 3938 6537 |9a80a10218c2c34efb.[m.Merge: 398e7| > > I think it tries to print the line in yellow (color code 33), and > prints the wrong sequence. The correct sequence would be: > \033[1;33mString Goes Here\033[0m > It misses "1;" in the beginning, and "0" in the end, this is why the > sequence is not interpreted. No, the "\033[33m" is correct. The "1;" in your string is turning on the bold attribute, and we are not trying to do that. The "0" in the reset is optional (at least according to [1]; I do not have an actual standard to reference). But I do not think that is your problem anyway; the "ESC" you are seeing is almost certainly generated by "less" escaping the \033. > Why does it print a wrong sequence? Is this because this is some kind > of linuxism that doesn't work on FreeBSD maybe? No. See above. > Also, there are the termcap functions that allow to determine what > does the actual terminal supports. You should first check for cap > bits corresponding to the features you expect, if you expect > something uncommon. Almost every terminal supports ANSI colors. The notable exceptions is the Windows console, but we handle the translation there. If you have a terminal that actually doesn't support ANSI colors, please let us know and we can see what is going on. But I'm reasonably sure that you are just running up against the escaping in "less" here. -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 1:47 ` Jeff King @ 2014-01-17 2:02 ` Yuri 2014-01-17 2:13 ` Jeff King 2014-01-17 20:15 ` 'git log' escape symbols shown as ESC[33 and ESC[m Yuri 2014-02-05 1:24 ` Yuri 2 siblings, 1 reply; 39+ messages in thread From: Yuri @ 2014-01-17 2:02 UTC (permalink / raw) To: Jeff King; +Cc: git On 01/16/2014 17:47, Jeff King wrote: > Are you using "less" as your pager (it is the default in git unless you > have set your PAGER environment variable)? If so, do you have the "R" > option set to pass through ANSI codes? Git will set this automatically > in your "LESS" variable if you do not already have such a variable (but > it will not touch it if you already have it set, and are missing "R"). My PAGER variable was set to "more". PAGER=more is also a default for a newly created user in FreeBSD. So what would be the correct fix here in general, so that git will work fine for a new unchanged user? Yuri ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 2:02 ` Yuri @ 2014-01-17 2:13 ` Jeff King 2014-01-17 2:28 ` Yuri 2014-01-17 2:29 ` Jonathan Nieder 0 siblings, 2 replies; 39+ messages in thread From: Jeff King @ 2014-01-17 2:13 UTC (permalink / raw) To: Yuri; +Cc: git On Thu, Jan 16, 2014 at 06:02:24PM -0800, Yuri wrote: > On 01/16/2014 17:47, Jeff King wrote: > >Are you using "less" as your pager (it is the default in git unless you > >have set your PAGER environment variable)? If so, do you have the "R" > >option set to pass through ANSI codes? Git will set this automatically > >in your "LESS" variable if you do not already have such a variable (but > >it will not touch it if you already have it set, and are missing "R"). > > My PAGER variable was set to "more". PAGER=more is also a default for > a newly created user in FreeBSD. Interesting. I take it that "more" does not pass through ANSI codes at all, then. > So what would be the correct fix here in general, so that git will > work fine for a new unchanged user? This was a non-issue until 4c7f181 (make color.ui default to 'auto', 2013-06-10), which was released in git v1.8.4, as people had to explicitly turn color on. I'm cc-ing Matthieu, who authored that commit. You can: git config color.ui false to turn off color completely. But in this case, I think it is more exact to tell git simply that your pager does not handle color: git config color.pager false Obviously that does not help the "new unchanged user". I think we can be smarter about guessing whether the pager can actually handle color, based on the name. Is it possible for you to compile git with the patch below? It should make your problem go away without having to configure anything. It can't cover every possible pager, but I think it should catch the common ones. --- diff --git a/cache.h b/cache.h index 83a2726..7fd1977 100644 --- a/cache.h +++ b/cache.h @@ -1215,7 +1215,8 @@ static inline ssize_t write_str_in_full(int fd, const char *str) extern void setup_pager(void); extern const char *pager_program; extern int pager_in_use(void); -extern int pager_use_color; +extern int pager_use_color_config; +extern int pager_use_color(void); extern int term_columns(void); extern int decimal_width(int); extern int check_pager_config(const char *cmd); diff --git a/color.c b/color.c index f672885..ffbff40 100644 --- a/color.c +++ b/color.c @@ -184,7 +184,7 @@ static int check_auto_color(void) { if (color_stdout_is_tty < 0) color_stdout_is_tty = isatty(1); - if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) { + if (color_stdout_is_tty || (pager_in_use() && pager_use_color())) { char *term = getenv("TERM"); if (term && strcmp(term, "dumb")) return 1; diff --git a/config.c b/config.c index d969a5a..2a8236b 100644 --- a/config.c +++ b/config.c @@ -991,7 +991,7 @@ int git_default_config(const char *var, const char *value, void *dummy) return git_default_advice_config(var, value); if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) { - pager_use_color = git_config_bool(var,value); + pager_use_color_config = git_config_bool(var,value); return 0; } diff --git a/environment.c b/environment.c index 3c76905..5cd642f 100644 --- a/environment.c +++ b/environment.c @@ -39,7 +39,7 @@ size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; size_t delta_base_cache_limit = 16 * 1024 * 1024; unsigned long big_file_threshold = 512 * 1024 * 1024; const char *pager_program; -int pager_use_color = 1; +int pager_use_color_config = -1; const char *editor_program; const char *askpass_program; const char *excludes_file; diff --git a/pager.c b/pager.c index 0cc75a8..a816af3 100644 --- a/pager.c +++ b/pager.c @@ -182,3 +182,38 @@ int check_pager_config(const char *cmd) pager_program = c.value; return c.want; } + +static int pager_can_handle_color(void) +{ + const char *pager = git_pager(1); + + /* + * If it's less, we automatically set "R" and can handle color, + * unless the user already has a "LESS" variable that does not + * include "R". + */ + if (!strcmp(pager, "less")) { + const char *x = getenv("LESS"); + return !x || !!strchr(x, 'R'); + } + + /* + * We know that "more" does not pass through colors at all. + */ + if (!strcmp(pager, "more")) + return 0; + + /* + * Otherwise, we don't recognize it. Guess that it can probably handle + * color. This matches historical behavior, though it is probably not + * the safest default. + */ + return 1; +} + +int pager_use_color(void) +{ + if (pager_use_color_config < 0) + pager_use_color_config = pager_can_handle_color(); + return pager_use_color_config; +} ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 2:13 ` Jeff King @ 2014-01-17 2:28 ` Yuri 2014-01-17 2:32 ` Jeff King 2014-01-17 2:29 ` Jonathan Nieder 1 sibling, 1 reply; 39+ messages in thread From: Yuri @ 2014-01-17 2:28 UTC (permalink / raw) To: Jeff King; +Cc: git On 01/16/2014 18:13, Jeff King wrote: > Interesting. I take it that "more" does not pass through ANSI codes at > all, then. > Actually, 'more -R' also passes colors on FreeBSD. So maybe you can always add -R if PAGER=more on *BSD (any of them) ? This will fix this issue. I know, it is unpleasant when you add some new minor feature (like term colors), and people begin to complain about some related issues. But turning colors off isn't a good approach also. App just needs to be smarter about when and how to use them. I would go even further, and convey even more information with colors. For example, make all dates which are less than 24 hours red. Yuri ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 2:28 ` Yuri @ 2014-01-17 2:32 ` Jeff King 2014-01-17 2:46 ` Yuri 0 siblings, 1 reply; 39+ messages in thread From: Jeff King @ 2014-01-17 2:32 UTC (permalink / raw) To: Yuri; +Cc: git On Thu, Jan 16, 2014 at 06:28:10PM -0800, Yuri wrote: > On 01/16/2014 18:13, Jeff King wrote: > >Interesting. I take it that "more" does not pass through ANSI codes at > >all, then. > > Actually, 'more -R' also passes colors on FreeBSD. So maybe you can > always add -R if PAGER=more on *BSD (any of them) ? This will fix > this issue. Ah, if "more" can handle the colors, then that is preferable. I do think it would have to be system-specific, though. Unlike "less", there are many implementations of "more", and quite a lot of them will probably not support colors. We can make it a build-time option, and set it to "on" for FreeBSD. > I know, it is unpleasant when you add some new minor feature (like > term colors), and people begin to complain about some related issues. > But turning colors off isn't a good approach also. App just needs to > be smarter about when and how to use them. Agreed. It is simply that most people seem to either use "less", or not set their "PAGER" at all. I think FreeBSD is the exception here. > I would go even further, and convey even more information with > colors. For example, make all dates which are less than 24 hours red. That's an orthogonal issue. I'm not sure I agree, but if you are interested, feel free to prepare a patch, which will get some discussion going. -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 2:32 ` Jeff King @ 2014-01-17 2:46 ` Yuri 0 siblings, 0 replies; 39+ messages in thread From: Yuri @ 2014-01-17 2:46 UTC (permalink / raw) To: Jeff King; +Cc: git On 01/16/2014 18:32, Jeff King wrote: > Ah, if "more" can handle the colors, then that is preferable. > > I do think it would have to be system-specific, though. Unlike "less", > there are many implementations of "more", and quite a lot of them will > probably not support colors. We can make it a build-time option, and > set it to "on" for FreeBSD. Build-time option would be perfect. #if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) I think they all use 'more' derived from the same root. > I would go even further, and convey even more information with > colors. For example, make all dates which are less than 24 hours red. > That's an orthogonal issue. I'm not sure I agree, but if you are > interested, feel free to prepare a patch, which will get some discussion > going. Orthogonal, I know. Conveying more info in aesthetic way is always good, without making it look like a rainbow. Yuri ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 2:13 ` Jeff King 2014-01-17 2:28 ` Yuri @ 2014-01-17 2:29 ` Jonathan Nieder 2014-01-17 2:35 ` Jeff King 1 sibling, 1 reply; 39+ messages in thread From: Jonathan Nieder @ 2014-01-17 2:29 UTC (permalink / raw) To: Jeff King; +Cc: Yuri, git Hi, Jeff King wrote: > Obviously that does not help the "new unchanged user". I think we can > be smarter about guessing whether the pager can actually handle color, > based on the name. [...] > +++ b/pager.c > @@ -182,3 +182,38 @@ int check_pager_config(const char *cmd) [...] > + /* > + * We know that "more" does not pass through colors at all. > + */ > + if (!strcmp(pager, "more")) > + return 0; I seem to remember that on some systems "more" is the name of the full-featured pager that knows how to scroll forward and backward and handle color. (My memory could be faulty. A search in the makefile for DEFAULT_PAGER=more only finds AIX, which is not the platform I was thinking of.) On a stock Debian system "more" is especially primitive, which means that it passes colors through, too. It being so primitive also means it is not a particularly good choice for the PAGER setting, though, so probably that's not too important. Hope that helps, Jonathan ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 2:29 ` Jonathan Nieder @ 2014-01-17 2:35 ` Jeff King 2014-01-17 3:21 ` Jeff King 2014-01-17 4:14 ` [PATCH 0/3] improved out-of-the-box color settings Jeff King 0 siblings, 2 replies; 39+ messages in thread From: Jeff King @ 2014-01-17 2:35 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Yuri, git On Thu, Jan 16, 2014 at 06:29:21PM -0800, Jonathan Nieder wrote: > > + /* > > + * We know that "more" does not pass through colors at all. > > + */ > > + if (!strcmp(pager, "more")) > > + return 0; > > I seem to remember that on some systems "more" is the name of the > full-featured pager that knows how to scroll forward and backward and > handle color. (My memory could be faulty. A search in the makefile > for DEFAULT_PAGER=more only finds AIX, which is not the platform I was > thinking of.) Yeah, my "we know" here was still guessing. According to Yuri, FreeBSD is the system you are thinking of. :) > On a stock Debian system "more" is especially primitive, which means > that it passes colors through, too. It being so primitive also means > it is not a particularly good choice for the PAGER setting, though, > so probably that's not too important. Yeah, colors do get passed through on Debian. It's possible that other primitive "more" implementations are OK, too (and certainly we have defaulted to "on" for them until now). I think we should make an effort to set MORE=R on FreeBSD. We can perhaps just set it unconditionally, and assume that primitive "more" will ignore it. And then assume that "more" will handle colors (either because of the R setting, or because it is too dumb to escape it). I can prepare a patch series, but I happily no longer have any antique systems to test this kind of stuff on. -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 2:35 ` Jeff King @ 2014-01-17 3:21 ` Jeff King 2014-01-17 4:14 ` [PATCH 0/3] improved out-of-the-box color settings Jeff King 1 sibling, 0 replies; 39+ messages in thread From: Jeff King @ 2014-01-17 3:21 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Yuri, git On Thu, Jan 16, 2014 at 09:35:48PM -0500, Jeff King wrote: > I think we should make an effort to set MORE=R on FreeBSD. We can > perhaps just set it unconditionally, and assume that primitive "more" > will ignore it. And then assume that "more" will handle colors (either > because of the R setting, or because it is too dumb to escape it). > > I can prepare a patch series, but I happily no longer have any antique > systems to test this kind of stuff on. Meh. I figured I would have to go to an antique system to find breakage, but it is easy to do it on Debian: $ MORE=R more more: unknown option -R So we do need to make it conditional. -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 0/3] improved out-of-the-box color settings 2014-01-17 2:35 ` Jeff King 2014-01-17 3:21 ` Jeff King @ 2014-01-17 4:14 ` Jeff King 2014-01-17 4:21 ` [PATCH 1/3] setup_pager: refactor LESS/LV environment setting Jeff King ` (3 more replies) 1 sibling, 4 replies; 39+ messages in thread From: Jeff King @ 2014-01-17 4:14 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, Yuri On Thu, Jan 16, 2014 at 09:35:48PM -0500, Jeff King wrote: > I can prepare a patch series, but I happily no longer have any antique > systems to test this kind of stuff on. Here it is. For those just joining us, the problem is that on some systems, "git log" out of the box produces ANSI codes which are munged by the pager and look terrible. I'm trying to improve that by auto-configuring "more" (to get color in more cases that can handle it), and by dynamically adjusting color.pager's default based on which pager is in use (to turn off color in cases we know won't work). I was able to test my assumptions about "MORE" on Debian, and adjusted the patches accordingly from the previous discussion. I am still guessing that versions of "more" without "R" will pass through the ANSI codes as-is. That works on Debian. If anybody has an older system to test and report on, I'd love to hear it. The second patch turns on "MORE=R" only for FreeBSD. Yuri, if you can confirm that my patch works for you, that would be excellent. And if you (or anyone) has access to NetBSD or OpenBSD to test the second patch on, I'd welcome updates to config.mak.uname for those systems. We also set "LV=-c" along with "LESS". I punted on detecting that in the third patch, as I do not know the exact format of "LV". It looks like just checking for "c" might not be enough, and I need to actually understand the option format (i.e., "-c" or "-dc", but not "-Ac"). I'll leave that to somebody who cares more about LV to improve (with these patches, the behavior for them should remain the same, so it's at least not making anything worse). [1/3]: setup_pager: refactor LESS/LV environment setting [2/3]: setup_pager: set MORE=R [3/3]: pager: disable colors for some known-bad configurations -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/3] setup_pager: refactor LESS/LV environment setting 2014-01-17 4:14 ` [PATCH 0/3] improved out-of-the-box color settings Jeff King @ 2014-01-17 4:21 ` Jeff King 2014-01-17 4:21 ` [PATCH 2/3] setup_pager: set MORE=R Jeff King ` (2 subsequent siblings) 3 siblings, 0 replies; 39+ messages in thread From: Jeff King @ 2014-01-17 4:21 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, Yuri The way the code is structured now, we have to repeat ourselves in fetching the environment variables (which will get annoying as we add more). Instead, let's use an argv_array. That removes a lot of the extra conditionals and makes it easier to add new variables. It does means we'll leak the memory for the array, but: 1. This function is only called once per program. 2. We're now leaking heap memory instead of wasting BSS on the static array. Signed-off-by: Jeff King <peff@peff.net> --- I actually think we can free pager_process.env after start_command is run. You _cannot_ do so with argv, though, and I'd rather leak this tiny bit than have a hard-to-track assumption on memory lifetime buried here. pager.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/pager.c b/pager.c index 0cc75a8..90d237e 100644 --- a/pager.c +++ b/pager.c @@ -1,6 +1,7 @@ #include "cache.h" #include "run-command.h" #include "sigchain.h" +#include "argv-array.h" #ifndef DEFAULT_PAGER #define DEFAULT_PAGER "less" @@ -63,6 +64,7 @@ const char *git_pager(int stdout_is_tty) void setup_pager(void) { const char *pager = git_pager(isatty(1)); + struct argv_array env = ARGV_ARRAY_INIT; if (!pager || pager_in_use()) return; @@ -80,17 +82,13 @@ void setup_pager(void) pager_process.use_shell = 1; pager_process.argv = pager_argv; pager_process.in = -1; - if (!getenv("LESS") || !getenv("LV")) { - static const char *env[3]; - int i = 0; - - if (!getenv("LESS")) - env[i++] = "LESS=FRSX"; - if (!getenv("LV")) - env[i++] = "LV=-c"; - env[i] = NULL; - pager_process.env = env; - } + + if (!getenv("LESS")) + argv_array_push(&env, "LESS=FRSX"); + if (!getenv("LV")) + argv_array_push(&env, "LV=-c"); + pager_process.env = argv_array_detach(&env, NULL); + if (start_command(&pager_process)) return; -- 1.8.5.2.500.g8060133 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/3] setup_pager: set MORE=R 2014-01-17 4:14 ` [PATCH 0/3] improved out-of-the-box color settings Jeff King 2014-01-17 4:21 ` [PATCH 1/3] setup_pager: refactor LESS/LV environment setting Jeff King @ 2014-01-17 4:21 ` Jeff King 2014-01-17 7:26 ` Kyle J. McKay 2014-01-17 19:17 ` Junio C Hamano 2014-01-17 4:24 ` [PATCH 3/3] pager: disable colors for some known-bad configurations Jeff King 2014-01-17 9:13 ` [PATCH 0/3] improved out-of-the-box color settings Yuri 3 siblings, 2 replies; 39+ messages in thread From: Jeff King @ 2014-01-17 4:21 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, Yuri When we run the pager, we always set "LESS=R" to tell the pager to pass through ANSI colors. On modern versions of FreeBSD, the system "more" can do the same trick. Unlike "less", we may be dealing with various versions of "more" that do not understand the "R" option, but do know how to read options from MORE (Debian's "more" is an example). For this reason, we make the setting a compile-time option (but turn it on by default on FreeBSD). Signed-off-by: Jeff King <peff@peff.net> --- Makefile | 7 +++++++ config.mak.uname | 1 + pager.c | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/Makefile b/Makefile index b4af1e2..6cd7a2a 100644 --- a/Makefile +++ b/Makefile @@ -312,6 +312,9 @@ all:: # you want to use something different. The value will be interpreted by the # shell at runtime when it is used. # +# Define PAGER_MORE_UNDERSTANDS_R if your system's "more" pager +# can pass-through ANSI colors using the "R" option. +# # Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you # want to use something different. The value will be interpreted by the shell # if necessary when it is used. Examples: @@ -1608,6 +1611,10 @@ DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ)) BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)' endif +ifdef PAGER_MORE_UNDERSTANDS_R +BASIC_CFLAGS += -DPAGER_MORE_UNDERSTANDS_R +endif + ifdef SHELL_PATH SHELL_PATH_CQ = "$(subst ",\",$(subst \,\\,$(SHELL_PATH)))" SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ)) diff --git a/config.mak.uname b/config.mak.uname index 7d31fad..d63babc 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -188,6 +188,7 @@ ifeq ($(uname_S),FreeBSD) endif PYTHON_PATH = /usr/local/bin/python HAVE_PATHS_H = YesPlease + PAGER_MORE_UNDERSTANDS_R = YesPlease endif ifeq ($(uname_S),OpenBSD) NO_STRCASESTR = YesPlease diff --git a/pager.c b/pager.c index 90d237e..2303164 100644 --- a/pager.c +++ b/pager.c @@ -87,6 +87,10 @@ void setup_pager(void) argv_array_push(&env, "LESS=FRSX"); if (!getenv("LV")) argv_array_push(&env, "LV=-c"); +#ifdef PAGER_MORE_UNDERSTANDS_R + if (!getenv("MORE")) + argv_array_push(&env, "MORE=R"); +#endif pager_process.env = argv_array_detach(&env, NULL); if (start_command(&pager_process)) -- 1.8.5.2.500.g8060133 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-17 4:21 ` [PATCH 2/3] setup_pager: set MORE=R Jeff King @ 2014-01-17 7:26 ` Kyle J. McKay 2014-01-17 19:11 ` Junio C Hamano 2014-01-21 5:30 ` Jeff King 2014-01-17 19:17 ` Junio C Hamano 1 sibling, 2 replies; 39+ messages in thread From: Kyle J. McKay @ 2014-01-17 7:26 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Jonathan Nieder, Yuri On Jan 16, 2014, at 20:21, Jeff King wrote: > When we run the pager, we always set "LESS=R" to tell the > pager to pass through ANSI colors. On modern versions of > FreeBSD, the system "more" can do the same trick. [snip] > diff --git a/pager.c b/pager.c > index 90d237e..2303164 100644 > --- a/pager.c > +++ b/pager.c > @@ -87,6 +87,10 @@ void setup_pager(void) > argv_array_push(&env, "LESS=FRSX"); > if (!getenv("LV")) > argv_array_push(&env, "LV=-c"); > +#ifdef PAGER_MORE_UNDERSTANDS_R > + if (!getenv("MORE")) > + argv_array_push(&env, "MORE=R"); > +#endif How about adding a leading "-" to both the LESS and MORE settings? Since you're in there patching... :) The man page for more states: "Options are also taken from the environment variable MORE (make sure to precede them with a dash (``-'')) but command line options will override them." And while the less man page does not have that wording, it does show this: LESS="-options"; export LESS and this: LESS="-Dn9.1$-Ds4.1" So it looks like both LESS and MORE prefer to have their options start with a '-' more or less. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-17 7:26 ` Kyle J. McKay @ 2014-01-17 19:11 ` Junio C Hamano 2014-01-21 5:30 ` Jeff King 1 sibling, 0 replies; 39+ messages in thread From: Junio C Hamano @ 2014-01-17 19:11 UTC (permalink / raw) To: Kyle J. McKay; +Cc: Jeff King, Git Mailing List, Jonathan Nieder, Yuri "Kyle J. McKay" <mackyle@gmail.com> writes: > On Jan 16, 2014, at 20:21, Jeff King wrote: >> When we run the pager, we always set "LESS=R" to tell the >> pager to pass through ANSI colors. On modern versions of >> FreeBSD, the system "more" can do the same trick. > [snip] >> diff --git a/pager.c b/pager.c >> index 90d237e..2303164 100644 >> --- a/pager.c >> +++ b/pager.c >> @@ -87,6 +87,10 @@ void setup_pager(void) >> argv_array_push(&env, "LESS=FRSX"); >> if (!getenv("LV")) >> argv_array_push(&env, "LV=-c"); >> +#ifdef PAGER_MORE_UNDERSTANDS_R >> + if (!getenv("MORE")) >> + argv_array_push(&env, "MORE=R"); >> +#endif > > How about adding a leading "-" to both the LESS and MORE settings? > Since you're in there patching... :) The discussion we had when LV=-c was added, namely $gmane/240124, agrees. I however am perfectly fine to see it done as a separate clean-up patch. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-17 7:26 ` Kyle J. McKay 2014-01-17 19:11 ` Junio C Hamano @ 2014-01-21 5:30 ` Jeff King 2014-01-21 8:42 ` Kyle J. McKay 1 sibling, 1 reply; 39+ messages in thread From: Jeff King @ 2014-01-21 5:30 UTC (permalink / raw) To: Kyle J. McKay; +Cc: Git Mailing List, Jonathan Nieder, Yuri On Thu, Jan 16, 2014 at 11:26:50PM -0800, Kyle J. McKay wrote: > >--- a/pager.c > >+++ b/pager.c > >@@ -87,6 +87,10 @@ void setup_pager(void) > > argv_array_push(&env, "LESS=FRSX"); > > if (!getenv("LV")) > > argv_array_push(&env, "LV=-c"); > >+#ifdef PAGER_MORE_UNDERSTANDS_R > >+ if (!getenv("MORE")) > >+ argv_array_push(&env, "MORE=R"); > >+#endif > > How about adding a leading "-" to both the LESS and MORE settings? > Since you're in there patching... :) I do not have any problem with that, but would very much prefer it as a separate patch, in case there are any fallouts. > And while the less man page does not have that wording, it does show > this: > > LESS="-options"; export LESS > > and this: > > LESS="-Dn9.1$-Ds4.1" Ugh. Having just read the LESS discussion, it makes me wonder if the strchr(getenv("LESS"), 'R') check I add elsewhere in the series is sufficient. I suspect in practice it is good enough, but I would not be surprised if there is a way to fool it with a sufficiently complex LESS variable. Maybe we should just assume it is enough, and people with crazy LESS settings can set color.pager as appropriate? ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-21 5:30 ` Jeff King @ 2014-01-21 8:42 ` Kyle J. McKay 2014-01-23 2:14 ` Jeff King 0 siblings, 1 reply; 39+ messages in thread From: Kyle J. McKay @ 2014-01-21 8:42 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Jonathan Nieder, Yuri On Jan 20, 2014, at 21:30, Jeff King wrote: > Ugh. Having just read the LESS discussion, it makes me wonder if the > > strchr(getenv("LESS"), 'R') > > check I add elsewhere in the series is sufficient. I suspect in > practice > it is good enough, but I would not be surprised if there is a way to > fool it with a sufficiently complex LESS variable. Maybe we should > just > assume it is enough, and people with crazy LESS settings can set > color.pager as appropriate? This attempts to show colors but doesn't work properly: LESS=-+R git -c color.ui=auto -c color.pager=true log --oneline -- graph but this does LESS=-R git -c color.ui=auto -c color.pager=true log --oneline -- graph so yeah, just checking for 'R' in $LESS is only approximate. :) Also -r is good enough to show colors too... ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-21 8:42 ` Kyle J. McKay @ 2014-01-23 2:14 ` Jeff King 2014-01-23 17:22 ` Junio C Hamano 0 siblings, 1 reply; 39+ messages in thread From: Jeff King @ 2014-01-23 2:14 UTC (permalink / raw) To: Kyle J. McKay; +Cc: Git Mailing List, Jonathan Nieder, Yuri On Tue, Jan 21, 2014 at 12:42:58AM -0800, Kyle J. McKay wrote: > This attempts to show colors but doesn't work properly: > > LESS=-+R git -c color.ui=auto -c color.pager=true log --oneline --graph > > but this does > > LESS=-R git -c color.ui=auto -c color.pager=true log --oneline --graph > > so yeah, just checking for 'R' in $LESS is only approximate. :) > Also -r is good enough to show colors too... Ugh, yeah. I think LESS=+R would also break. Doing it right would involve parsing less's option format, including "-", "+", and "-+". It's not _that_ much code, but it feels awfully wrong to be so intimate with a subprogram that we do not control. -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-23 2:14 ` Jeff King @ 2014-01-23 17:22 ` Junio C Hamano 0 siblings, 0 replies; 39+ messages in thread From: Junio C Hamano @ 2014-01-23 17:22 UTC (permalink / raw) To: Jeff King; +Cc: Kyle J. McKay, Git Mailing List, Jonathan Nieder, Yuri Jeff King <peff@peff.net> writes: > ..., but it feels awfully wrong to be so intimate with > a subprogram that we do not control. Yeah, I think we are in agreement on that point. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-17 4:21 ` [PATCH 2/3] setup_pager: set MORE=R Jeff King 2014-01-17 7:26 ` Kyle J. McKay @ 2014-01-17 19:17 ` Junio C Hamano 2014-01-17 19:57 ` Junio C Hamano 2014-01-21 5:49 ` Jeff King 1 sibling, 2 replies; 39+ messages in thread From: Junio C Hamano @ 2014-01-17 19:17 UTC (permalink / raw) To: Jeff King; +Cc: git, Jonathan Nieder, Yuri Jeff King <peff@peff.net> writes: > diff --git a/pager.c b/pager.c > index 90d237e..2303164 100644 > --- a/pager.c > +++ b/pager.c > @@ -87,6 +87,10 @@ void setup_pager(void) > argv_array_push(&env, "LESS=FRSX"); > if (!getenv("LV")) > argv_array_push(&env, "LV=-c"); > +#ifdef PAGER_MORE_UNDERSTANDS_R > + if (!getenv("MORE")) > + argv_array_push(&env, "MORE=R"); > +#endif > pager_process.env = argv_array_detach(&env, NULL); > > if (start_command(&pager_process)) Let me repeat from $gmane/240110: - Can we generalize this a bit so that a builder can pass a list of var=val pairs and demote the existing LESS=FRSX to just a canned setting of such a mechanism? As we need to maintain this "set these environments when unset" here and also in git-sh-setup.sh, I think it is about time to do that clean-up. Duplicating two settings was borderline OK, but seeing the third added fairly soon after the second was added tells me that the clean-up must come before adding the third. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-17 19:17 ` Junio C Hamano @ 2014-01-17 19:57 ` Junio C Hamano 2014-01-17 23:42 ` Junio C Hamano 2014-01-21 5:49 ` Jeff King 1 sibling, 1 reply; 39+ messages in thread From: Junio C Hamano @ 2014-01-17 19:57 UTC (permalink / raw) To: Jeff King; +Cc: git, Jonathan Nieder, Yuri Junio C Hamano <gitster@pobox.com> writes: > Jeff King <peff@peff.net> writes: > >> diff --git a/pager.c b/pager.c >> index 90d237e..2303164 100644 >> --- a/pager.c >> +++ b/pager.c >> @@ -87,6 +87,10 @@ void setup_pager(void) >> argv_array_push(&env, "LESS=FRSX"); >> if (!getenv("LV")) >> argv_array_push(&env, "LV=-c"); >> +#ifdef PAGER_MORE_UNDERSTANDS_R >> + if (!getenv("MORE")) >> + argv_array_push(&env, "MORE=R"); >> +#endif >> pager_process.env = argv_array_detach(&env, NULL); >> >> if (start_command(&pager_process)) > > Let me repeat from $gmane/240110: > > - Can we generalize this a bit so that a builder can pass a list > of var=val pairs and demote the existing LESS=FRSX to just a > canned setting of such a mechanism? > > As we need to maintain this "set these environments when unset" here > and also in git-sh-setup.sh, I think it is about time to do that > clean-up. Duplicating two settings was borderline OK, but seeing > the third added fairly soon after the second was added tells me that > the clean-up must come before adding the third. Perhaps we can start it like this. Then pager.c can iterate over the PAGER_ENV string, parse out LESS=, LV=, etc., and do its thing. We would also need to muck with git-sh-setup.sh but that file is already preprocessed in the Makefile, so we should be able to do something similar to "# @@BROKEN_PATH_FIX@@" there. Makefile | 15 ++++++++++++++- config.mak.uname | 1 + 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index b4af1e2..c9e7847 100644 --- a/Makefile +++ b/Makefile @@ -342,6 +342,14 @@ all:: # Define DEFAULT_HELP_FORMAT to "man", "info" or "html" # (defaults to "man") if you want to have a different default when # "git help" is called without a parameter specifying the format. +# +# Define PAGER_ENV to a SP separated VAR=VAL pairs to define +# default environment variables to be passed when a pager is spawned, e.g. +# +# PAGER_ENV = LESS=-FRSX LV=-c +# +# to say "export LESS=-FRSX (and LV=-c) if the environment variable +# LESS (and LV) is not set, respectively". GIT-VERSION-FILE: FORCE @$(SHELL_PATH) ./GIT-VERSION-GEN @@ -1506,6 +1514,10 @@ ifeq ($(PYTHON_PATH),) NO_PYTHON = NoThanks endif +ifndef PAGER_ENV +PAGER_ENV = LESS=-FRSX LV=-c +endif + QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir QUIET_SUBDIR1 = @@ -1585,11 +1597,12 @@ PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH)) TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH)) DIFF_SQ = $(subst ','\'',$(DIFF)) PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA)) +PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV)) LIBS = $(GITLIBS) $(EXTLIBS) BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \ - $(COMPAT_CFLAGS) + $(COMPAT_CFLAGS) -DPAGER_ENV='$(PAGER_ENV_SQ)' LIB_OBJS += $(COMPAT_OBJS) # Quote for C diff --git a/config.mak.uname b/config.mak.uname index 7d31fad..8aea8a6 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -188,6 +188,7 @@ ifeq ($(uname_S),FreeBSD) endif PYTHON_PATH = /usr/local/bin/python HAVE_PATHS_H = YesPlease + PAGER_ENV = LESS=-FRSX LV=-c MORE=-R endif ifeq ($(uname_S),OpenBSD) NO_STRCASESTR = YesPlease ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-17 19:57 ` Junio C Hamano @ 2014-01-17 23:42 ` Junio C Hamano 2014-01-21 6:13 ` Jeff King 0 siblings, 1 reply; 39+ messages in thread From: Junio C Hamano @ 2014-01-17 23:42 UTC (permalink / raw) To: Jeff King; +Cc: git, Jonathan Nieder, Yuri Junio C Hamano <gitster@pobox.com> writes: > Perhaps we can start it like this. Then pager.c can iterate over > the PAGER_ENV string, parse out LESS=, LV=, etc., and do its thing. > > We would also need to muck with git-sh-setup.sh but that file is > already preprocessed in the Makefile, so we should be able to do > something similar to "# @@BROKEN_PATH_FIX@@" there. And here is such an attempt. There may be some missing dependencies that need to be covered with a mechanism like GIT-BUILD-OPTIONS, though. Makefile | 18 ++++++++++++++++-- config.mak.uname | 1 + git-sh-setup.sh | 9 +++++---- pager.c | 44 +++++++++++++++++++++++++++++++++----------- 4 files changed, 55 insertions(+), 17 deletions(-) diff --git a/Makefile b/Makefile index b4af1e2..690f4c6 100644 --- a/Makefile +++ b/Makefile @@ -342,6 +342,14 @@ all:: # Define DEFAULT_HELP_FORMAT to "man", "info" or "html" # (defaults to "man") if you want to have a different default when # "git help" is called without a parameter specifying the format. +# +# Define PAGER_ENV to a SP separated VAR=VAL pairs to define +# default environment variables to be passed when a pager is spawned, e.g. +# +# PAGER_ENV = LESS=-FRSX LV=-c +# +# to say "export LESS=-FRSX (and LV=-c) if the environment variable +# LESS (and LV) is not set, respectively". GIT-VERSION-FILE: FORCE @$(SHELL_PATH) ./GIT-VERSION-GEN @@ -1506,6 +1514,10 @@ ifeq ($(PYTHON_PATH),) NO_PYTHON = NoThanks endif +ifndef PAGER_ENV +PAGER_ENV = LESS=-FRSX LV=-c +endif + QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir QUIET_SUBDIR1 = @@ -1585,11 +1597,12 @@ PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH)) TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH)) DIFF_SQ = $(subst ','\'',$(DIFF)) PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA)) +PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV)) LIBS = $(GITLIBS) $(EXTLIBS) BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \ - $(COMPAT_CFLAGS) + $(COMPAT_CFLAGS) -DPAGER_ENV='$(PAGER_ENV)' LIB_OBJS += $(COMPAT_OBJS) # Quote for C @@ -1739,7 +1752,7 @@ common-cmds.h: $(wildcard Documentation/git-*.txt) SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\ $(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\ - $(gitwebdir_SQ):$(PERL_PATH_SQ) + $(gitwebdir_SQ):$(PERL_PATH_SQ):$(PAGER_ENV) define cmd_munge_script $(RM) $@ $@+ && \ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ @@ -1751,6 +1764,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ -e $(BROKEN_PATH_FIX) \ -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \ -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \ + -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \ $@.sh >$@+ endef diff --git a/config.mak.uname b/config.mak.uname index 7d31fad..8aea8a6 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -188,6 +188,7 @@ ifeq ($(uname_S),FreeBSD) endif PYTHON_PATH = /usr/local/bin/python HAVE_PATHS_H = YesPlease + PAGER_ENV = LESS=-FRSX LV=-c MORE=-R endif ifeq ($(uname_S),OpenBSD) NO_STRCASESTR = YesPlease diff --git a/git-sh-setup.sh b/git-sh-setup.sh index fffa3c7..8fc1bbd 100644 --- a/git-sh-setup.sh +++ b/git-sh-setup.sh @@ -158,10 +158,11 @@ git_pager() { else GIT_PAGER=cat fi - : ${LESS=-FRSX} - : ${LV=-c} - export LESS LV - + for vardef in @@PAGER_ENV@@ + do + var=${vardef%%=*} + eval ": \${$vardef} && export $var" + done eval "$GIT_PAGER" '"$@"' } diff --git a/pager.c b/pager.c index 0cc75a8..81a8af7 100644 --- a/pager.c +++ b/pager.c @@ -1,6 +1,7 @@ #include "cache.h" #include "run-command.h" #include "sigchain.h" +#include "argv-array.h" #ifndef DEFAULT_PAGER #define DEFAULT_PAGER "less" @@ -60,9 +61,37 @@ const char *git_pager(int stdout_is_tty) return pager; } +#define stringify_(x) #x +#define stringify(x) stringify_(x) + +static void setup_pager_env(struct argv_array *env) +{ + const char *pager_env = stringify(PAGER_ENV); + + while (*pager_env) { + struct strbuf buf = STRBUF_INIT; + const char *cp = strchrnul(pager_env, '='); + + if (!*cp) + die("malformed build-time PAGER_ENV"); + strbuf_add(&buf, pager_env, cp - pager_env); + cp = strchrnul(pager_env, ' '); + if (!getenv(buf.buf)) { + strbuf_reset(&buf); + strbuf_add(&buf, pager_env, cp - pager_env); + argv_array_push(env, strbuf_detach(&buf, NULL)); + } + strbuf_reset(&buf); + while (*cp && *cp == ' ') + cp++; + pager_env = cp; + } +} + void setup_pager(void) { const char *pager = git_pager(isatty(1)); + struct argv_array env = ARGV_ARRAY_INIT; if (!pager || pager_in_use()) return; @@ -80,17 +109,10 @@ void setup_pager(void) pager_process.use_shell = 1; pager_process.argv = pager_argv; pager_process.in = -1; - if (!getenv("LESS") || !getenv("LV")) { - static const char *env[3]; - int i = 0; - - if (!getenv("LESS")) - env[i++] = "LESS=FRSX"; - if (!getenv("LV")) - env[i++] = "LV=-c"; - env[i] = NULL; - pager_process.env = env; - } + + setup_pager_env(&env); + pager_process.env = argv_array_detach(&env, NULL); + if (start_command(&pager_process)) return; ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-17 23:42 ` Junio C Hamano @ 2014-01-21 6:13 ` Jeff King 2014-01-21 19:28 ` Junio C Hamano 0 siblings, 1 reply; 39+ messages in thread From: Jeff King @ 2014-01-21 6:13 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder, Yuri On Fri, Jan 17, 2014 at 03:42:32PM -0800, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Perhaps we can start it like this. Then pager.c can iterate over > > the PAGER_ENV string, parse out LESS=, LV=, etc., and do its thing. > > > > We would also need to muck with git-sh-setup.sh but that file is > > already preprocessed in the Makefile, so we should be able to do > > something similar to "# @@BROKEN_PATH_FIX@@" there. > > And here is such an attempt. There may be some missing dependencies > that need to be covered with a mechanism like GIT-BUILD-OPTIONS, > though. Perhaps instead of just dumping it into a macro, we could actually parse it at compile time into C code, and store the result as a header file. Then that header file becomes the proper dependency, and this run-time error: > + while (*pager_env) { > + struct strbuf buf = STRBUF_INIT; > + const char *cp = strchrnul(pager_env, '='); > + > + if (!*cp) > + die("malformed build-time PAGER_ENV"); would become a compile-time error. We could do the same for the shell code, too. I'm thinking something like: diff --git a/Makefile b/Makefile index b4af1e2..3a8d15e 100644 --- a/Makefile +++ b/Makefile @@ -2182,6 +2182,16 @@ GIT-LDFLAGS: FORCE echo "$$FLAGS" >GIT-LDFLAGS; \ fi +GIT-PAGER-ENV: + @PAGER_ENV='$(PAGER_ENV)'; \ + if test x"$$PAGER_ENV" != x"`cat GIT-PAGER-ENV 2>/dev/null`"; then \ + echo "$$PAGER_ENV" >GIT-PAGER-ENV; \ + fi + +pager-env.h: GIT-PAGER-ENV mkcarray + $(SHELL_PATH) mkcarray pager_env <$< >$@+ + mv $@+ $@ + # We need to apply sq twice, once to protect from the shell # that runs GIT-BUILD-OPTIONS, and then again to protect it # and the first level quoting from the shell that runs "echo". diff --git a/mkcarray b/mkcarray index e69de29..5962440 100644 --- a/mkcarray +++ b/mkcarray @@ -0,0 +1,21 @@ +name=$1; shift +guard=$(echo "$name" | tr a-z A-Z) + +cat <<-EOF +#ifndef ${guard}_H +#define ${guard}_H + +static const char *${name} = { +EOF + +for i in $(cat); do + # XXX c-quote the values? + printf '\t"%s",\n' "$i" +done + +cat <<EOF + NULL +}; + +#endif /* ${guard}_H */ +EOF ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-21 6:13 ` Jeff King @ 2014-01-21 19:28 ` Junio C Hamano 0 siblings, 0 replies; 39+ messages in thread From: Junio C Hamano @ 2014-01-21 19:28 UTC (permalink / raw) To: Jeff King; +Cc: git, Jonathan Nieder, Yuri Jeff King <peff@peff.net> writes: > Perhaps instead of just dumping it into a macro, we could actually parse > it at compile time into C code, and store the result as a header file. > Then that header file becomes the proper dependency, and this run-time > error: > >> + while (*pager_env) { >> + struct strbuf buf = STRBUF_INIT; >> + const char *cp = strchrnul(pager_env, '='); >> + >> + if (!*cp) >> + die("malformed build-time PAGER_ENV"); > > would become a compile-time error. We could do the same for the shell > code, too. > > I'm thinking something like: Heh, great minds think alike. I did start writing a toy-patch with a new file called "pager-env.h", before discarding it and sending a simpler alternative which you are responding to. I do not mind the approach at all; the primary reason I stopped doing that was because the amount of code to get quotes right turned out to be sufficiently big to obscure the real point of the "how about doing it this way" illustration patch. > diff --git a/Makefile b/Makefile > index b4af1e2..3a8d15e 100644 > --- a/Makefile > +++ b/Makefile > @@ -2182,6 +2182,16 @@ GIT-LDFLAGS: FORCE > echo "$$FLAGS" >GIT-LDFLAGS; \ > fi > > +GIT-PAGER-ENV: > + @PAGER_ENV='$(PAGER_ENV)'; \ > + if test x"$$PAGER_ENV" != x"`cat GIT-PAGER-ENV 2>/dev/null`"; then \ > + echo "$$PAGER_ENV" >GIT-PAGER-ENV; \ > + fi > + > +pager-env.h: GIT-PAGER-ENV mkcarray > + $(SHELL_PATH) mkcarray pager_env <$< >$@+ > + mv $@+ $@ > + > # We need to apply sq twice, once to protect from the shell > # that runs GIT-BUILD-OPTIONS, and then again to protect it > # and the first level quoting from the shell that runs "echo". > diff --git a/mkcarray b/mkcarray > index e69de29..5962440 100644 > --- a/mkcarray > +++ b/mkcarray > @@ -0,0 +1,21 @@ > +name=$1; shift > +guard=$(echo "$name" | tr a-z A-Z) > + > +cat <<-EOF > +#ifndef ${guard}_H > +#define ${guard}_H > + > +static const char *${name} = { > +EOF > + > +for i in $(cat); do > + # XXX c-quote the values? > + printf '\t"%s",\n' "$i" > +done > + > +cat <<EOF > + NULL > +}; > + > +#endif /* ${guard}_H */ > +EOF ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-17 19:17 ` Junio C Hamano 2014-01-17 19:57 ` Junio C Hamano @ 2014-01-21 5:49 ` Jeff King 2014-01-21 19:23 ` Junio C Hamano 1 sibling, 1 reply; 39+ messages in thread From: Jeff King @ 2014-01-21 5:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder, Yuri On Fri, Jan 17, 2014 at 11:17:01AM -0800, Junio C Hamano wrote: > > +#ifdef PAGER_MORE_UNDERSTANDS_R > > + if (!getenv("MORE")) > > + argv_array_push(&env, "MORE=R"); > > +#endif > > pager_process.env = argv_array_detach(&env, NULL); > > > > if (start_command(&pager_process)) > > Let me repeat from $gmane/240110: > > - Can we generalize this a bit so that a builder can pass a list > of var=val pairs and demote the existing LESS=FRSX to just a > canned setting of such a mechanism? > > As we need to maintain this "set these environments when unset" here > and also in git-sh-setup.sh, I think it is about time to do that > clean-up. Duplicating two settings was borderline OK, but seeing > the third added fairly soon after the second was added tells me that > the clean-up must come before adding the third. Wow, I somehow _completely_ missed that thread. When I looked at the code with LV, I assumed it was just something that had happened long ago and I had forgotten about. I didn't even bother reading "git log". Yeah, I agree that git-sh-setup should be consistent with what the C porcelains do. However, adding build-time variables like: PAGER_ENV = LESS=-FRSX LV=-c does complicate the point of my series, which was to add more intimate logic about how we handle LESS. With the current code, I can know that an unset "LESS" variable means we will set "R" ourselves and turn on color. We can get around that with something like: int pager_can_handle_color(void) { const char *pager = get_pager(); if (!strcmp(pager, "less")) { const char *x = getenv("LESS"); if (!x) { const char *e; for (e = pager_env; *e; e++) if ((x = skip_prefix(e, "LESS="))) break; } return !x || strchr(x, 'R'); } [...] } but we are still hard-coding a lot of intelligence about "less" here. I wonder if the abstraction provided by the Makefile variable is really worthwhile. Anybody adding a new line to it would also want to tweak pager_can_handle_color to add similar logic. Taking a step back for a moment, we are getting two things out of such a Makefile variable: 1. An easy way for packager to add intelligence about common pagers on their system. 2. DRY between git-sh-setup and the C code. I do like (1), but I do not know if we want to try to encode the "can handle color" logic into a Makefile variable. What would it look like? For (2), an alternate method would be to simply provide "git pager" as C code, which spawns the appropriate pager as the C code would. Then git-sh-setup can easily build around that. -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-21 5:49 ` Jeff King @ 2014-01-21 19:23 ` Junio C Hamano 2014-02-04 22:12 ` Jeff King 0 siblings, 1 reply; 39+ messages in thread From: Junio C Hamano @ 2014-01-21 19:23 UTC (permalink / raw) To: Jeff King; +Cc: git, Jonathan Nieder, Yuri Jeff King <peff@peff.net> writes: > ... > does complicate the point of my series, which was to add more intimate > logic about how we handle LESS. > ... > return !x || strchr(x, 'R'); > } > > [...] > } > > but we are still hard-coding a lot of intelligence about "less" here. I am not sure if it is even a good idea for us to have so intimate logic for various pagers in the first place. I'd seriously wonder if it is better to take this position: A platform packager who sets the default pager and/or the default environment for the pager at the build time, or an individual user who tells your Git what pager you want to use and/or with what setting that pager should be run under with environment variables. These people ought to know far better than Git what their specific choices do. Do not try to second-guess them. The potential breakage caused by setting MORE=R unconditionally is a good example. A careless "intimate logic" may think that any pager that is called 'more' would behave like traditional 'more', breaking half the 'more' user population while catering to the other half. > I > wonder if the abstraction provided by the Makefile variable is really > worthwhile. Anybody adding a new line to it would also want to tweak > pager_can_handle_color to add similar logic. And that is why I am not enthused by the idea of adding such logic in the first place. I view the Makefile customization as a way for the packager to offer a sensible default for their platform without touching the code, which is slightly different from your 1. below. > Taking a step back for a moment, we are getting two things out of such a > Makefile variable: > > 1. An easy way for packager to add intelligence about common pagers on > their system. > > 2. DRY between git-sh-setup and the C code. > > I do like (1), but I do not know if we want to try to encode the "can > handle color" logic into a Makefile variable. What would it look like? > > For (2), an alternate method would be to simply provide "git pager" as C > code, which spawns the appropriate pager as the C code would. Then > git-sh-setup can easily build around that. And as to 2., if the answer to the other issue "do we want our code to be intimately aware of pager-specific quirks, or do we just want to give packagers a knob to express their choice of the default?" resolves to the former, I would think that "git pager" would be not just a workable alternative, but would be the only viable one. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-01-21 19:23 ` Junio C Hamano @ 2014-02-04 22:12 ` Jeff King 2014-02-04 22:17 ` Junio C Hamano 2014-02-05 2:11 ` Kyle J. McKay 0 siblings, 2 replies; 39+ messages in thread From: Jeff King @ 2014-02-04 22:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder, Yuri On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote: > > does complicate the point of my series, which was to add more intimate > > logic about how we handle LESS. > > ... > > return !x || strchr(x, 'R'); > [...] > > I am not sure if it is even a good idea for us to have so intimate > logic for various pagers in the first place. I'd seriously wonder > if it is better to take this position: > > A platform packager who sets the default pager and/or the > default environment for the pager at the build time, or an > individual user who tells your Git what pager you want to > use and/or with what setting that pager should be run under > with environment variables. These people ought to know far > better than Git what their specific choices do. Do not try > to second-guess them. Sorry for letting this topic lapse; I wanted to look at some possible Makefile improvements, which I'll be posting in a moment. I did want to address this point, though. If we are just talking about packagers and defaults (e.g., setting MORE=R on FreeBSD), then I agree that the right tool for the job is empowering the packagers, and the Makefile knob we've discussed does that. But there's another set of people that I was intending to help with the patch, which is people that have set up LESS, and did not necessarily care about the "R" flag in the past (e.g., for many years before git came along, I set LESS=giM, and never even knew that "R" existed). Since git comes out of the box these days with color and the pager turned on, that means people with such a setup see broken output from day one. And I think it is Git's fault here, not the user or the packager. Our defaults assume that the user's pager can handle color, and that is not the default for many pagers, including our default "less"! The user did not turn off "R" here; they simply set other options they cared about, and our hacky workaround to auto-enable "R" did not kick in. It seems a shame to me that we cannot do better for such users. However, the level of intimacy with the pager is getting to be a bit too much for my taste, and the saving grace is that not that many people set LESS themselves (and git is widely enough known that "R" is a common recommendation when people do). So it's probably the lesser of two evils to ignore the situation, and let people who run into it find the answers on the web. So I think there is nothing to be done. But I did want to mention it in case somebody else can come up with some clever solution. :) -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-02-04 22:12 ` Jeff King @ 2014-02-04 22:17 ` Junio C Hamano 2014-02-04 22:25 ` Jeff King 2014-02-05 2:11 ` Kyle J. McKay 1 sibling, 1 reply; 39+ messages in thread From: Junio C Hamano @ 2014-02-04 22:17 UTC (permalink / raw) To: Jeff King; +Cc: git, Jonathan Nieder, Yuri Jeff King <peff@peff.net> writes: > But there's another set of people that I was intending to help with the > patch, which is people that have set up LESS, and did not necessarily > care about the "R" flag in the past (e.g., for many years before git > came along, I set LESS=giM, and never even knew that "R" existed). Since > git comes out of the box these days with color and the pager turned on, > that means people with such a setup see broken output from day one. > > And I think it is Git's fault here, not the user or the packager. I am not particularly itnterested in whose fault it is ;-) If the user sets LESS himself, he knows how to set it (and if he is setting it automatically for all of his sessions, he knows where to do so), and would know better than Git about "less", his pager of choice. If he did not know about R and did not see color, that is even better. Now he knows and his update to his LESS settings will let him view colors in outputs from programs that are not Git. > So I think there is nothing to be done. But I did want to mention it in > case somebody else can come up with some clever solution. :) Sure. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-02-04 22:17 ` Junio C Hamano @ 2014-02-04 22:25 ` Jeff King 2014-02-04 22:45 ` Yuri 0 siblings, 1 reply; 39+ messages in thread From: Jeff King @ 2014-02-04 22:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder, Yuri On Tue, Feb 04, 2014 at 02:17:57PM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > But there's another set of people that I was intending to help with the > > patch, which is people that have set up LESS, and did not necessarily > > care about the "R" flag in the past (e.g., for many years before git > > came along, I set LESS=giM, and never even knew that "R" existed). Since > > git comes out of the box these days with color and the pager turned on, > > that means people with such a setup see broken output from day one. > > > > And I think it is Git's fault here, not the user or the packager. > > I am not particularly itnterested in whose fault it is ;-) If the > user sets LESS himself, he knows how to set it (and if he is setting > it automatically for all of his sessions, he knows where to do so), > and would know better than Git about "less", his pager of choice. > > If he did not know about R and did not see color, that is even > better. Now he knows and his update to his LESS settings will let > him view colors in outputs from programs that are not Git. Right. If git just disabled the color, I think that would be sane (and that is what my patch was shooting for). But somebody who sees: $ git log ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m does not necessarily know what is going on. They do not know that it is a "less" problem, nor that their less settings are relevant. They only see that Git is broken out of the box. Anyway, I will leave it at that. It's an unfortunate problem, but one not worth fixing. -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-02-04 22:25 ` Jeff King @ 2014-02-04 22:45 ` Yuri 2014-02-04 22:48 ` Jeff King 0 siblings, 1 reply; 39+ messages in thread From: Yuri @ 2014-02-04 22:45 UTC (permalink / raw) To: Jeff King, Junio C Hamano; +Cc: git, Jonathan Nieder On 02/04/2014 14:25, Jeff King wrote: > Right. If git just disabled the color, I think that would be sane (and > that is what my patch was shooting for). But somebody who sees: > > $ git log > ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m > (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m > > does not necessarily know what is going on. They do not know that it is > a "less" problem, nor that their less settings are relevant. They only > see that Git is broken out of the box. Maybe, instead of doing all the elaborate guess and assumption work, have configure script check if the current PAGER supports colors and build git accordingly? configure could run the pager as one of its tests, and determine if "ESC" appears on the output. Yuri ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-02-04 22:45 ` Yuri @ 2014-02-04 22:48 ` Jeff King 2014-02-04 22:54 ` Junio C Hamano 2014-02-04 23:00 ` Yuri 0 siblings, 2 replies; 39+ messages in thread From: Jeff King @ 2014-02-04 22:48 UTC (permalink / raw) To: Yuri; +Cc: Junio C Hamano, git, Jonathan Nieder On Tue, Feb 04, 2014 at 02:45:11PM -0800, Yuri wrote: > On 02/04/2014 14:25, Jeff King wrote: > >Right. If git just disabled the color, I think that would be sane (and > >that is what my patch was shooting for). But somebody who sees: > > > > $ git log > > ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m > > (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m > > > >does not necessarily know what is going on. They do not know that it is > >a "less" problem, nor that their less settings are relevant. They only > >see that Git is broken out of the box. > > Maybe, instead of doing all the elaborate guess and assumption work, > have configure script check if the current PAGER supports colors and > build git accordingly? > configure could run the pager as one of its tests, and determine if > "ESC" appears on the output. But this is not a build-time problem, but rather a run-time problem. We do not know what the environment of the user will be at run-time. The safest thing would be to turn off auto-color when LESS (or any of the pager environment variables) is set at all (and not worry about whether "R" is set; only know that _we_ are not setting it, so we cannot count on it). But that would potentially inconvenience a lot of people whose default color would suddenly go away. -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-02-04 22:48 ` Jeff King @ 2014-02-04 22:54 ` Junio C Hamano 2014-02-04 23:00 ` Yuri 1 sibling, 0 replies; 39+ messages in thread From: Junio C Hamano @ 2014-02-04 22:54 UTC (permalink / raw) To: Jeff King; +Cc: Yuri, git, Jonathan Nieder Jeff King <peff@peff.net> writes: > The safest thing would be to turn off auto-color when LESS (or any of > the pager environment variables) is set at all (and not worry about > whether "R" is set; only know that _we_ are not setting it, so we cannot > count on it). But that would potentially inconvenience a lot of people > whose default color would suddenly go away. That is just as safe as disabling color for everybody, isn't it? Half of existing users have LESS with R, and the other half do not have LESS at all. The former will be harmed, the latter will not see any difference. Oh, and then new users who do not know R for LESS will not even notice that Git could support coloured output. Those among them who read manpages and find --color option will then see ESC[33m in their output and we are back to where we started X-<. So I think we are already at the safest place. Those who see ESC[33m will know they are missing some good stuff and can ask around, which is better than doing anything else at this point. I think that is the same conclusion as yours, "there is nothing to be done." ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-02-04 22:48 ` Jeff King 2014-02-04 22:54 ` Junio C Hamano @ 2014-02-04 23:00 ` Yuri 1 sibling, 0 replies; 39+ messages in thread From: Yuri @ 2014-02-04 23:00 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, Jonathan Nieder On 02/04/2014 14:48, Jeff King wrote: > But this is not a build-time problem, but rather a run-time problem. We > do not know what the environment of the user will be at run-time. Ok, git can test the pager on the given system before its first use and remember the result in ~/.git for later use for example. Such 'experimental' approach is maybe more reliable. Yuri ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/3] setup_pager: set MORE=R 2014-02-04 22:12 ` Jeff King 2014-02-04 22:17 ` Junio C Hamano @ 2014-02-05 2:11 ` Kyle J. McKay 1 sibling, 0 replies; 39+ messages in thread From: Kyle J. McKay @ 2014-02-05 2:11 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, Jonathan Nieder, Yuri On Feb 4, 2014, at 14:12, Jeff King wrote: > On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote: > >>> does complicate the point of my series, which was to add more >>> intimate >>> logic about how we handle LESS. >>> ... >>> return !x || strchr(x, 'R'); >> [...] >> >> I am not sure if it is even a good idea for us to have so intimate >> logic for various pagers in the first place. I'd seriously wonder >> if it is better to take this position: >> >> A platform packager who sets the default pager and/or the >> default environment for the pager at the build time, or an >> individual user who tells your Git what pager you want to >> use and/or with what setting that pager should be run under >> with environment variables. These people ought to know far >> better than Git what their specific choices do. Do not try >> to second-guess them. > > Sorry for letting this topic lapse; I wanted to look at some possible > Makefile improvements, which I'll be posting in a moment. I did want > to > address this point, though. > > If we are just talking about packagers and defaults (e.g., setting > MORE=R on FreeBSD), then I agree that the right tool for the job is > empowering the packagers, and the Makefile knob we've discussed does > that. > [snip] > > It seems a shame to me that we cannot do better for such users. > However, the level of intimacy with the pager is getting to be a bit > too > much for my taste, and the saving grace is that not that many people > set > LESS themselves (and git is widely enough known that "R" is a common > recommendation when people do). So it's probably the lesser of two > evils > to ignore the situation, and let people who run into it find the > answers > on the web. > > So I think there is nothing to be done. But I did want to mention > it in > case somebody else can come up with some clever solution. :) I think we can do better without adding intimate pager knowledge. And at the same time make it a simple matter of tweaking the Makefile to deal with new pagers on specific systems. There are two parts to the proposal. Part 1 Add a new configuration item which I will call "pager.env" for now that can have multiple values of the form ENV=value (whether multiple lines or whitespace separated on a single line to be decided later). On a system where more can support color by setting MORE=-R it might look like this: [pager] env = LESS=-FRSX LV=-c MORE=-R On a system where more does not it might look like this: [pager] env = LESS=-FRSX LV=-c The default value of pager.env is to be configured in a system- specific way (i.e. Makefile knob) at build time. Then, if Git is going to output color AND start a pager (that logic remains unchanged by this proposal), then it processes the pager.env value by examining each ENV=value setting and if the environment variable ENV is NOT already set, then sets it to value before starting the pager. This is mostly a clean up and shouldn't really be controversial since before commit e54c1f2d2 the system basically behaved like this where pager.env was always set to "LESS=FRSX" and after it behaves as though pager.env is always set to "LESS=FRSX LV=-c". Part 2 Alongside the current false/never, always, true/auto settings for "color.ui" a new "carefully" setting is introduced and the color.ui default if not set is changed from auto (commit 4c7f1819) to the new "carefully" setting. Why a new setting? So that people who have explicitly set color.ui to auto (or one of the other values) will not be affected by the proposed new logic. Another new configuration item which I will call "pager.carefully" for now is introduced that again can have multiple values of the form name=ENV. It might look like this: [pager] carefully = less=LESS LV=lv more=MORE Again the default value of pager.carefully can be configured in a system-specific way (i.e. Makefile knob) at build time. If color.ui is set to "carefully", then the logic is as follows: a) Decide whether to output color the same way as for color.ui=auto b) Select the pager the same way as for color.ui=auto c) If (a) and (b) have selected a pager AND enabled color output then check to see if the selected pager appears in pager.carefully as one of the name values (perhaps a suffix match on the first whitespace- separated word of the selected pager value). d) If the selected pager does not appear in pager.carefully, disable color output. e) If the selected pager appears in pager.carefully, but the ENV associated with it does NOT appear in pager.env, disable color output. f) If the pager appears in pager.carefully, but the ENV associated with it is already set, disable color output. g) Otherwise, go ahead with color output as the change in Part 1 above will properly set the pager's options to enable it. This has the following advantages: 1. No intimate pager knowledge. 2. Color output will be selected on supported systems by default assuming the user has not set any pager-specific environment variables. 3. This should prevent the user from ever seeing the ugly ESC[33 and ESC[m sequences with the default settings. 4. A user who has color.ui=auto set is not affected by this change. 5. Support for additional color pagers is easily added by tweaking the Makefile. It has the following disadvantage: 1. A user who has a pager-specific environment variable set for the default pager will not get color by default even if color would be supported without first setting color.ui=auto or using an equivalent. Essentially they will get the pre-1.8.4 behavior. --Kyle ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 3/3] pager: disable colors for some known-bad configurations 2014-01-17 4:14 ` [PATCH 0/3] improved out-of-the-box color settings Jeff King 2014-01-17 4:21 ` [PATCH 1/3] setup_pager: refactor LESS/LV environment setting Jeff King 2014-01-17 4:21 ` [PATCH 2/3] setup_pager: set MORE=R Jeff King @ 2014-01-17 4:24 ` Jeff King 2014-01-17 9:13 ` [PATCH 0/3] improved out-of-the-box color settings Yuri 3 siblings, 0 replies; 39+ messages in thread From: Jeff King @ 2014-01-17 4:24 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, Yuri Some pagers do not like the ANSI colors we print. It used to be that one had to opt into the colors, and you would usually check your pager config at the same time. These days, we turn on color.ui by default, meaning that some users may see broken "git log" from the start, before they have configured anything. They can fix it by turning off "color.pager", but we should try to make things work out-of-the-box as much as possible. The common cause of this is that the user is using "less" or "more", has setup its config variable in the environment (so that we do not override it), but has not included "R" in their config. For other pagers, we simply continue to guess that the pager can handle it. This is compatible with the current behavior (and will keep exotic things like "diff-highlight | less" working without further config). The downside is that other random pagers may break. Signed-off-by: Jeff King <peff@peff.net> --- cache.h | 3 ++- color.c | 2 +- config.c | 2 +- environment.c | 2 +- pager.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 4 deletions(-) diff --git a/cache.h b/cache.h index 83a2726..7fd1977 100644 --- a/cache.h +++ b/cache.h @@ -1215,7 +1215,8 @@ static inline ssize_t write_str_in_full(int fd, const char *str) extern void setup_pager(void); extern const char *pager_program; extern int pager_in_use(void); -extern int pager_use_color; +extern int pager_use_color_config; +extern int pager_use_color(void); extern int term_columns(void); extern int decimal_width(int); extern int check_pager_config(const char *cmd); diff --git a/color.c b/color.c index f672885..ffbff40 100644 --- a/color.c +++ b/color.c @@ -184,7 +184,7 @@ static int check_auto_color(void) { if (color_stdout_is_tty < 0) color_stdout_is_tty = isatty(1); - if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) { + if (color_stdout_is_tty || (pager_in_use() && pager_use_color())) { char *term = getenv("TERM"); if (term && strcmp(term, "dumb")) return 1; diff --git a/config.c b/config.c index d969a5a..2a8236b 100644 --- a/config.c +++ b/config.c @@ -991,7 +991,7 @@ int git_default_config(const char *var, const char *value, void *dummy) return git_default_advice_config(var, value); if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) { - pager_use_color = git_config_bool(var,value); + pager_use_color_config = git_config_bool(var,value); return 0; } diff --git a/environment.c b/environment.c index 3c76905..5cd642f 100644 --- a/environment.c +++ b/environment.c @@ -39,7 +39,7 @@ size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; size_t delta_base_cache_limit = 16 * 1024 * 1024; unsigned long big_file_threshold = 512 * 1024 * 1024; const char *pager_program; -int pager_use_color = 1; +int pager_use_color_config = -1; const char *editor_program; const char *askpass_program; const char *excludes_file; diff --git a/pager.c b/pager.c index 2303164..63e9252 100644 --- a/pager.c +++ b/pager.c @@ -184,3 +184,48 @@ int check_pager_config(const char *cmd) pager_program = c.value; return c.want; } + +static int pager_can_handle_color(void) +{ + const char *pager = git_pager(1); + + /* + * If it's less, we automatically set "R" and can handle color, + * unless the user already has a "LESS" variable that does not + * include "R". + */ + if (!strcmp(pager, "less")) { + const char *x = getenv("LESS"); + return !x || !!strchr(x, 'R'); + } + + if (!strcmp(pager, "more")) { +#ifdef PAGER_MORE_UNDERSTANDS_R + /* + * An advanced "more" that knows "R" is in the same boat as + * "less". + */ + const char *x = getenv("MORE"); + return !x || !!strchr(x, 'R'); +#else + /* + * For a more primitive "more", just assume that it will pass + * through the control codes verbatim. + */ + return 1; +#endif + } + + /* + * Otherwise, we don't recognize it. Guess that it can probably handle + * color. This matches what we have done historically. + */ + return 1; +} + +int pager_use_color(void) +{ + if (pager_use_color_config < 0) + pager_use_color_config = pager_can_handle_color(); + return pager_use_color_config; +} -- 1.8.5.2.500.g8060133 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 0/3] improved out-of-the-box color settings 2014-01-17 4:14 ` [PATCH 0/3] improved out-of-the-box color settings Jeff King ` (2 preceding siblings ...) 2014-01-17 4:24 ` [PATCH 3/3] pager: disable colors for some known-bad configurations Jeff King @ 2014-01-17 9:13 ` Yuri 3 siblings, 0 replies; 39+ messages in thread From: Yuri @ 2014-01-17 9:13 UTC (permalink / raw) To: Jeff King, git; +Cc: Jonathan Nieder On 01/16/2014 20:14, Jeff King wrote: > The second patch turns on "MORE=R" only for FreeBSD. Yuri, if you can > confirm that my patch works for you, that would be excellent. And if you > (or anyone) has access to NetBSD or OpenBSD to test the second patch on, > I'd welcome updates to config.mak.uname for those systems. I applied 3 patches, and it fixed the issue for me. Thank you for such a fast response! This is very impressive! I saw this issue for a long while, and only last night took time to report it. Unfortunately, I don't have any other BSDs sitting around. The easiest way to try this is to install them in VM. Yuri ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 1:47 ` Jeff King 2014-01-17 2:02 ` Yuri @ 2014-01-17 20:15 ` Yuri 2014-02-05 1:24 ` Yuri 2 siblings, 0 replies; 39+ messages in thread From: Yuri @ 2014-01-17 20:15 UTC (permalink / raw) To: Jeff King; +Cc: git One other idea to handle this is at configuration phase. You can test more and less with every option used on every platform for each of them respectively. Test could run the command with the option, and check if it passes the given escape sequence. This would be reflected in config.h defines like this: MORE_DASH_R_WORKS This would be very accurate. Not sure if this is an overkill for this issue. Yuri ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-01-17 1:47 ` Jeff King 2014-01-17 2:02 ` Yuri 2014-01-17 20:15 ` 'git log' escape symbols shown as ESC[33 and ESC[m Yuri @ 2014-02-05 1:24 ` Yuri 2014-02-05 1:33 ` Jeff King 2 siblings, 1 reply; 39+ messages in thread From: Yuri @ 2014-02-05 1:24 UTC (permalink / raw) To: git; +Cc: Jeff King On 01/16/2014 17:47, Jeff King wrote: > Are you using "less" as your pager (it is the default in git unless you > have set your PAGER environment variable)? If so, do you have the "R" > option set to pass through ANSI codes? Git will set this automatically > in your "LESS" variable if you do not already have such a variable (but > it will not touch it if you already have it set, and are missing "R"). I think the 'experimental' approach is simpler and better. When the git command requiring pager is first run, git would run the pager with some simple one line escape sequence, and see if sequence is preserved. If it was preserved, git should just run with escape sequences. If the pager destroyed the sequence, git should issue a warning to the user: git: your default pager PAGER=more doesn't pass escape sequences, and they will be disabled them. You can revert this decision by changing the file ~/.git/pager.conf This way: * damaged sequences will not show up by default * colors will be displayed by default when possible * user would be informed, and will have a clear choice * this is easy to implement, and no elaborate and obscure reasoning should be employed in the implementation For me, if git would have told me that my pager doesn't support escape sequences, I could have taken it from there. Yuri ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: 'git log' escape symbols shown as ESC[33 and ESC[m 2014-02-05 1:24 ` Yuri @ 2014-02-05 1:33 ` Jeff King 0 siblings, 0 replies; 39+ messages in thread From: Jeff King @ 2014-02-05 1:33 UTC (permalink / raw) To: Yuri; +Cc: git On Tue, Feb 04, 2014 at 05:24:55PM -0800, Yuri wrote: > I think the 'experimental' approach is simpler and better. > When the git command requiring pager is first run, git would run the > pager with some simple one line escape sequence, and see if sequence > is preserved. See how? If less's stdout is not connected to a terminal, it simply passes through the output as-is. E.g., try: foo() { # blue foo printf '\x1b[34mfoo\x1b[m' } unset LESS foo | less which has the problematic output. Now try: foo | less | cat which passes through the ANSI codes unscathed. I have no idea how other pagers behave. So I think this is going back down the road of hard-coding lots of pager-specific behaviors. -Peff ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2014-02-05 2:11 UTC | newest] Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-01-17 0:34 'git log' escape symbols shown as ESC[33 and ESC[m Yuri 2014-01-17 1:47 ` Jeff King 2014-01-17 2:02 ` Yuri 2014-01-17 2:13 ` Jeff King 2014-01-17 2:28 ` Yuri 2014-01-17 2:32 ` Jeff King 2014-01-17 2:46 ` Yuri 2014-01-17 2:29 ` Jonathan Nieder 2014-01-17 2:35 ` Jeff King 2014-01-17 3:21 ` Jeff King 2014-01-17 4:14 ` [PATCH 0/3] improved out-of-the-box color settings Jeff King 2014-01-17 4:21 ` [PATCH 1/3] setup_pager: refactor LESS/LV environment setting Jeff King 2014-01-17 4:21 ` [PATCH 2/3] setup_pager: set MORE=R Jeff King 2014-01-17 7:26 ` Kyle J. McKay 2014-01-17 19:11 ` Junio C Hamano 2014-01-21 5:30 ` Jeff King 2014-01-21 8:42 ` Kyle J. McKay 2014-01-23 2:14 ` Jeff King 2014-01-23 17:22 ` Junio C Hamano 2014-01-17 19:17 ` Junio C Hamano 2014-01-17 19:57 ` Junio C Hamano 2014-01-17 23:42 ` Junio C Hamano 2014-01-21 6:13 ` Jeff King 2014-01-21 19:28 ` Junio C Hamano 2014-01-21 5:49 ` Jeff King 2014-01-21 19:23 ` Junio C Hamano 2014-02-04 22:12 ` Jeff King 2014-02-04 22:17 ` Junio C Hamano 2014-02-04 22:25 ` Jeff King 2014-02-04 22:45 ` Yuri 2014-02-04 22:48 ` Jeff King 2014-02-04 22:54 ` Junio C Hamano 2014-02-04 23:00 ` Yuri 2014-02-05 2:11 ` Kyle J. McKay 2014-01-17 4:24 ` [PATCH 3/3] pager: disable colors for some known-bad configurations Jeff King 2014-01-17 9:13 ` [PATCH 0/3] improved out-of-the-box color settings Yuri 2014-01-17 20:15 ` 'git log' escape symbols shown as ESC[33 and ESC[m Yuri 2014-02-05 1:24 ` Yuri 2014-02-05 1:33 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).