{"thread":{"id":"35676","subject":"'git log' escape symbols shown as ESC[33 and ESC[m","startedAt":"2014-01-17T00:34:01Z","lastAt":"2014-02-05T02:11:01Z","messageCount":39,"participants":["Yuri","Jeff King","Jonathan Nieder","Kyle J. McKay","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"233239","messageId":"52D87A79.6060600@rawbw.com","threadId":"35676","inReplyTo":null,"subject":"'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-01-17T00:34:01Z","receivedAt":"2014-01-17T00:34:01Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"When I run 'git log' on FreeBSD-9.2, I get output like this:\nESC[33mcommit 398e78c62fd507a317de7c2abb8e25c9fac7ac9eESC[m\nMerge: 5fb8f6e d2138ba\n...\n\nESC is white on black background.\n\nWhy ESC[33m aren't expanded by the terminal? Is this because git prints \nan unsupported sequence?\n\nHex of what git writes to terminal is here:\n0x0000 1b5b 3333 6d63 6f6d 6d69 7420 6636 6432 6136 3032 3965 6661 6439 \n6635 6334 3161 6261 |.[33mcommit f6d2a6029efad9f5c41aba|\n0x0022 3961 3830 6131 3032 3138 6332 6333 3465 6662 1b5b 6d0a 4d65 7267 \n653a 2033 3938 6537 |9a80a10218c2c34efb.[m.Merge: 398e7|\n\nI think it tries to print the line in yellow (color code 33), and prints \nthe wrong sequence. The correct sequence would be:\n\\033[1;33mString Goes Here\\033[0m\nIt misses \"1;\" in the beginning, and \"0\" in the end, this is why the \nsequence is not interpreted.\n\nWhy does it print a wrong sequence? Is this because this is some kind of \nlinuxism that doesn't work on FreeBSD maybe?\nAlso, there are the termcap functions that allow to determine what does \nthe actual terminal supports. You should first check for cap bits \ncorresponding to the features you expect, if you expect something uncommon.\n\nYuri\n"},{"id":"233247","messageId":"20140117014758.GF7249@sigill.intra.peff.net","threadId":"35676","inReplyTo":"52D87A79.6060600@rawbw.com","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T01:47:58Z","receivedAt":"2014-01-17T01:47:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2014 at 04:34:01PM -0800, Yuri wrote:\n\n> When I run 'git log' on FreeBSD-9.2, I get output like this:\n> ESC[33mcommit 398e78c62fd507a317de7c2abb8e25c9fac7ac9eESC[m\n> Merge: 5fb8f6e d2138ba\n> ...\n> \n> ESC is white on black background.\n> \n> Why ESC[33m aren't expanded by the terminal? Is this because git\n> prints an unsupported sequence?\n\nAre you using \"less\" as your pager (it is the default in git unless you\nhave set your PAGER environment variable)? If so, do you have the \"R\"\noption set to pass through ANSI codes? Git will set this automatically\nin your \"LESS\" variable if you do not already have such a variable (but\nit will not touch it if you already have it set, and are missing \"R\").\n\n> Hex of what git writes to terminal is here:\n> 0x0000 1b5b 3333 6d63 6f6d 6d69 7420 6636 6432 6136 3032 3965 6661\n> 6439 6635 6334 3161 6261 |.[33mcommit f6d2a6029efad9f5c41aba|\n> 0x0022 3961 3830 6131 3032 3138 6332 6333 3465 6662 1b5b 6d0a 4d65\n> 7267 653a 2033 3938 6537 |9a80a10218c2c34efb.[m.Merge: 398e7|\n> \n> I think it tries to print the line in yellow (color code 33), and\n> prints the wrong sequence. The correct sequence would be:\n> \\033[1;33mString Goes Here\\033[0m\n> It misses \"1;\" in the beginning, and \"0\" in the end, this is why the\n> sequence is not interpreted.\n\nNo, the \"\\033[33m\" is correct. The \"1;\" in your string is turning on the\nbold attribute, and we are not trying to do that. The \"0\" in the reset\nis optional (at least according to [1]; I do not have an actual standard\nto reference).\n\nBut I do not think that is your problem anyway; the \"ESC\" you are seeing\nis almost certainly generated by \"less\" escaping the \\033.\n\n> Why does it print a wrong sequence? Is this because this is some kind\n> of linuxism that doesn't work on FreeBSD maybe?\n\nNo. See above.\n\n> Also, there are the termcap functions that allow to determine what\n> does the actual terminal supports. You should first check for cap\n> bits corresponding to the features you expect, if you expect\n> something uncommon.\n\nAlmost every terminal supports ANSI colors. The notable exceptions is\nthe Windows console, but we handle the translation there. If you have a\nterminal that actually doesn't support ANSI colors, please let us know\nand we can see what is going on. But I'm reasonably sure that you are\njust running up against the escaping in \"less\" here.\n\n-Peff\n"},{"id":"233248","messageId":"52D88F30.4000807@rawbw.com","threadId":"35676","inReplyTo":"20140117014758.GF7249@sigill.intra.peff.net","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-01-17T02:02:24Z","receivedAt":"2014-01-17T02:02:24Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On 01/16/2014 17:47, Jeff King wrote:\n> Are you using \"less\" as your pager (it is the default in git unless you\n> have set your PAGER environment variable)? If so, do you have the \"R\"\n> option set to pass through ANSI codes? Git will set this automatically\n> in your \"LESS\" variable if you do not already have such a variable (but\n> it will not touch it if you already have it set, and are missing \"R\").\n\nMy PAGER variable was set to \"more\". PAGER=more is also a default for a \nnewly created user in FreeBSD.\n\nSo what would be the correct fix here in general, so that git will work \nfine for a new unchanged user?\n\nYuri\n"},{"id":"233249","messageId":"20140117021320.GA12444@sigill.intra.peff.net","threadId":"35676","inReplyTo":"52D88F30.4000807@rawbw.com","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T02:13:20Z","receivedAt":"2014-01-17T02:13:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2014 at 06:02:24PM -0800, Yuri wrote:\n\n> On 01/16/2014 17:47, Jeff King wrote:\n> >Are you using \"less\" as your pager (it is the default in git unless you\n> >have set your PAGER environment variable)? If so, do you have the \"R\"\n> >option set to pass through ANSI codes? Git will set this automatically\n> >in your \"LESS\" variable if you do not already have such a variable (but\n> >it will not touch it if you already have it set, and are missing \"R\").\n> \n> My PAGER variable was set to \"more\". PAGER=more is also a default for\n> a newly created user in FreeBSD.\n\nInteresting. I take it that \"more\" does not pass through ANSI codes at\nall, then.\n\n> So what would be the correct fix here in general, so that git will\n> work fine for a new unchanged user?\n\nThis was a non-issue until 4c7f181 (make color.ui default to 'auto',\n2013-06-10), which was released in git v1.8.4, as people had to\nexplicitly turn color on. I'm cc-ing Matthieu, who authored that commit.\n\nYou can:\n\n git config color.ui false\n\nto turn off color completely. But in this case, I think it is more\nexact to tell git simply that your pager does not handle color:\n\n git config color.pager false\n\nObviously that does not help the \"new unchanged user\". I think we can\nbe smarter about guessing whether the pager can actually handle color,\nbased on the name. Is it possible for you to compile git with the patch\nbelow? It should make your problem go away without having to configure\nanything. It can't cover every possible pager, but I think it should\ncatch the common ones.\n\n---\ndiff --git a/cache.h b/cache.h\nindex 83a2726..7fd1977 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1215,7 +1215,8 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n extern void setup_pager(void);\n extern const char *pager_program;\n extern int pager_in_use(void);\n-extern int pager_use_color;\n+extern int pager_use_color_config;\n+extern int pager_use_color(void);\n extern int term_columns(void);\n extern int decimal_width(int);\n extern int check_pager_config(const char *cmd);\ndiff --git a/color.c b/color.c\nindex f672885..ffbff40 100644\n--- a/color.c\n+++ b/color.c\n@@ -184,7 +184,7 @@ static int check_auto_color(void)\n {\n \tif (color_stdout_is_tty < 0)\n \t\tcolor_stdout_is_tty = isatty(1);\n-\tif (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {\n+\tif (color_stdout_is_tty || (pager_in_use() && pager_use_color())) {\n \t\tchar *term = getenv(\"TERM\");\n \t\tif (term && strcmp(term, \"dumb\"))\n \t\t\treturn 1;\ndiff --git a/config.c b/config.c\nindex d969a5a..2a8236b 100644\n--- a/config.c\n+++ b/config.c\n@@ -991,7 +991,7 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \t\treturn git_default_advice_config(var, value);\n \n \tif (!strcmp(var, \"pager.color\") || !strcmp(var, \"color.pager\")) {\n-\t\tpager_use_color = git_config_bool(var,value);\n+\t\tpager_use_color_config = git_config_bool(var,value);\n \t\treturn 0;\n \t}\n \ndiff --git a/environment.c b/environment.c\nindex 3c76905..5cd642f 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -39,7 +39,7 @@ size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n size_t delta_base_cache_limit = 16 * 1024 * 1024;\n unsigned long big_file_threshold = 512 * 1024 * 1024;\n const char *pager_program;\n-int pager_use_color = 1;\n+int pager_use_color_config = -1;\n const char *editor_program;\n const char *askpass_program;\n const char *excludes_file;\ndiff --git a/pager.c b/pager.c\nindex 0cc75a8..a816af3 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -182,3 +182,38 @@ int check_pager_config(const char *cmd)\n \t\tpager_program = c.value;\n \treturn c.want;\n }\n+\n+static int pager_can_handle_color(void)\n+{\n+\tconst char *pager = git_pager(1);\n+\n+\t/*\n+\t * If it's less, we automatically set \"R\" and can handle color,\n+\t * unless the user already has a \"LESS\" variable that does not\n+\t * include \"R\".\n+\t */\n+\tif (!strcmp(pager, \"less\")) {\n+\t\tconst char *x = getenv(\"LESS\");\n+\t\treturn !x || !!strchr(x, 'R');\n+\t}\n+\n+\t/*\n+\t * We know that \"more\" does not pass through colors at all.\n+\t */\n+\tif (!strcmp(pager, \"more\"))\n+\t\treturn 0;\n+\n+\t/*\n+\t * Otherwise, we don't recognize it. Guess that it can probably handle\n+\t * color. This matches historical behavior, though it is probably not\n+\t * the safest default.\n+\t */\n+\treturn 1;\n+}\n+\n+int pager_use_color(void)\n+{\n+\tif (pager_use_color_config < 0)\n+\t\tpager_use_color_config = pager_can_handle_color();\n+\treturn pager_use_color_config;\n+}\n"},{"id":"233252","messageId":"52D8953A.8010307@rawbw.com","threadId":"35676","inReplyTo":"20140117021320.GA12444@sigill.intra.peff.net","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-01-17T02:28:10Z","receivedAt":"2014-01-17T02:28:10Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On 01/16/2014 18:13, Jeff King wrote:\n> Interesting. I take it that \"more\" does not pass through ANSI codes at\n> all, then.\n>\n\n\nActually, 'more -R' also passes colors on FreeBSD. So maybe you can \nalways add -R if PAGER=more on *BSD (any of them) ? This will fix this \nissue.\n\nI know, it is unpleasant when you add some new minor feature (like term \ncolors), and people begin to complain about some related issues. But \nturning colors off isn't a good approach also. App just needs to be \nsmarter about when and how to use them.\n\nI would go even further, and convey even more information with colors. \nFor example, make all dates which are less than 24 hours red.\n\nYuri\n"},{"id":"233253","messageId":"20140117022921.GU18964@google.com","threadId":"35676","inReplyTo":"20140117021320.GA12444@sigill.intra.peff.net","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-17T02:29:21Z","receivedAt":"2014-01-17T02:29:21Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> Obviously that does not help the \"new unchanged user\". I think we can\n> be smarter about guessing whether the pager can actually handle color,\n> based on the name.\n[...]\n> +++ b/pager.c\n> @@ -182,3 +182,38 @@ int check_pager_config(const char *cmd)\n[...]\n> +\t/*\n> +\t * We know that \"more\" does not pass through colors at all.\n> +\t */\n> +\tif (!strcmp(pager, \"more\"))\n> +\t\treturn 0;\n\nI seem to remember that on some systems \"more\" is the name of the\nfull-featured pager that knows how to scroll forward and backward and\nhandle color. (My memory could be faulty. A search in the makefile\nfor DEFAULT_PAGER=more only finds AIX, which is not the platform I was\nthinking of.)\n\nOn a stock Debian system \"more\" is especially primitive, which means\nthat it passes colors through, too. It being so primitive also means\nit is not a particularly good choice for the PAGER setting, though,\nso probably that's not too important.\n\nHope that helps,\nJonathan\n"},{"id":"233254","messageId":"20140117023205.GA13023@sigill.intra.peff.net","threadId":"35676","inReplyTo":"52D8953A.8010307@rawbw.com","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T02:32:05Z","receivedAt":"2014-01-17T02:32:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2014 at 06:28:10PM -0800, Yuri wrote:\n\n> On 01/16/2014 18:13, Jeff King wrote:\n> >Interesting. I take it that \"more\" does not pass through ANSI codes at\n> >all, then.\n> \n> Actually, 'more -R' also passes colors on FreeBSD. So maybe you can\n> always add -R if PAGER=more on *BSD (any of them) ? This will fix\n> this issue.\n\nAh, if \"more\" can handle the colors, then that is preferable.\n\nI do think it would have to be system-specific, though. Unlike \"less\",\nthere are many implementations of \"more\", and quite a lot of them will\nprobably not support colors. We can make it a build-time option, and\nset it to \"on\" for FreeBSD.\n\n> I know, it is unpleasant when you add some new minor feature (like\n> term colors), and people begin to complain about some related issues.\n> But turning colors off isn't a good approach also. App just needs to\n> be smarter about when and how to use them.\n\nAgreed. It is simply that most people seem to either use \"less\", or not\nset their \"PAGER\" at all. I think FreeBSD is the exception here.\n\n> I would go even further, and convey even more information with\n> colors. For example, make all dates which are less than 24 hours red.\n\nThat's an orthogonal issue. I'm not sure I agree, but if you are\ninterested, feel free to prepare a patch, which will get some discussion\ngoing.\n\n-Peff\n"},{"id":"233255","messageId":"20140117023548.GB13023@sigill.intra.peff.net","threadId":"35676","inReplyTo":"20140117022921.GU18964@google.com","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T02:35:48Z","receivedAt":"2014-01-17T02:35:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2014 at 06:29:21PM -0800, Jonathan Nieder wrote:\n\n> > +\t/*\n> > +\t * We know that \"more\" does not pass through colors at all.\n> > +\t */\n> > +\tif (!strcmp(pager, \"more\"))\n> > +\t\treturn 0;\n> \n> I seem to remember that on some systems \"more\" is the name of the\n> full-featured pager that knows how to scroll forward and backward and\n> handle color. (My memory could be faulty. A search in the makefile\n> for DEFAULT_PAGER=more only finds AIX, which is not the platform I was\n> thinking of.)\n\nYeah, my \"we know\" here was still guessing. According to Yuri, FreeBSD\nis the system you are thinking of. :)\n\n> On a stock Debian system \"more\" is especially primitive, which means\n> that it passes colors through, too. It being so primitive also means\n> it is not a particularly good choice for the PAGER setting, though,\n> so probably that's not too important.\n\nYeah, colors do get passed through on Debian. It's possible that other\nprimitive \"more\" implementations are OK, too (and certainly we have\ndefaulted to \"on\" for them until now).\n\nI think we should make an effort to set MORE=R on FreeBSD. We can\nperhaps just set it unconditionally, and assume that primitive \"more\"\nwill ignore it. And then assume that \"more\" will handle colors (either\nbecause of the R setting, or because it is too dumb to escape it).\n\nI can prepare a patch series, but I happily no longer have any antique\nsystems to test this kind of stuff on.\n\n-Peff\n"},{"id":"233257","messageId":"52D89969.3040802@rawbw.com","threadId":"35676","inReplyTo":"20140117023205.GA13023@sigill.intra.peff.net","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-01-17T02:46:01Z","receivedAt":"2014-01-17T02:46:01Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On 01/16/2014 18:32, Jeff King wrote:\n> Ah, if \"more\" can handle the colors, then that is preferable.\n>\n> I do think it would have to be system-specific, though. Unlike \"less\",\n> there are many implementations of \"more\", and quite a lot of them will\n> probably not support colors. We can make it a build-time option, and\n> set it to \"on\" for FreeBSD.\n\nBuild-time option would be perfect.\n#if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__)\nI think they all use 'more' derived from the same root.\n\n> I would go even further, and convey even more information with\n> colors. For example, make all dates which are less than 24 hours red.\n> That's an orthogonal issue. I'm not sure I agree, but if you are\n> interested, feel free to prepare a patch, which will get some discussion\n> going.\n\nOrthogonal, I know. Conveying more info in aesthetic way is always good, \nwithout making it look like a rainbow.\n\nYuri\n"},{"id":"233259","messageId":"20140117032151.GA17311@sigill.intra.peff.net","threadId":"35676","inReplyTo":"20140117023548.GB13023@sigill.intra.peff.net","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T03:21:51Z","receivedAt":"2014-01-17T03:21:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2014 at 09:35:48PM -0500, Jeff King wrote:\n\n> I think we should make an effort to set MORE=R on FreeBSD. We can\n> perhaps just set it unconditionally, and assume that primitive \"more\"\n> will ignore it. And then assume that \"more\" will handle colors (either\n> because of the R setting, or because it is too dumb to escape it).\n> \n> I can prepare a patch series, but I happily no longer have any antique\n> systems to test this kind of stuff on.\n\nMeh. I figured I would have to go to an antique system to find breakage,\nbut it is easy to do it on Debian:\n\n $ MORE=R more\n more: unknown option -R\n\nSo we do need to make it conditional.\n\n-Peff\n"},{"id":"233260","messageId":"20140117041430.GB19551@sigill.intra.peff.net","threadId":"35676","inReplyTo":"20140117023548.GB13023@sigill.intra.peff.net","subject":"[PATCH 0/3] improved out-of-the-box color settings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T04:14:30Z","receivedAt":"2014-01-17T04:14:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2014 at 09:35:48PM -0500, Jeff King wrote:\n\n> I can prepare a patch series, but I happily no longer have any antique\n> systems to test this kind of stuff on.\n\nHere it is. For those just joining us, the problem is that on some\nsystems, \"git log\" out of the box produces ANSI codes which are munged\nby the pager and look terrible. I'm trying to improve that by\nauto-configuring \"more\" (to get color in more cases that can handle it),\nand by dynamically adjusting color.pager's default based on which pager\nis in use (to turn off color in cases we know won't work).\n\nI was able to test my assumptions about \"MORE\" on Debian, and adjusted\nthe patches accordingly from the previous discussion. I am still\nguessing that versions of \"more\" without \"R\" will pass through the ANSI\ncodes as-is. That works on Debian. If anybody has an older system to\ntest and report on, I'd love to hear it.\n\nThe second patch turns on \"MORE=R\" only for FreeBSD. Yuri, if you can\nconfirm that my patch works for you, that would be excellent. And if you\n(or anyone) has access to NetBSD or OpenBSD to test the second patch on,\nI'd welcome updates to config.mak.uname for those systems.\n\nWe also set \"LV=-c\" along with \"LESS\". I punted on detecting that in the\nthird patch, as I do not know the exact format of \"LV\". It looks like\njust checking for \"c\" might not be enough, and I need to actually\nunderstand the option format (i.e., \"-c\" or \"-dc\", but not \"-Ac\"). I'll\nleave that to somebody who cares more about LV to improve (with these\npatches, the behavior for them should remain the same, so it's at least\nnot making anything worse).\n\n [1/3]: setup_pager: refactor LESS/LV environment setting\n [2/3]: setup_pager: set MORE=R\n [3/3]: pager: disable colors for some known-bad configurations\n\n-Peff\n"},{"id":"233261","messageId":"20140117042101.GA23443@sigill.intra.peff.net","threadId":"35676","inReplyTo":"20140117041430.GB19551@sigill.intra.peff.net","subject":"[PATCH 1/3] setup_pager: refactor LESS/LV environment setting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T04:21:02Z","receivedAt":"2014-01-17T04:21:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The way the code is structured now, we have to repeat\nourselves in fetching the environment variables (which will\nget annoying as we add more). Instead, let's use an\nargv_array. That removes a lot of the extra conditionals\nand makes it easier to add new variables.\n\nIt does means we'll leak the memory for the array, but:\n\n 1. This function is only called once per program.\n\n 2. We're now leaking heap memory instead of wasting BSS on\n the static array.\n\nSigned-off-by: Jeff King \n---\nI actually think we can free pager_process.env after\nstart_command is run. You _cannot_ do so with argv, though,\nand I'd rather leak this tiny bit than have a hard-to-track\nassumption on memory lifetime buried here.\n\n pager.c | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 0cc75a8..90d237e 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n+#include \"argv-array.h\"\n \n #ifndef DEFAULT_PAGER\n #define DEFAULT_PAGER \"less\"\n@@ -63,6 +64,7 @@ const char *git_pager(int stdout_is_tty)\n void setup_pager(void)\n {\n \tconst char *pager = git_pager(isatty(1));\n+\tstruct argv_array env = ARGV_ARRAY_INIT;\n \n \tif (!pager || pager_in_use())\n \t\treturn;\n@@ -80,17 +82,13 @@ void setup_pager(void)\n \tpager_process.use_shell = 1;\n \tpager_process.argv = pager_argv;\n \tpager_process.in = -1;\n-\tif (!getenv(\"LESS\") || !getenv(\"LV\")) {\n-\t\tstatic const char *env[3];\n-\t\tint i = 0;\n-\n-\t\tif (!getenv(\"LESS\"))\n-\t\t\tenv[i++] = \"LESS=FRSX\";\n-\t\tif (!getenv(\"LV\"))\n-\t\t\tenv[i++] = \"LV=-c\";\n-\t\tenv[i] = NULL;\n-\t\tpager_process.env = env;\n-\t}\n+\n+\tif (!getenv(\"LESS\"))\n+\t\targv_array_push(&env, \"LESS=FRSX\");\n+\tif (!getenv(\"LV\"))\n+\t\targv_array_push(&env, \"LV=-c\");\n+\tpager_process.env = argv_array_detach(&env, NULL);\n+\n \tif (start_command(&pager_process))\n \t\treturn;\n \n-- \n1.8.5.2.500.g8060133\n"},{"id":"233263","messageId":"20140117042153.GB23443@sigill.intra.peff.net","threadId":"35676","inReplyTo":"20140117041430.GB19551@sigill.intra.peff.net","subject":"[PATCH 2/3] setup_pager: set MORE=R","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T04:21:53Z","receivedAt":"2014-01-17T04:21:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we run the pager, we always set \"LESS=R\" to tell the\npager to pass through ANSI colors. On modern versions of\nFreeBSD, the system \"more\" can do the same trick.\n\nUnlike \"less\", we may be dealing with various versions of\n\"more\" that do not understand the \"R\" option, but do know\nhow to read options from MORE (Debian's \"more\" is an\nexample). For this reason, we make the setting a\ncompile-time option (but turn it on by default on FreeBSD).\n\nSigned-off-by: Jeff King \n---\n Makefile | 7 +++++++\n config.mak.uname | 1 +\n pager.c | 4 ++++\n 3 files changed, 12 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex b4af1e2..6cd7a2a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -312,6 +312,9 @@ all::\n # you want to use something different. The value will be interpreted by the\n # shell at runtime when it is used.\n #\n+# Define PAGER_MORE_UNDERSTANDS_R if your system's \"more\" pager\n+# can pass-through ANSI colors using the \"R\" option.\n+#\n # Define DEFAULT_EDITOR to a sensible editor command (defaults to \"vi\") if you\n # want to use something different. The value will be interpreted by the shell\n # if necessary when it is used. Examples:\n@@ -1608,6 +1611,10 @@ DEFAULT_PAGER_CQ_SQ = $(subst ','\\'',$(DEFAULT_PAGER_CQ))\n BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'\n endif\n \n+ifdef PAGER_MORE_UNDERSTANDS_R\n+BASIC_CFLAGS += -DPAGER_MORE_UNDERSTANDS_R\n+endif\n+\n ifdef SHELL_PATH\n SHELL_PATH_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(SHELL_PATH)))\"\n SHELL_PATH_CQ_SQ = $(subst ','\\'',$(SHELL_PATH_CQ))\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 7d31fad..d63babc 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -188,6 +188,7 @@ ifeq ($(uname_S),FreeBSD)\n \tendif\n \tPYTHON_PATH = /usr/local/bin/python\n \tHAVE_PATHS_H = YesPlease\n+\tPAGER_MORE_UNDERSTANDS_R = YesPlease\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\ndiff --git a/pager.c b/pager.c\nindex 90d237e..2303164 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -87,6 +87,10 @@ void setup_pager(void)\n \t\targv_array_push(&env, \"LESS=FRSX\");\n \tif (!getenv(\"LV\"))\n \t\targv_array_push(&env, \"LV=-c\");\n+#ifdef PAGER_MORE_UNDERSTANDS_R\n+\tif (!getenv(\"MORE\"))\n+\t\targv_array_push(&env, \"MORE=R\");\n+#endif\n \tpager_process.env = argv_array_detach(&env, NULL);\n \n \tif (start_command(&pager_process))\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233264","messageId":"20140117042450.GC23443@sigill.intra.peff.net","threadId":"35676","inReplyTo":"20140117041430.GB19551@sigill.intra.peff.net","subject":"[PATCH 3/3] pager: disable colors for some known-bad configurations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T04:24:50Z","receivedAt":"2014-01-17T04:24:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some pagers do not like the ANSI colors we print. It used to\nbe that one had to opt into the colors, and you would\nusually check your pager config at the same time. These\ndays, we turn on color.ui by default, meaning that some\nusers may see broken \"git log\" from the start, before they\nhave configured anything. They can fix it by turning off\n\"color.pager\", but we should try to make things work\nout-of-the-box as much as possible.\n\nThe common cause of this is that the user is using \"less\" or\n\"more\", has setup its config variable in the environment (so\nthat we do not override it), but has not included \"R\" in\ntheir config.\n\nFor other pagers, we simply continue to guess that the pager\ncan handle it. This is compatible with the current behavior\n(and will keep exotic things like \"diff-highlight | less\"\nworking without further config). The downside is that other\nrandom pagers may break.\n\nSigned-off-by: Jeff King \n---\n cache.h | 3 ++-\n color.c | 2 +-\n config.c | 2 +-\n environment.c | 2 +-\n pager.c | 45 +++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 50 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 83a2726..7fd1977 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1215,7 +1215,8 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n extern void setup_pager(void);\n extern const char *pager_program;\n extern int pager_in_use(void);\n-extern int pager_use_color;\n+extern int pager_use_color_config;\n+extern int pager_use_color(void);\n extern int term_columns(void);\n extern int decimal_width(int);\n extern int check_pager_config(const char *cmd);\ndiff --git a/color.c b/color.c\nindex f672885..ffbff40 100644\n--- a/color.c\n+++ b/color.c\n@@ -184,7 +184,7 @@ static int check_auto_color(void)\n {\n \tif (color_stdout_is_tty < 0)\n \t\tcolor_stdout_is_tty = isatty(1);\n-\tif (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {\n+\tif (color_stdout_is_tty || (pager_in_use() && pager_use_color())) {\n \t\tchar *term = getenv(\"TERM\");\n \t\tif (term && strcmp(term, \"dumb\"))\n \t\t\treturn 1;\ndiff --git a/config.c b/config.c\nindex d969a5a..2a8236b 100644\n--- a/config.c\n+++ b/config.c\n@@ -991,7 +991,7 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \t\treturn git_default_advice_config(var, value);\n \n \tif (!strcmp(var, \"pager.color\") || !strcmp(var, \"color.pager\")) {\n-\t\tpager_use_color = git_config_bool(var,value);\n+\t\tpager_use_color_config = git_config_bool(var,value);\n \t\treturn 0;\n \t}\n \ndiff --git a/environment.c b/environment.c\nindex 3c76905..5cd642f 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -39,7 +39,7 @@ size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n size_t delta_base_cache_limit = 16 * 1024 * 1024;\n unsigned long big_file_threshold = 512 * 1024 * 1024;\n const char *pager_program;\n-int pager_use_color = 1;\n+int pager_use_color_config = -1;\n const char *editor_program;\n const char *askpass_program;\n const char *excludes_file;\ndiff --git a/pager.c b/pager.c\nindex 2303164..63e9252 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -184,3 +184,48 @@ int check_pager_config(const char *cmd)\n \t\tpager_program = c.value;\n \treturn c.want;\n }\n+\n+static int pager_can_handle_color(void)\n+{\n+\tconst char *pager = git_pager(1);\n+\n+\t/*\n+\t * If it's less, we automatically set \"R\" and can handle color,\n+\t * unless the user already has a \"LESS\" variable that does not\n+\t * include \"R\".\n+\t */\n+\tif (!strcmp(pager, \"less\")) {\n+\t\tconst char *x = getenv(\"LESS\");\n+\t\treturn !x || !!strchr(x, 'R');\n+\t}\n+\n+\tif (!strcmp(pager, \"more\")) {\n+#ifdef PAGER_MORE_UNDERSTANDS_R\n+\t\t/*\n+\t\t * An advanced \"more\" that knows \"R\" is in the same boat as\n+\t\t * \"less\".\n+\t\t */\n+\t\tconst char *x = getenv(\"MORE\");\n+\t\treturn !x || !!strchr(x, 'R');\n+#else\n+\t\t/*\n+\t\t * For a more primitive \"more\", just assume that it will pass\n+\t\t * through the control codes verbatim.\n+\t\t */\n+\t\treturn 1;\n+#endif\n+\t}\n+\n+\t/*\n+\t * Otherwise, we don't recognize it. Guess that it can probably handle\n+\t * color. This matches what we have done historically.\n+\t */\n+\treturn 1;\n+}\n+\n+int pager_use_color(void)\n+{\n+\tif (pager_use_color_config < 0)\n+\t\tpager_use_color_config = pager_can_handle_color();\n+\treturn pager_use_color_config;\n+}\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233267","messageId":"398F146D-72F1-44CD-B205-729665FD8765@gmail.com","threadId":"35676","inReplyTo":"20140117042153.GB23443@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-01-17T07:26:50Z","receivedAt":"2014-01-17T07:26:50Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jan 16, 2014, at 20:21, Jeff King wrote:\n> When we run the pager, we always set \"LESS=R\" to tell the\n> pager to pass through ANSI colors. On modern versions of\n> FreeBSD, the system \"more\" can do the same trick.\n[snip]\n> diff --git a/pager.c b/pager.c\n> index 90d237e..2303164 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -87,6 +87,10 @@ void setup_pager(void)\n> \t\targv_array_push(&env, \"LESS=FRSX\");\n> \tif (!getenv(\"LV\"))\n> \t\targv_array_push(&env, \"LV=-c\");\n> +#ifdef PAGER_MORE_UNDERSTANDS_R\n> +\tif (!getenv(\"MORE\"))\n> +\t\targv_array_push(&env, \"MORE=R\");\n> +#endif\n\nHow about adding a leading \"-\" to both the LESS and MORE settings? \nSince you're in there patching... :)\n\nThe man page for more states:\n\n\"Options are also taken from the environment variable MORE (make sure \nto precede them with a dash (``-'')) but command line options will \noverride them.\"\n\nAnd while the less man page does not have that wording, it does show \nthis:\n\n LESS=\"-options\"; export LESS\n\nand this:\n\n LESS=\"-Dn9.1$-Ds4.1\"\n\nSo it looks like both LESS and MORE prefer to have their options start \nwith a '-' more or less.\n"},{"id":"233270","messageId":"52D8F43C.9020709@rawbw.com","threadId":"35676","inReplyTo":"20140117041430.GB19551@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] improved out-of-the-box color settings","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-01-17T09:13:32Z","receivedAt":"2014-01-17T09:13:32Z","isPatch":true,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On 01/16/2014 20:14, Jeff King wrote:\n> The second patch turns on \"MORE=R\" only for FreeBSD. Yuri, if you can\n> confirm that my patch works for you, that would be excellent. And if you\n> (or anyone) has access to NetBSD or OpenBSD to test the second patch on,\n> I'd welcome updates to config.mak.uname for those systems.\n\nI applied 3 patches, and it fixed the issue for me.\n\nThank you for such a fast response! This is very impressive!\nI saw this issue for a long while, and only last night took time to \nreport it.\n\nUnfortunately, I don't have any other BSDs sitting around. The easiest \nway to try this is to install them in VM.\n\nYuri\n"},{"id":"233302","messageId":"xmqqzjmuwhi7.fsf@gitster.dls.corp.google.com","threadId":"35676","inReplyTo":"398F146D-72F1-44CD-B205-729665FD8765@gmail.com","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-17T19:11:28Z","receivedAt":"2014-01-17T19:11:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> On Jan 16, 2014, at 20:21, Jeff King wrote:\n>> When we run the pager, we always set \"LESS=R\" to tell the\n>> pager to pass through ANSI colors. On modern versions of\n>> FreeBSD, the system \"more\" can do the same trick.\n> [snip]\n>> diff --git a/pager.c b/pager.c\n>> index 90d237e..2303164 100644\n>> --- a/pager.c\n>> +++ b/pager.c\n>> @@ -87,6 +87,10 @@ void setup_pager(void)\n>> \t\targv_array_push(&env, \"LESS=FRSX\");\n>> \tif (!getenv(\"LV\"))\n>> \t\targv_array_push(&env, \"LV=-c\");\n>> +#ifdef PAGER_MORE_UNDERSTANDS_R\n>> +\tif (!getenv(\"MORE\"))\n>> +\t\targv_array_push(&env, \"MORE=R\");\n>> +#endif\n>\n> How about adding a leading \"-\" to both the LESS and MORE settings?\n> Since you're in there patching... :)\n\nThe discussion we had when LV=-c was added, namely $gmane/240124,\nagrees. I however am perfectly fine to see it done as a separate\nclean-up patch.\n"},{"id":"233303","messageId":"xmqqvbxiwh8y.fsf@gitster.dls.corp.google.com","threadId":"35676","inReplyTo":"20140117042153.GB23443@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-17T19:17:01Z","receivedAt":"2014-01-17T19:17:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/pager.c b/pager.c\n> index 90d237e..2303164 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -87,6 +87,10 @@ void setup_pager(void)\n> \t\targv_array_push(&env, \"LESS=FRSX\");\n> \tif (!getenv(\"LV\"))\n> \t\targv_array_push(&env, \"LV=-c\");\n> +#ifdef PAGER_MORE_UNDERSTANDS_R\n> +\tif (!getenv(\"MORE\"))\n> +\t\targv_array_push(&env, \"MORE=R\");\n> +#endif\n> \tpager_process.env = argv_array_detach(&env, NULL);\n> \n> \tif (start_command(&pager_process))\n\nLet me repeat from $gmane/240110:\n\n - Can we generalize this a bit so that a builder can pass a list\n of var=val pairs and demote the existing LESS=FRSX to just a\n canned setting of such a mechanism?\n\nAs we need to maintain this \"set these environments when unset\" here\nand also in git-sh-setup.sh, I think it is about time to do that\nclean-up. Duplicating two settings was borderline OK, but seeing\nthe third added fairly soon after the second was added tells me that\nthe clean-up must come before adding the third.\n"},{"id":"233310","messageId":"xmqqiotiwfdk.fsf@gitster.dls.corp.google.com","threadId":"35676","inReplyTo":"xmqqvbxiwh8y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-17T19:57:27Z","receivedAt":"2014-01-17T19:57:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> diff --git a/pager.c b/pager.c\n>> index 90d237e..2303164 100644\n>> --- a/pager.c\n>> +++ b/pager.c\n>> @@ -87,6 +87,10 @@ void setup_pager(void)\n>> \t\targv_array_push(&env, \"LESS=FRSX\");\n>> \tif (!getenv(\"LV\"))\n>> \t\targv_array_push(&env, \"LV=-c\");\n>> +#ifdef PAGER_MORE_UNDERSTANDS_R\n>> +\tif (!getenv(\"MORE\"))\n>> +\t\targv_array_push(&env, \"MORE=R\");\n>> +#endif\n>> \tpager_process.env = argv_array_detach(&env, NULL);\n>> \n>> \tif (start_command(&pager_process))\n>\n> Let me repeat from $gmane/240110:\n>\n> - Can we generalize this a bit so that a builder can pass a list\n> of var=val pairs and demote the existing LESS=FRSX to just a\n> canned setting of such a mechanism?\n>\n> As we need to maintain this \"set these environments when unset\" here\n> and also in git-sh-setup.sh, I think it is about time to do that\n> clean-up. Duplicating two settings was borderline OK, but seeing\n> the third added fairly soon after the second was added tells me that\n> the clean-up must come before adding the third.\n\nPerhaps we can start it like this. Then pager.c can iterate over\nthe PAGER_ENV string, parse out LESS=, LV=, etc., and do its thing.\n\nWe would also need to muck with git-sh-setup.sh but that file is\nalready preprocessed in the Makefile, so we should be able to do\nsomething similar to \"# @@BROKEN_PATH_FIX@@\" there.\n\n Makefile | 15 ++++++++++++++-\n config.mak.uname | 1 +\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex b4af1e2..c9e7847 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -342,6 +342,14 @@ all::\n # Define DEFAULT_HELP_FORMAT to \"man\", \"info\" or \"html\"\n # (defaults to \"man\") if you want to have a different default when\n # \"git help\" is called without a parameter specifying the format.\n+#\n+# Define PAGER_ENV to a SP separated VAR=VAL pairs to define\n+# default environment variables to be passed when a pager is spawned, e.g.\n+#\n+# PAGER_ENV = LESS=-FRSX LV=-c\n+#\n+# to say \"export LESS=-FRSX (and LV=-c) if the environment variable\n+# LESS (and LV) is not set, respectively\".\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -1506,6 +1514,10 @@ ifeq ($(PYTHON_PATH),)\n NO_PYTHON = NoThanks\n endif\n \n+ifndef PAGER_ENV\n+PAGER_ENV = LESS=-FRSX LV=-c\n+endif\n+\n QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir\n QUIET_SUBDIR1 =\n \n@@ -1585,11 +1597,12 @@ PYTHON_PATH_SQ = $(subst ','\\'',$(PYTHON_PATH))\n TCLTK_PATH_SQ = $(subst ','\\'',$(TCLTK_PATH))\n DIFF_SQ = $(subst ','\\'',$(DIFF))\n PERLLIB_EXTRA_SQ = $(subst ','\\'',$(PERLLIB_EXTRA))\n+PAGER_ENV_SQ = $(subst ','\\'',$(PAGER_ENV))\n \n LIBS = $(GITLIBS) $(EXTLIBS)\n \n BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \\\n-\t$(COMPAT_CFLAGS)\n+\t$(COMPAT_CFLAGS) -DPAGER_ENV='$(PAGER_ENV_SQ)'\n LIB_OBJS += $(COMPAT_OBJS)\n \n # Quote for C\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 7d31fad..8aea8a6 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -188,6 +188,7 @@ ifeq ($(uname_S),FreeBSD)\n \tendif\n \tPYTHON_PATH = /usr/local/bin/python\n \tHAVE_PATHS_H = YesPlease\n+\tPAGER_ENV = LESS=-FRSX LV=-c MORE=-R\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\n"},{"id":"233317","messageId":"52D98F58.9050808@rawbw.com","threadId":"35676","inReplyTo":"20140117014758.GF7249@sigill.intra.peff.net","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-01-17T20:15:20Z","receivedAt":"2014-01-17T20:15:20Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"One other idea to handle this is at configuration phase. You can test \nmore and less with every option used on every platform for each of them \nrespectively. Test could run the command with the option, and check if \nit passes the given escape sequence. This would be reflected in config.h \ndefines like this: MORE_DASH_R_WORKS This would be very accurate.\n\nNot sure if this is an overkill for this issue.\n\nYuri\n"},{"id":"233328","messageId":"xmqq61piw4yf.fsf@gitster.dls.corp.google.com","threadId":"35676","inReplyTo":"xmqqiotiwfdk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-17T23:42:32Z","receivedAt":"2014-01-17T23:42:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Perhaps we can start it like this. Then pager.c can iterate over\n> the PAGER_ENV string, parse out LESS=, LV=, etc., and do its thing.\n>\n> We would also need to muck with git-sh-setup.sh but that file is\n> already preprocessed in the Makefile, so we should be able to do\n> something similar to \"# @@BROKEN_PATH_FIX@@\" there.\n\nAnd here is such an attempt. There may be some missing dependencies\nthat need to be covered with a mechanism like GIT-BUILD-OPTIONS,\nthough.\n\n Makefile | 18 ++++++++++++++++--\n config.mak.uname | 1 +\n git-sh-setup.sh | 9 +++++----\n pager.c | 44 +++++++++++++++++++++++++++++++++-----------\n 4 files changed, 55 insertions(+), 17 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex b4af1e2..690f4c6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -342,6 +342,14 @@ all::\n # Define DEFAULT_HELP_FORMAT to \"man\", \"info\" or \"html\"\n # (defaults to \"man\") if you want to have a different default when\n # \"git help\" is called without a parameter specifying the format.\n+#\n+# Define PAGER_ENV to a SP separated VAR=VAL pairs to define\n+# default environment variables to be passed when a pager is spawned, e.g.\n+#\n+# PAGER_ENV = LESS=-FRSX LV=-c\n+#\n+# to say \"export LESS=-FRSX (and LV=-c) if the environment variable\n+# LESS (and LV) is not set, respectively\".\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -1506,6 +1514,10 @@ ifeq ($(PYTHON_PATH),)\n NO_PYTHON = NoThanks\n endif\n \n+ifndef PAGER_ENV\n+PAGER_ENV = LESS=-FRSX LV=-c\n+endif\n+\n QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir\n QUIET_SUBDIR1 =\n \n@@ -1585,11 +1597,12 @@ PYTHON_PATH_SQ = $(subst ','\\'',$(PYTHON_PATH))\n TCLTK_PATH_SQ = $(subst ','\\'',$(TCLTK_PATH))\n DIFF_SQ = $(subst ','\\'',$(DIFF))\n PERLLIB_EXTRA_SQ = $(subst ','\\'',$(PERLLIB_EXTRA))\n+PAGER_ENV_SQ = $(subst ','\\'',$(PAGER_ENV))\n \n LIBS = $(GITLIBS) $(EXTLIBS)\n \n BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \\\n-\t$(COMPAT_CFLAGS)\n+\t$(COMPAT_CFLAGS) -DPAGER_ENV='$(PAGER_ENV)'\n LIB_OBJS += $(COMPAT_OBJS)\n \n # Quote for C\n@@ -1739,7 +1752,7 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)\n \n SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\\\n \t$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\\\n-\t$(gitwebdir_SQ):$(PERL_PATH_SQ)\n+\t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(PAGER_ENV)\n define cmd_munge_script\n $(RM) $@ $@+ && \\\n sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n@@ -1751,6 +1764,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n -e $(BROKEN_PATH_FIX) \\\n -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \\\n -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n+ -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \\\n $@.sh >$@+\n endef\n \ndiff --git a/config.mak.uname b/config.mak.uname\nindex 7d31fad..8aea8a6 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -188,6 +188,7 @@ ifeq ($(uname_S),FreeBSD)\n \tendif\n \tPYTHON_PATH = /usr/local/bin/python\n \tHAVE_PATHS_H = YesPlease\n+\tPAGER_ENV = LESS=-FRSX LV=-c MORE=-R\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex fffa3c7..8fc1bbd 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -158,10 +158,11 @@ git_pager() {\n \telse\n \t\tGIT_PAGER=cat\n \tfi\n-\t: ${LESS=-FRSX}\n-\t: ${LV=-c}\n-\texport LESS LV\n-\n+\tfor vardef in @@PAGER_ENV@@\n+\tdo\n+\t\tvar=${vardef%%=*}\n+\t\teval \": \\${$vardef} && export $var\"\n+\tdone\n \teval \"$GIT_PAGER\" '\"$@\"'\n }\n \ndiff --git a/pager.c b/pager.c\nindex 0cc75a8..81a8af7 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n+#include \"argv-array.h\"\n \n #ifndef DEFAULT_PAGER\n #define DEFAULT_PAGER \"less\"\n@@ -60,9 +61,37 @@ const char *git_pager(int stdout_is_tty)\n \treturn pager;\n }\n \n+#define stringify_(x) #x\n+#define stringify(x) stringify_(x)\n+\n+static void setup_pager_env(struct argv_array *env)\n+{\n+\tconst char *pager_env = stringify(PAGER_ENV);\n+\n+\twhile (*pager_env) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tconst char *cp = strchrnul(pager_env, '=');\n+\n+\t\tif (!*cp)\n+\t\t\tdie(\"malformed build-time PAGER_ENV\");\n+\t\tstrbuf_add(&buf, pager_env, cp - pager_env);\n+\t\tcp = strchrnul(pager_env, ' ');\n+\t\tif (!getenv(buf.buf)) {\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tstrbuf_add(&buf, pager_env, cp - pager_env);\n+\t\t\targv_array_push(env, strbuf_detach(&buf, NULL));\n+\t\t}\n+\t\tstrbuf_reset(&buf);\n+\t\twhile (*cp && *cp == ' ')\n+\t\t\tcp++;\n+\t\tpager_env = cp;\n+\t}\n+}\n+\n void setup_pager(void)\n {\n \tconst char *pager = git_pager(isatty(1));\n+\tstruct argv_array env = ARGV_ARRAY_INIT;\n \n \tif (!pager || pager_in_use())\n \t\treturn;\n@@ -80,17 +109,10 @@ void setup_pager(void)\n \tpager_process.use_shell = 1;\n \tpager_process.argv = pager_argv;\n \tpager_process.in = -1;\n-\tif (!getenv(\"LESS\") || !getenv(\"LV\")) {\n-\t\tstatic const char *env[3];\n-\t\tint i = 0;\n-\n-\t\tif (!getenv(\"LESS\"))\n-\t\t\tenv[i++] = \"LESS=FRSX\";\n-\t\tif (!getenv(\"LV\"))\n-\t\t\tenv[i++] = \"LV=-c\";\n-\t\tenv[i] = NULL;\n-\t\tpager_process.env = env;\n-\t}\n+\n+\tsetup_pager_env(&env);\n+\tpager_process.env = argv_array_detach(&env, NULL);\n+\n \tif (start_command(&pager_process))\n \t\treturn;\n \n"},{"id":"233431","messageId":"20140121053026.GC5878@sigill.intra.peff.net","threadId":"35676","inReplyTo":"398F146D-72F1-44CD-B205-729665FD8765@gmail.com","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-21T05:30:26Z","receivedAt":"2014-01-21T05:30:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2014 at 11:26:50PM -0800, Kyle J. McKay wrote:\n\n> >--- a/pager.c\n> >+++ b/pager.c\n> >@@ -87,6 +87,10 @@ void setup_pager(void)\n> >\t\targv_array_push(&env, \"LESS=FRSX\");\n> >\tif (!getenv(\"LV\"))\n> >\t\targv_array_push(&env, \"LV=-c\");\n> >+#ifdef PAGER_MORE_UNDERSTANDS_R\n> >+\tif (!getenv(\"MORE\"))\n> >+\t\targv_array_push(&env, \"MORE=R\");\n> >+#endif\n> \n> How about adding a leading \"-\" to both the LESS and MORE settings?\n> Since you're in there patching... :)\n\nI do not have any problem with that, but would very much prefer it as a\nseparate patch, in case there are any fallouts.\n\n> And while the less man page does not have that wording, it does show\n> this:\n> \n> LESS=\"-options\"; export LESS\n> \n> and this:\n> \n> LESS=\"-Dn9.1$-Ds4.1\"\n\nUgh. Having just read the LESS discussion, it makes me wonder if the\n\n strchr(getenv(\"LESS\"), 'R')\n\ncheck I add elsewhere in the series is sufficient. I suspect in practice\nit is good enough, but I would not be surprised if there is a way to\nfool it with a sufficiently complex LESS variable. Maybe we should just\nassume it is enough, and people with crazy LESS settings can set\ncolor.pager as appropriate?\n"},{"id":"233432","messageId":"20140121054927.GD5878@sigill.intra.peff.net","threadId":"35676","inReplyTo":"xmqqvbxiwh8y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-21T05:49:27Z","receivedAt":"2014-01-21T05:49:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 17, 2014 at 11:17:01AM -0800, Junio C Hamano wrote:\n\n> > +#ifdef PAGER_MORE_UNDERSTANDS_R\n> > +\tif (!getenv(\"MORE\"))\n> > +\t\targv_array_push(&env, \"MORE=R\");\n> > +#endif\n> > \tpager_process.env = argv_array_detach(&env, NULL);\n> > \n> > \tif (start_command(&pager_process))\n> \n> Let me repeat from $gmane/240110:\n> \n> - Can we generalize this a bit so that a builder can pass a list\n> of var=val pairs and demote the existing LESS=FRSX to just a\n> canned setting of such a mechanism?\n> \n> As we need to maintain this \"set these environments when unset\" here\n> and also in git-sh-setup.sh, I think it is about time to do that\n> clean-up. Duplicating two settings was borderline OK, but seeing\n> the third added fairly soon after the second was added tells me that\n> the clean-up must come before adding the third.\n\nWow, I somehow _completely_ missed that thread. When I looked at the\ncode with LV, I assumed it was just something that had happened long ago\nand I had forgotten about. I didn't even bother reading \"git log\".\n\nYeah, I agree that git-sh-setup should be consistent with what the C\nporcelains do. However, adding build-time variables like:\n\n PAGER_ENV = LESS=-FRSX LV=-c\n\ndoes complicate the point of my series, which was to add more intimate\nlogic about how we handle LESS. With the current code, I can know that\nan unset \"LESS\" variable means we will set \"R\" ourselves and turn on\ncolor. We can get around that with something like:\n\n int pager_can_handle_color(void)\n {\n const char *pager = get_pager();\n\n if (!strcmp(pager, \"less\")) {\n const char *x = getenv(\"LESS\");\n if (!x) {\n const char *e;\n for (e = pager_env; *e; e++)\n if ((x = skip_prefix(e, \"LESS=\")))\n break;\n }\n return !x || strchr(x, 'R');\n }\n\n [...]\n }\n\nbut we are still hard-coding a lot of intelligence about \"less\" here. I\nwonder if the abstraction provided by the Makefile variable is really\nworthwhile. Anybody adding a new line to it would also want to tweak\npager_can_handle_color to add similar logic.\n\nTaking a step back for a moment, we are getting two things out of such a\nMakefile variable:\n\n 1. An easy way for packager to add intelligence about common pagers on\n their system.\n\n 2. DRY between git-sh-setup and the C code.\n\nI do like (1), but I do not know if we want to try to encode the \"can\nhandle color\" logic into a Makefile variable. What would it look like?\n\nFor (2), an alternate method would be to simply provide \"git pager\" as C\ncode, which spawns the appropriate pager as the C code would. Then\ngit-sh-setup can easily build around that.\n\n-Peff\n"},{"id":"233433","messageId":"20140121061307.GE5878@sigill.intra.peff.net","threadId":"35676","inReplyTo":"xmqq61piw4yf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-21T06:13:07Z","receivedAt":"2014-01-21T06:13:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 17, 2014 at 03:42:32PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Perhaps we can start it like this. Then pager.c can iterate over\n> > the PAGER_ENV string, parse out LESS=, LV=, etc., and do its thing.\n> >\n> > We would also need to muck with git-sh-setup.sh but that file is\n> > already preprocessed in the Makefile, so we should be able to do\n> > something similar to \"# @@BROKEN_PATH_FIX@@\" there.\n> \n> And here is such an attempt. There may be some missing dependencies\n> that need to be covered with a mechanism like GIT-BUILD-OPTIONS,\n> though.\n\nPerhaps instead of just dumping it into a macro, we could actually parse\nit at compile time into C code, and store the result as a header file.\nThen that header file becomes the proper dependency, and this run-time\nerror:\n\n> +\twhile (*pager_env) {\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\tconst char *cp = strchrnul(pager_env, '=');\n> +\n> +\t\tif (!*cp)\n> +\t\t\tdie(\"malformed build-time PAGER_ENV\");\n\nwould become a compile-time error. We could do the same for the shell\ncode, too.\n\nI'm thinking something like:\n\ndiff --git a/Makefile b/Makefile\nindex b4af1e2..3a8d15e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2182,6 +2182,16 @@ GIT-LDFLAGS: FORCE\n \t\techo \"$$FLAGS\" >GIT-LDFLAGS; \\\n fi\n \n+GIT-PAGER-ENV:\n+\t@PAGER_ENV='$(PAGER_ENV)'; \\\n+\tif test x\"$$PAGER_ENV\" != x\"`cat GIT-PAGER-ENV 2>/dev/null`\"; then \\\n+\t\techo \"$$PAGER_ENV\" >GIT-PAGER-ENV; \\\n+\tfi\n+\n+pager-env.h: GIT-PAGER-ENV mkcarray\n+\t$(SHELL_PATH) mkcarray pager_env <$< >$@+\n+\tmv $@+ $@\n+\n # We need to apply sq twice, once to protect from the shell\n # that runs GIT-BUILD-OPTIONS, and then again to protect it\n # and the first level quoting from the shell that runs \"echo\".\ndiff --git a/mkcarray b/mkcarray\nindex e69de29..5962440 100644\n--- a/mkcarray\n+++ b/mkcarray\n@@ -0,0 +1,21 @@\n+name=$1; shift\n+guard=$(echo \"$name\" | tr a-z A-Z)\n+\n+cat <<-EOF\n+#ifndef ${guard}_H\n+#define ${guard}_H\n+\n+static const char *${name} = {\n+EOF\n+\n+for i in $(cat); do\n+\t# XXX c-quote the values?\n+\tprintf '\\t\"%s\",\\n' \"$i\"\n+done\n+\n+cat < Ugh. Having just read the LESS discussion, it makes me wonder if the\n>\n> strchr(getenv(\"LESS\"), 'R')\n>\n> check I add elsewhere in the series is sufficient. I suspect in \n> practice\n> it is good enough, but I would not be surprised if there is a way to\n> fool it with a sufficiently complex LESS variable. Maybe we should \n> just\n> assume it is enough, and people with crazy LESS settings can set\n> color.pager as appropriate?\n\n\nThis attempts to show colors but doesn't work properly:\n\n LESS=-+R git -c color.ui=auto -c color.pager=true log --oneline -- \ngraph\n\nbut this does\n\n LESS=-R git -c color.ui=auto -c color.pager=true log --oneline -- \ngraph\n\nso yeah, just checking for 'R' in $LESS is only approximate. :)\nAlso -r is good enough to show colors too...\n"},{"id":"233450","messageId":"xmqqwqhtuojx.fsf@gitster.dls.corp.google.com","threadId":"35676","inReplyTo":"20140121054927.GD5878@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-21T19:23:30Z","receivedAt":"2014-01-21T19:23:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ...\n> does complicate the point of my series, which was to add more intimate\n> logic about how we handle LESS.\n> ...\n> return !x || strchr(x, 'R');\n> }\n>\n> [...]\n> }\n>\n> but we are still hard-coding a lot of intelligence about \"less\" here.\n\nI am not sure if it is even a good idea for us to have so intimate\nlogic for various pagers in the first place. I'd seriously wonder\nif it is better to take this position:\n\n\tA platform packager who sets the default pager and/or the\n\tdefault environment for the pager at the build time, or an\n\tindividual user who tells your Git what pager you want to\n\tuse and/or with what setting that pager should be run under\n\twith environment variables. These people ought to know far\n\tbetter than Git what their specific choices do. Do not try\n\tto second-guess them.\n\nThe potential breakage caused by setting MORE=R unconditionally is a\ngood example. A careless \"intimate logic\" may think that any pager\nthat is called 'more' would behave like traditional 'more', breaking\nhalf the 'more' user population while catering to the other half.\n\n> I\n> wonder if the abstraction provided by the Makefile variable is really\n> worthwhile. Anybody adding a new line to it would also want to tweak\n> pager_can_handle_color to add similar logic.\n\nAnd that is why I am not enthused by the idea of adding such logic\nin the first place. I view the Makefile customization as a way for\nthe packager to offer a sensible default for their platform without\ntouching the code, which is slightly different from your 1. below.\n\n> Taking a step back for a moment, we are getting two things out of such a\n> Makefile variable:\n>\n> 1. An easy way for packager to add intelligence about common pagers on\n> their system.\n>\n> 2. DRY between git-sh-setup and the C code.\n>\n> I do like (1), but I do not know if we want to try to encode the \"can\n> handle color\" logic into a Makefile variable. What would it look like?\n>\n> For (2), an alternate method would be to simply provide \"git pager\" as C\n> code, which spawns the appropriate pager as the C code would. Then\n> git-sh-setup can easily build around that.\n\nAnd as to 2., if the answer to the other issue \"do we want our code\nto be intimately aware of pager-specific quirks, or do we just want\nto give packagers a knob to express their choice of the default?\"\nresolves to the former, I would think that \"git pager\" would be not\njust a workable alternative, but would be the only viable one.\n"},{"id":"233451","messageId":"xmqqsishuobv.fsf@gitster.dls.corp.google.com","threadId":"35676","inReplyTo":"20140121061307.GE5878@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-21T19:28:20Z","receivedAt":"2014-01-21T19:28:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Perhaps instead of just dumping it into a macro, we could actually parse\n> it at compile time into C code, and store the result as a header file.\n> Then that header file becomes the proper dependency, and this run-time\n> error:\n>\n>> +\twhile (*pager_env) {\n>> +\t\tstruct strbuf buf = STRBUF_INIT;\n>> +\t\tconst char *cp = strchrnul(pager_env, '=');\n>> +\n>> +\t\tif (!*cp)\n>> +\t\t\tdie(\"malformed build-time PAGER_ENV\");\n>\n> would become a compile-time error. We could do the same for the shell\n> code, too.\n>\n> I'm thinking something like:\n\nHeh, great minds think alike. I did start writing a toy-patch with\na new file called \"pager-env.h\", before discarding it and sending a\nsimpler alternative which you are responding to.\n\nI do not mind the approach at all; the primary reason I stopped\ndoing that was because the amount of code to get quotes right turned\nout to be sufficiently big to obscure the real point of the \"how\nabout doing it this way\" illustration patch.\n\n> diff --git a/Makefile b/Makefile\n> index b4af1e2..3a8d15e 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2182,6 +2182,16 @@ GIT-LDFLAGS: FORCE\n> \t\techo \"$$FLAGS\" >GIT-LDFLAGS; \\\n> fi\n> \n> +GIT-PAGER-ENV:\n> +\t@PAGER_ENV='$(PAGER_ENV)'; \\\n> +\tif test x\"$$PAGER_ENV\" != x\"`cat GIT-PAGER-ENV 2>/dev/null`\"; then \\\n> +\t\techo \"$$PAGER_ENV\" >GIT-PAGER-ENV; \\\n> +\tfi\n> +\n> +pager-env.h: GIT-PAGER-ENV mkcarray\n> +\t$(SHELL_PATH) mkcarray pager_env <$< >$@+\n> +\tmv $@+ $@\n> +\n> # We need to apply sq twice, once to protect from the shell\n> # that runs GIT-BUILD-OPTIONS, and then again to protect it\n> # and the first level quoting from the shell that runs \"echo\".\n> diff --git a/mkcarray b/mkcarray\n> index e69de29..5962440 100644\n> --- a/mkcarray\n> +++ b/mkcarray\n> @@ -0,0 +1,21 @@\n> +name=$1; shift\n> +guard=$(echo \"$name\" | tr a-z A-Z)\n> +\n> +cat <<-EOF\n> +#ifndef ${guard}_H\n> +#define ${guard}_H\n> +\n> +static const char *${name} = {\n> +EOF\n> +\n> +for i in $(cat); do\n> +\t# XXX c-quote the values?\n> +\tprintf '\\t\"%s\",\\n' \"$i\"\n> +done\n> +\n> +cat < +\tNULL\n> +};\n> +\n> +#endif /* ${guard}_H */\n> +EOF\n"},{"id":"233581","messageId":"20140123021407.GG17254@sigill.intra.peff.net","threadId":"35676","inReplyTo":"5B91161D-7B06-4C80-9554-89F379C4E2A6@gmail.com","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-23T02:14:07Z","receivedAt":"2014-01-23T02:14:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 21, 2014 at 12:42:58AM -0800, Kyle J. McKay wrote:\n\n> This attempts to show colors but doesn't work properly:\n> \n> LESS=-+R git -c color.ui=auto -c color.pager=true log --oneline --graph\n> \n> but this does\n> \n> LESS=-R git -c color.ui=auto -c color.pager=true log --oneline --graph\n> \n> so yeah, just checking for 'R' in $LESS is only approximate. :)\n> Also -r is good enough to show colors too...\n\nUgh, yeah. I think LESS=+R would also break. Doing it right would\ninvolve parsing less's option format, including \"-\", \"+\", and \"-+\". It's\nnot _that_ much code, but it feels awfully wrong to be so intimate with\na subprogram that we do not control.\n\n-Peff\n"},{"id":"233603","messageId":"xmqqtxcuvcjl.fsf@gitster.dls.corp.google.com","threadId":"35676","inReplyTo":"20140123021407.GG17254@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-23T17:22:06Z","receivedAt":"2014-01-23T17:22:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ..., but it feels awfully wrong to be so intimate with\n> a subprogram that we do not control.\n\nYeah, I think we are in agreement on that point.\n"},{"id":"234239","messageId":"20140204221220.GA5457@sigill.intra.peff.net","threadId":"35676","inReplyTo":"xmqqwqhtuojx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-04T22:12:20Z","receivedAt":"2014-02-04T22:12:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote:\n\n> > does complicate the point of my series, which was to add more intimate\n> > logic about how we handle LESS.\n> > ...\n> > return !x || strchr(x, 'R');\n> [...]\n> \n> I am not sure if it is even a good idea for us to have so intimate\n> logic for various pagers in the first place. I'd seriously wonder\n> if it is better to take this position:\n> \n> \tA platform packager who sets the default pager and/or the\n> \tdefault environment for the pager at the build time, or an\n> \tindividual user who tells your Git what pager you want to\n> \tuse and/or with what setting that pager should be run under\n> \twith environment variables. These people ought to know far\n> \tbetter than Git what their specific choices do. Do not try\n> \tto second-guess them.\n\nSorry for letting this topic lapse; I wanted to look at some possible\nMakefile improvements, which I'll be posting in a moment. I did want to\naddress this point, though.\n\nIf we are just talking about packagers and defaults (e.g., setting\nMORE=R on FreeBSD), then I agree that the right tool for the job is\nempowering the packagers, and the Makefile knob we've discussed does\nthat.\n\nBut there's another set of people that I was intending to help with the\npatch, which is people that have set up LESS, and did not necessarily\ncare about the \"R\" flag in the past (e.g., for many years before git\ncame along, I set LESS=giM, and never even knew that \"R\" existed). Since\ngit comes out of the box these days with color and the pager turned on,\nthat means people with such a setup see broken output from day one.\n\nAnd I think it is Git's fault here, not the user or the packager. Our\ndefaults assume that the user's pager can handle color, and that is not\nthe default for many pagers, including our default \"less\"! The user did\nnot turn off \"R\" here; they simply set other options they cared about,\nand our hacky workaround to auto-enable \"R\" did not kick in.\n\nIt seems a shame to me that we cannot do better for such users.\nHowever, the level of intimacy with the pager is getting to be a bit too\nmuch for my taste, and the saving grace is that not that many people set\nLESS themselves (and git is widely enough known that \"R\" is a common\nrecommendation when people do). So it's probably the lesser of two evils\nto ignore the situation, and let people who run into it find the answers\non the web.\n\nSo I think there is nothing to be done. But I did want to mention it in\ncase somebody else can come up with some clever solution. :)\n\n-Peff\n"},{"id":"234247","messageId":"xmqqiosu7ca2.fsf@gitster.dls.corp.google.com","threadId":"35676","inReplyTo":"20140204221220.GA5457@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-04T22:17:57Z","receivedAt":"2014-02-04T22:17:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But there's another set of people that I was intending to help with the\n> patch, which is people that have set up LESS, and did not necessarily\n> care about the \"R\" flag in the past (e.g., for many years before git\n> came along, I set LESS=giM, and never even knew that \"R\" existed). Since\n> git comes out of the box these days with color and the pager turned on,\n> that means people with such a setup see broken output from day one.\n>\n> And I think it is Git's fault here, not the user or the packager.\n\nI am not particularly itnterested in whose fault it is ;-) If the\nuser sets LESS himself, he knows how to set it (and if he is setting\nit automatically for all of his sessions, he knows where to do so),\nand would know better than Git about \"less\", his pager of choice.\n\nIf he did not know about R and did not see color, that is even\nbetter. Now he knows and his update to his LESS settings will let\nhim view colors in outputs from programs that are not Git.\n\n> So I think there is nothing to be done. But I did want to mention it in\n> case somebody else can come up with some clever solution. :)\n\nSure.\n"},{"id":"234251","messageId":"20140204222507.GA6031@sigill.intra.peff.net","threadId":"35676","inReplyTo":"xmqqiosu7ca2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-04T22:25:08Z","receivedAt":"2014-02-04T22:25:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 04, 2014 at 02:17:57PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But there's another set of people that I was intending to help with the\n> > patch, which is people that have set up LESS, and did not necessarily\n> > care about the \"R\" flag in the past (e.g., for many years before git\n> > came along, I set LESS=giM, and never even knew that \"R\" existed). Since\n> > git comes out of the box these days with color and the pager turned on,\n> > that means people with such a setup see broken output from day one.\n> >\n> > And I think it is Git's fault here, not the user or the packager.\n> \n> I am not particularly itnterested in whose fault it is ;-) If the\n> user sets LESS himself, he knows how to set it (and if he is setting\n> it automatically for all of his sessions, he knows where to do so),\n> and would know better than Git about \"less\", his pager of choice.\n> \n> If he did not know about R and did not see color, that is even\n> better. Now he knows and his update to his LESS settings will let\n> him view colors in outputs from programs that are not Git.\n\nRight. If git just disabled the color, I think that would be sane (and\nthat is what my patch was shooting for). But somebody who sees:\n\n $ git log\n ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m\n (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m\n\ndoes not necessarily know what is going on. They do not know that it is\na \"less\" problem, nor that their less settings are relevant. They only\nsee that Git is broken out of the box.\n\nAnyway, I will leave it at that. It's an unfortunate problem, but one\nnot worth fixing.\n\n-Peff\n"},{"id":"234254","messageId":"52F16D77.2000604@rawbw.com","threadId":"35676","inReplyTo":"20140204222507.GA6031@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-02-04T22:45:11Z","receivedAt":"2014-02-04T22:45:11Z","isPatch":true,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On 02/04/2014 14:25, Jeff King wrote:\n> Right. If git just disabled the color, I think that would be sane (and\n> that is what my patch was shooting for). But somebody who sees:\n>\n> $ git log\n> ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m\n> (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m\n>\n> does not necessarily know what is going on. They do not know that it is\n> a \"less\" problem, nor that their less settings are relevant. They only\n> see that Git is broken out of the box.\n\nMaybe, instead of doing all the elaborate guess and assumption work, \nhave configure script check if the current PAGER supports colors and \nbuild git accordingly?\nconfigure could run the pager as one of its tests, and determine if \n\"ESC\" appears on the output.\n\nYuri\n"},{"id":"234256","messageId":"20140204224803.GB6317@sigill.intra.peff.net","threadId":"35676","inReplyTo":"52F16D77.2000604@rawbw.com","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-04T22:48:03Z","receivedAt":"2014-02-04T22:48:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 04, 2014 at 02:45:11PM -0800, Yuri wrote:\n\n> On 02/04/2014 14:25, Jeff King wrote:\n> >Right. If git just disabled the color, I think that would be sane (and\n> >that is what my patch was shooting for). But somebody who sees:\n> >\n> > $ git log\n> > ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m\n> > (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m\n> >\n> >does not necessarily know what is going on. They do not know that it is\n> >a \"less\" problem, nor that their less settings are relevant. They only\n> >see that Git is broken out of the box.\n> \n> Maybe, instead of doing all the elaborate guess and assumption work,\n> have configure script check if the current PAGER supports colors and\n> build git accordingly?\n> configure could run the pager as one of its tests, and determine if\n> \"ESC\" appears on the output.\n\nBut this is not a build-time problem, but rather a run-time problem. We\ndo not know what the environment of the user will be at run-time.\n\nThe safest thing would be to turn off auto-color when LESS (or any of\nthe pager environment variables) is set at all (and not worry about\nwhether \"R\" is set; only know that _we_ are not setting it, so we cannot\ncount on it). But that would potentially inconvenience a lot of people\nwhose default color would suddenly go away.\n\n-Peff\n"},{"id":"234258","messageId":"xmqq61ou7al7.fsf@gitster.dls.corp.google.com","threadId":"35676","inReplyTo":"20140204224803.GB6317@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-04T22:54:28Z","receivedAt":"2014-02-04T22:54:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The safest thing would be to turn off auto-color when LESS (or any of\n> the pager environment variables) is set at all (and not worry about\n> whether \"R\" is set; only know that _we_ are not setting it, so we cannot\n> count on it). But that would potentially inconvenience a lot of people\n> whose default color would suddenly go away.\n\nThat is just as safe as disabling color for everybody, isn't it?\nHalf of existing users have LESS with R, and the other half do not\nhave LESS at all. The former will be harmed, the latter will not\nsee any difference.\n\nOh, and then new users who do not know R for LESS will not even\nnotice that Git could support coloured output. Those among them who\nread manpages and find --color option will then see ESC[33m in their\noutput and we are back to where we started X-<.\n\nSo I think we are already at the safest place. Those who see ESC[33m\nwill know they are missing some good stuff and can ask around, which\nis better than doing anything else at this point.\n\nI think that is the same conclusion as yours, \"there is nothing to\nbe done.\"\n"},{"id":"234259","messageId":"52F1710D.70304@rawbw.com","threadId":"35676","inReplyTo":"20140204224803.GB6317@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-02-04T23:00:29Z","receivedAt":"2014-02-04T23:00:29Z","isPatch":true,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On 02/04/2014 14:48, Jeff King wrote:\n> But this is not a build-time problem, but rather a run-time problem. We\n> do not know what the environment of the user will be at run-time.\n\nOk, git can test the pager on the given system before its first use and \nremember the result in ~/.git for later use for example. Such \n'experimental' approach is maybe more reliable.\n\nYuri\n"},{"id":"234265","messageId":"52F192E7.1000301@rawbw.com","threadId":"35676","inReplyTo":"20140117014758.GF7249@sigill.intra.peff.net","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-02-05T01:24:55Z","receivedAt":"2014-02-05T01:24:55Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On 01/16/2014 17:47, Jeff King wrote:\n> Are you using \"less\" as your pager (it is the default in git unless you\n> have set your PAGER environment variable)? If so, do you have the \"R\"\n> option set to pass through ANSI codes? Git will set this automatically\n> in your \"LESS\" variable if you do not already have such a variable (but\n> it will not touch it if you already have it set, and are missing \"R\").\n\n\nI think the 'experimental' approach is simpler and better.\nWhen the git command requiring pager is first run, git would run the \npager with some simple one line escape sequence, and see if sequence is \npreserved. If it was preserved, git should just run with escape \nsequences. If the pager destroyed the sequence, git should issue a \nwarning to the user:\ngit: your default pager PAGER=more doesn't pass escape sequences, and \nthey will be disabled them. You can revert this decision by changing \nthe file ~/.git/pager.conf\n\nThis way:\n* damaged sequences will not show up by default\n* colors will be displayed by default when possible\n* user would be informed, and will have a clear choice\n* this is easy to implement, and no elaborate and obscure reasoning \nshould be employed in the implementation\n\nFor me, if git would have told me that my pager doesn't support escape \nsequences, I could have taken it from there.\n\nYuri\n"},{"id":"234266","messageId":"20140205013330.GB3923@sigill.intra.peff.net","threadId":"35676","inReplyTo":"52F192E7.1000301@rawbw.com","subject":"Re: 'git log' escape symbols shown as ESC[33 and ESC[m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-05T01:33:30Z","receivedAt":"2014-02-05T01:33:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 04, 2014 at 05:24:55PM -0800, Yuri wrote:\n\n> I think the 'experimental' approach is simpler and better.\n> When the git command requiring pager is first run, git would run the\n> pager with some simple one line escape sequence, and see if sequence\n> is preserved.\n\nSee how? If less's stdout is not connected to a terminal, it simply\npasses through the output as-is. E.g., try:\n\n foo() {\n # blue foo\n printf '\\x1b[34mfoo\\x1b[m'\n }\n\n unset LESS\n foo | less\n\nwhich has the problematic output. Now try:\n\n foo | less | cat\n\nwhich passes through the ANSI codes unscathed. I have no idea how other\npagers behave. So I think this is going back down the road of\nhard-coding lots of pager-specific behaviors.\n\n-Peff\n"},{"id":"234267","messageId":"62DB6DEF-8B39-4481-BA06-245BF45233E5@gmail.com","threadId":"35676","inReplyTo":"20140204221220.GA5457@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup_pager: set MORE=R","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-02-05T02:11:01Z","receivedAt":"2014-02-05T02:11:01Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Feb 4, 2014, at 14:12, Jeff King wrote:\n> On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote:\n>\n>>> does complicate the point of my series, which was to add more \n>>> intimate\n>>> logic about how we handle LESS.\n>>> ...\n>>> return !x || strchr(x, 'R');\n>> [...]\n>>\n>> I am not sure if it is even a good idea for us to have so intimate\n>> logic for various pagers in the first place. I'd seriously wonder\n>> if it is better to take this position:\n>>\n>> \tA platform packager who sets the default pager and/or the\n>> \tdefault environment for the pager at the build time, or an\n>> \tindividual user who tells your Git what pager you want to\n>> \tuse and/or with what setting that pager should be run under\n>> \twith environment variables. These people ought to know far\n>> \tbetter than Git what their specific choices do. Do not try\n>> \tto second-guess them.\n>\n> Sorry for letting this topic lapse; I wanted to look at some possible\n> Makefile improvements, which I'll be posting in a moment. I did want \n> to\n> address this point, though.\n>\n> If we are just talking about packagers and defaults (e.g., setting\n> MORE=R on FreeBSD), then I agree that the right tool for the job is\n> empowering the packagers, and the Makefile knob we've discussed does\n> that.\n>\n[snip]\n>\n> It seems a shame to me that we cannot do better for such users.\n> However, the level of intimacy with the pager is getting to be a bit \n> too\n> much for my taste, and the saving grace is that not that many people \n> set\n> LESS themselves (and git is widely enough known that \"R\" is a common\n> recommendation when people do). So it's probably the lesser of two \n> evils\n> to ignore the situation, and let people who run into it find the \n> answers\n> on the web.\n>\n> So I think there is nothing to be done. But I did want to mention \n> it in\n> case somebody else can come up with some clever solution. :)\n\nI think we can do better without adding intimate pager knowledge. And \nat the same time make it a simple matter of tweaking the Makefile to \ndeal with new pagers on specific systems.\n\nThere are two parts to the proposal.\n\nPart 1\n\nAdd a new configuration item which I will call \"pager.env\" for now \nthat can have multiple values of the form ENV=value (whether multiple \nlines or whitespace separated on a single line to be decided later).\n\nOn a system where more can support color by setting MORE=-R it might \nlook like this:\n\n[pager]\n env = LESS=-FRSX LV=-c MORE=-R\n\nOn a system where more does not it might look like this:\n\n[pager]\n env = LESS=-FRSX LV=-c\n\nThe default value of pager.env is to be configured in a system- \nspecific way (i.e. Makefile knob) at build time.\n\nThen, if Git is going to output color AND start a pager (that logic \nremains unchanged by this proposal), then it processes the pager.env \nvalue by examining each ENV=value setting and if the environment \nvariable ENV is NOT already set, then sets it to value before starting \nthe pager.\n\nThis is mostly a clean up and shouldn't really be controversial since \nbefore commit e54c1f2d2 the system basically behaved like this where \npager.env was always set to \"LESS=FRSX\" and after it behaves as though \npager.env is always set to \"LESS=FRSX LV=-c\".\n\nPart 2\n\nAlongside the current false/never, always, true/auto settings for \n\"color.ui\" a new \"carefully\" setting is introduced and the color.ui \ndefault if not set is changed from auto (commit 4c7f1819) to the new \n\"carefully\" setting.\n\nWhy a new setting? So that people who have explicitly set color.ui to \nauto (or one of the other values) will not be affected by the proposed \nnew logic.\n\nAnother new configuration item which I will call \"pager.carefully\" for \nnow is introduced that again can have multiple values of the form \nname=ENV. It might look like this:\n\n[pager]\n carefully = less=LESS LV=lv more=MORE\n\nAgain the default value of pager.carefully can be configured in a \nsystem-specific way (i.e. Makefile knob) at build time.\n\nIf color.ui is set to \"carefully\", then the logic is as follows:\n\na) Decide whether to output color the same way as for color.ui=auto\nb) Select the pager the same way as for color.ui=auto\nc) If (a) and (b) have selected a pager AND enabled color output then \ncheck to see if the selected pager appears in pager.carefully as one \nof the name values (perhaps a suffix match on the first whitespace- \nseparated word of the selected pager value).\nd) If the selected pager does not appear in pager.carefully, disable \ncolor output.\ne) If the selected pager appears in pager.carefully, but the ENV \nassociated with it does NOT appear in pager.env, disable color output.\nf) If the pager appears in pager.carefully, but the ENV associated \nwith it is already set, disable color output.\ng) Otherwise, go ahead with color output as the change in Part 1 above \nwill properly set the pager's options to enable it.\n\nThis has the following advantages:\n\n1. No intimate pager knowledge.\n2. Color output will be selected on supported systems by default \nassuming the user has not set any pager-specific environment variables.\n3. This should prevent the user from ever seeing the ugly ESC[33 and \nESC[m sequences with the default settings.\n4. A user who has color.ui=auto set is not affected by this change.\n5. Support for additional color pagers is easily added by tweaking the \nMakefile.\n\nIt has the following disadvantage:\n\n1. A user who has a pager-specific environment variable set for the \ndefault pager will not get color by default even if color would be \nsupported without first setting color.ui=auto or using an equivalent. \nEssentially they will get the pre-1.8.4 behavior.\n\n--Kyle\n"}]}