Re: 'git log' escape symbols shown as ESC[33 and ESC[m
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
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: 0x 1b5b 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
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;
+}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
