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

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

[Yuri]

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

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

[Jeff King]

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

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

[Yuri]

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

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

[Jeff King]

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;
+}

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

[Yuri]

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

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

[Jeff King]

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

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

[Yuri]

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

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

[Jonathan Nieder]

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

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

[Jeff King]

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

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

[Jeff King]

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

[PATCH 0/3] improved out-of-the-box color settings

[Jeff King]

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

[PATCH 1/3] setup_pager: refactor LESS/LV environment setting

[Jeff King]

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

[PATCH 2/3] setup_pager: set MORE=R

[Jeff King]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Kyle J. McKay]

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.

Re: [PATCH 2/3] setup_pager: set MORE=R

[Junio C Hamano]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Jeff King]

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?

Re: [PATCH 2/3] setup_pager: set MORE=R

[Kyle J. McKay]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Jeff King]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Junio C Hamano]

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.

Re: [PATCH 2/3] setup_pager: set MORE=R

[Junio C Hamano]

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.

Re: [PATCH 2/3] setup_pager: set MORE=R

[Junio C Hamano]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Junio C Hamano]

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;
 

Re: [PATCH 2/3] setup_pager: set MORE=R

[Jeff King]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Junio C Hamano]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Jeff King]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Junio C Hamano]

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.

Re: [PATCH 2/3] setup_pager: set MORE=R

[Jeff King]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Junio C Hamano]

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.

Re: [PATCH 2/3] setup_pager: set MORE=R

[Jeff King]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Yuri]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Jeff King]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Junio C Hamano]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Yuri]

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

Re: [PATCH 2/3] setup_pager: set MORE=R

[Kyle J. McKay]

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

[PATCH 3/3] pager: disable colors for some known-bad configurations

[Jeff King]

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

Re: [PATCH 0/3] improved out-of-the-box color settings

[Yuri]

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

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

[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

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

[Yuri]

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

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

[Jeff King]

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