'git log' escape symbols shown as ESC[33 and ESC[m

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

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").

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.

No. See above.

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

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

Interesting. I take it that "more" does not pass through ANSI codes at all, then.

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.

---

index 83a2726..7fd1977 100644
@@ -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);

index f672885..ffbff40 100644
@@ -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;

index d969a5a..2a8236b 100644
@@ -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;
 	}
 

index 3c76905..5cd642f 100644
@@ -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;

index 0cc75a8..a816af3 100644
@@ -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;
+}

Hi,

[...]

[...]

Yeah, my "we know" here was still guessing. According to Yuri, FreeBSD is the system you are thinking of. :)

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

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

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

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.

---
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(-)

index 0cc75a8..90d237e 100644
@@ -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

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).

---
 Makefile         | 7 +++++++
 config.mak.uname | 1 +
 pager.c          | 4 ++++
 3 files changed, 12 insertions(+)

index b4af1e2..6cd7a2a 100644
@@ -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))

index 7d31fad..d63babc 100644
@@ -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

index 90d237e..2303164 100644
@@ -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

[snip]

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.

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(-)

index b4af1e2..c9e7847 100644
@@ -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

index 7d31fad..8aea8a6 100644
@@ -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

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(-)

index b4af1e2..690f4c6 100644
@@ -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' \
     [email protected] >$@+
 endef
 

index 7d31fad..8aea8a6 100644
@@ -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

index fffa3c7..8fc1bbd 100644
@@ -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" '"$@"'
 }
 

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.

---
 cache.h       |  3 ++-
 color.c       |  2 +-
 config.c      |  2 +-
 environment.c |  2 +-
 pager.c       | 45 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 50 insertions(+), 4 deletions(-)

index 83a2726..7fd1977 100644
@@ -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);

index f672885..ffbff40 100644
@@ -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;

index d969a5a..2a8236b 100644
@@ -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;
 	}
 

index 3c76905..5cd642f 100644
@@ -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;

1.8.5.2.500.g8060133

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

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

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.

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.

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

Build-time option would be perfect.
#if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) I think they all use 'more' derived from the same root.

Orthogonal, I know. Conveying more info in aesthetic way is always good, without making it look like a rainbow.

Yuri

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

I do not have any problem with that, but would very much prefer it as a separate patch, in case there are any fallouts.

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...

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:

index b4af1e2..3a8d15e 100644
@@ -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".

index e69de29..5962440 100644
@@ -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

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.

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

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.

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.

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.

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

Yeah, I think we are in agreement on that point.

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

[snip]

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

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.

Sure.

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

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

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

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

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."

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

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