{"thread":{"id":"42982","subject":"[PATCH 0/2] add PAGER_ENV to build and core.pagerEnv to config","startedAt":"2016-08-01T01:06:12Z","lastAt":"2016-08-04T17:53:36Z","messageCount":32,"participants":["Eric Wong","brian m. carlson","Jakub Narębski","Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"292696","messageId":"20160801010557.22191-1-e@80x24.org","threadId":"42982","inReplyTo":null,"subject":"[PATCH 0/2] add PAGER_ENV to build and core.pagerEnv to config","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-01T01:05:55Z","receivedAt":"2016-08-01T01:06:12Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This series allows us to more easily configure platform-specific\ndefaults at build-time and also allow users to override them\nat runtime via config.\n\nPrevious discussion in this thread:\n\n\thttps://public-inbox.org/git/52D87A79.6060600@rawbw.com/T/\n\nThe following changes since commit f8f7adce9fc50a11a764d57815602dcb818d1816:\n\n Sync with maint (2016-07-28 14:21:18 -0700)\n\nare available in the git repository at:\n\n git://bogomips.org/git-svn.git pager-env\n\nfor you to fetch changes up to 1563ef177f9c1ee990bb3547f16bd7568a17379a:\n\n pager: implement core.pagerEnv in config (2016-08-01 00:51:42 +0000)\n\n----------------------------------------------------------------\nEric Wong (1):\n pager: implement core.pagerEnv in config\n\nJunio C Hamano (1):\n pager: move pager-specific setup into the build\n\n Documentation/config.txt | 7 +++++++\n Makefile | 19 +++++++++++++++++--\n config.mak.uname | 1 +\n git-sh-setup.sh | 8 +++++---\n pager.c | 35 +++++++++++++++++++++++++++++++----\n t/t7006-pager.sh | 14 ++++++++++++++\n 6 files changed, 75 insertions(+), 9 deletions(-)\n"},{"id":"292697","messageId":"20160801010557.22191-2-e@80x24.org","threadId":"42982","inReplyTo":"20160801010557.22191-1-e@80x24.org","subject":"[PATCH 1/2] pager: move pager-specific setup into the build","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-01T01:05:56Z","receivedAt":"2016-08-01T01:06:16Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"From: Junio C Hamano \n\nAllowing PAGER_ENV to be set at build-time allows us to move\npager-specific knowledge out of our build. Currently, this\nallows us to set a better default for FreeBSD where more(1)\nis the same binary as less(1).\n\nThis also prepares us for introducing a run-time config knob to\noverride the build-time environment in the next commit.\n\nOriginally-from:\n https://public-inbox.org/git/xmqq61piw4yf.fsf@gitster.dls.corp.google.com/\n\nSigned-off-by: Eric Wong \n---\n Makefile | 19 +++++++++++++++++--\n config.mak.uname | 1 +\n git-sh-setup.sh | 8 +++++---\n pager.c | 32 ++++++++++++++++++++++++++++----\n 4 files changed, 51 insertions(+), 9 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6a13386..fe469a6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -370,6 +370,14 @@ all::\n # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.\n #\n # Define HAVE_GETDELIM if your system has the getdelim() function.\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=FRX LV=-c\n+#\n+# to say \"export LESS=FRX (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@@ -1500,6 +1508,10 @@ ifeq ($(PYTHON_PATH),)\n NO_PYTHON = NoThanks\n endif\n \n+ifndef PAGER_ENV\n+PAGER_ENV = LESS=FRX LV=-c\n+endif\n+\n QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir\n QUIET_SUBDIR1 =\n \n@@ -1579,6 +1591,7 @@ 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 # We must filter out any object files from $(GITLIBS),\n # as it is typically used like:\n@@ -1591,7 +1604,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\\'',$(PERLLIB_EXTRA))\n LIBS = $(filter-out %.o, $(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\n@@ -1753,7 +1766,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):$(SANE_TEXT_GREP)\n+\t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)\n define cmd_munge_script\n $(RM) $@ $@+ && \\\n sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n@@ -1766,6 +1779,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \\\n -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \\\n+ -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \\\n $@.sh >$@+\n endef\n \n@@ -2173,6 +2187,7 @@ GIT-BUILD-OPTIONS: FORCE\n \t@echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@+\n \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\n \t@echo NO_UNIX_SOCKETS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_UNIX_SOCKETS)))'\\' >>$@+\n+\t@echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n ifdef TEST_OUTPUT_DIRECTORY\n \t@echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n endif\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 17fed2f..2484dfb 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)\n \tHAVE_PATHS_H = YesPlease\n \tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n \tHAVE_BSD_SYSCTL = YesPlease\n+\tPAGER_ENV = LESS=FRX 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 0c34aa6..0f5a56f 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -163,9 +163,11 @@ git_pager() {\n \telse\n \t\tGIT_PAGER=cat\n \tfi\n-\t: \"${LESS=-FRX}\"\n-\t: \"${LV=-c}\"\n-\texport LESS LV\n+\tfor vardef in @@PAGER_ENV@@\n+\tdo\n+\t\tvar=${vardef%%=*}\n+\t\teval \": \\${$vardef} && export $var\"\n+\tdone\n \n \teval \"$GIT_PAGER\" '\"$@\"'\n }\ndiff --git a/pager.c b/pager.c\nindex 4bc0481..2f2cadc 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -63,14 +63,38 @@ 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 prepare_pager_args(struct child_process *pager_process, const char *pager)\n {\n \targv_array_push(&pager_process->args, pager);\n \tpager_process->use_shell = 1;\n-\tif (!getenv(\"LESS\"))\n-\t\targv_array_push(&pager_process->env_array, \"LESS=FRX\");\n-\tif (!getenv(\"LV\"))\n-\t\targv_array_push(&pager_process->env_array, \"LV=-c\");\n+\tsetup_pager_env(&pager_process->env_array);\n }\n \n void setup_pager(void)\n-- \nEW\n\n"},{"id":"292698","messageId":"20160801010557.22191-3-e@80x24.org","threadId":"42982","inReplyTo":"20160801010557.22191-1-e@80x24.org","subject":"[PATCH 2/2] pager: implement core.pagerEnv in config","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-01T01:05:57Z","receivedAt":"2016-08-01T01:06:21Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This allows overriding the build-time PAGER_ENV variable\nat run-time.\n\nInspired by part 1 of an idea from Kyle J. McKay at:\nhttps://public-inbox.org/git/62DB6DEF-8B39-4481-BA06-245BF45233E5@gmail.com/\n\nSigned-off-by: Eric Wong \n---\n Documentation/config.txt | 7 +++++++\n pager.c | 5 ++++-\n t/t7006-pager.sh | 14 ++++++++++++++\n 3 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 8b1aee4..6c20269 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -714,6 +714,13 @@ Likewise, when the `LV` environment variable is unset, Git sets it\n to `-c`. You can override this setting by exporting `LV` with\n another value or setting `core.pager` to `lv +c`.\n \n+core.pagerEnv::\n+\tEnvironment for running `core.pager`.\n++\n+Defaults to the value set at build, usually `LESS=FRX LV=-c`.\n+On platforms where `more` and `less` are the same binary,\n+`LESS=FRX LV=-c MORE=FRX` is appropriate.\n+\n core.whitespace::\n \tA comma separated list of common whitespace problems to\n \tnotice. 'git diff' will use `color.diff.whitespace` to\ndiff --git a/pager.c b/pager.c\nindex 2f2cadc..cc2df7c 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -68,7 +68,10 @@ const char *git_pager(int stdout_is_tty)\n \n static void setup_pager_env(struct argv_array *env)\n {\n-\tconst char *pager_env = stringify(PAGER_ENV);\n+\tconst char *pager_env;\n+\n+\tif (git_config_get_value(\"core.pagerenv\", &pager_env))\n+\t\tpager_env = stringify(PAGER_ENV);\n \n \twhile (*pager_env) {\n \t\tstruct strbuf buf = STRBUF_INIT;\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex e4fc5c8..0c482fc 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -456,4 +456,18 @@ test_expect_success 'command with underscores does not complain' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success TTY 'core.pagerEnv overrides build-time env' '\n+\t(\n+\t\tsane_unset LESS LV MORE &&\n+\t\tgit config core.pagerEnv MORE=-R &&\n+\t\tPAGER=\"env >pager-env.out; wc\" &&\n+\t\texport PAGER &&\n+\t\ttest_terminal git log\n+\t) &&\n+\tgit config --unset core.pagerEnv &&\n+\tgrep ^MORE=-R pager-env.out &&\n+\tgrep -v ^LESS= pager-env.out &&\n+\tgrep -v ^LV= pager-env.out\n+'\n+\n test_done\n-- \nEW\n\n"},{"id":"292700","messageId":"20160801014303.x5j3dqumcmrkyc76@vauxhall.crustytoothpaste.net","threadId":"42982","inReplyTo":"20160801010557.22191-2-e@80x24.org","subject":"Re: [PATCH 1/2] pager: move pager-specific setup into the build","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2016-08-01T01:43:03Z","receivedAt":"2016-08-01T01:43:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:\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\nSo it looks like this function splits on spaces but doesn't provide any\nescaping mechanism. Is there any case in which we want to accept\nenvironment variables containing whitespace? I ask this as someone that\nhas EDITOR set to \"gvim -f\" on occasion and seeing how tools sometimes\nhandle that poorly.\n\nEven without that, I think this series is probably an improvement over\nthe status quo.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"292702","messageId":"20160801070037.GA18261@starla","threadId":"42982","inReplyTo":"20160801014303.x5j3dqumcmrkyc76@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 1/2] pager: move pager-specific setup into the build","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-01T07:00:37Z","receivedAt":"2016-08-01T07:01:11Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"\"brian m. carlson\" wrote:\n> On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:\n> > +\t\twhile (*cp && *cp == ' ')\n> > +\t\t\tcp++;\n> \n> So it looks like this function splits on spaces but doesn't provide any\n> escaping mechanism. Is there any case in which we want to accept\n> environment variables containing whitespace? I ask this as someone that\n> has EDITOR set to \"gvim -f\" on occasion and seeing how tools sometimes\n> handle that poorly.\n\nYes, it's only split on spaces right now. While I don't think\nthere's any current case where spaces would be useful/desirable;\nI suppose a 3rd patch in this series could add support for using\nsplit_cmdline (from alias.c)...\n"},{"id":"292705","messageId":"d4dc2061-afb0-baa3-f54d-bccfdf7658a5@gmail.com","threadId":"42982","inReplyTo":"20160801070037.GA18261@starla","subject":"Re: [PATCH 1/2] pager: move pager-specific setup into the build","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-08-01T08:57:02Z","receivedAt":"2016-08-01T08:58:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 01.08.2016 o 09:00, Eric Wong pisze:\n> \"brian m. carlson\" wrote:\n>> On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:\n>>> +\t\twhile (*cp && *cp == ' ')\n>>> +\t\t\tcp++;\n>>\n>> So it looks like this function splits on spaces but doesn't provide any\n>> escaping mechanism. Is there any case in which we want to accept\n>> environment variables containing whitespace? I ask this as someone that\n>> has EDITOR set to \"gvim -f\" on occasion and seeing how tools sometimes\n>> handle that poorly.\n\nThis is to handle environment variables holding program options,\nwhich are usually (but possibly not often) using single character\noptions bundled together, that is, not using spaces.\n\nMoreover, it is about holding program options to pager.\n\n> \n> Yes, it's only split on spaces right now. While I don't think\n> there's any current case where spaces would be useful/desirable;\n> I suppose a 3rd patch in this series could add support for using\n> split_cmdline (from alias.c)...\n\nIs there any pager that needs spaces in options-set environment\nvariable? Does MORE allow option bundling?\n\n-- \nJakub Narębski\n\n"},{"id":"292719","messageId":"20160801104018.jpebaum2vqkgzyoq@vauxhall.crustytoothpaste.net","threadId":"42982","inReplyTo":"d4dc2061-afb0-baa3-f54d-bccfdf7658a5@gmail.com","subject":"Re: [PATCH 1/2] pager: move pager-specific setup into the build","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2016-08-01T10:40:19Z","receivedAt":"2016-08-01T10:41:48Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Aug 01, 2016 at 10:57:02AM +0200, Jakub Narębski wrote:\n> W dniu 01.08.2016 o 09:00, Eric Wong pisze:\n> > \"brian m. carlson\" wrote:\n> >> So it looks like this function splits on spaces but doesn't provide any\n> >> escaping mechanism. Is there any case in which we want to accept\n> >> environment variables containing whitespace? I ask this as someone that\n> >> has EDITOR set to \"gvim -f\" on occasion and seeing how tools sometimes\n> >> handle that poorly.\n> \n> This is to handle environment variables holding program options,\n> which are usually (but possibly not often) using single character\n> options bundled together, that is, not using spaces.\n> \n> Moreover, it is about holding program options to pager.\n\nI understand that. My point is that we should consider corner cases\nlike how we're going to handle spaces.\n\n> > Yes, it's only split on spaces right now. While I don't think\n> > there's any current case where spaces would be useful/desirable;\n> > I suppose a 3rd patch in this series could add support for using\n> > split_cmdline (from alias.c)...\n> \n> Is there any pager that needs spaces in options-set environment\n> variable? Does MORE allow option bundling?\n\nWe seem to accept GIT_PAGER=\"par | less\" and par definitely accepts\nspaces in its environment variables. That seems to be a corner case,\nthough, and I haven't seen par practically used in years.\n\nWe may also want to consider EXINIT for people who pipe to vi. Again,\nI'm not sure this is very common; most people would use an .exrc or\n.vimrc.\n\nI'd say if we can't come up with any better examples, I'd skip handling\nit for now. I'll try to come up with a patch to add it later.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"292756","messageId":"20160801172459.wz7wigmytxpfvs22@sigill.intra.peff.net","threadId":"42982","inReplyTo":"20160801014303.x5j3dqumcmrkyc76@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 1/2] pager: move pager-specific setup into the build","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-01T17:24:59Z","receivedAt":"2016-08-01T17:25:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2016 at 01:43:03AM +0000, brian m. carlson wrote:\n\n> On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:\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> So it looks like this function splits on spaces but doesn't provide any\n> escaping mechanism. Is there any case in which we want to accept\n> environment variables containing whitespace? I ask this as someone that\n> has EDITOR set to \"gvim -f\" on occasion and seeing how tools sometimes\n> handle that poorly.\n> \n> Even without that, I think this series is probably an improvement over\n> the status quo.\n\nI'm not too worried about spaces here. This is a resurrection of an old\ndiscussion, and in all that time, I think the only realistic suggestions\nfor built-in values have been pretty tame.\n\nIf this were used to parse arbitrary user-provided runtime values, I'd\nbe more concerned. But I'm not sure why we would need that. Your $EDITOR\nexample is arbitrary shell code, and we let the shell handle it (modulo\nsome efficiency shortcuts). Likewise, fancy runtime things should go in\nGIT_PAGER, where you can not only set options with spaces, but do fancy\nthings like pipes, shell functions, etc.\n\nThe use of stringify() here is funny to me; I think there is a cpp\ntokenizing step in the middle that will do things like gobble up\nwhitespace (but I'm not sure if it has other possible effects). I think\nour more usual method here would be to C-quote in the Makefile (with the\nequivalent of 's/\\\\/\\\\\\\\/g; s/\"/\\\\\"/g'), and then pass it to the\ncompiler as a string literal, like -DPAGER_ENV=\\\"$(PAGER_ENV_CQ_SQ\\\".\n\n-Peff\n"},{"id":"292762","messageId":"CACsJy8AEFFTnGw01mTBrZu8QnUWmQav7bhZUyw7o91ph7DUwaA@mail.gmail.com","threadId":"42982","inReplyTo":"20160801010557.22191-2-e@80x24.org","subject":"Re: [PATCH 1/2] pager: move pager-specific setup into the build","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-08-01T17:46:34Z","receivedAt":"2016-08-01T17:54:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong wrote:\n> From: Junio C Hamano \n>\n> Allowing PAGER_ENV to be set at build-time allows us to move\n> pager-specific knowledge out of our build. Currently, this\n> allows us to set a better default for FreeBSD where more(1)\n> is the same binary as less(1).\n\nNice. I was just too lazy to do something like this and \"export\nPAGER=less LESS=FRX\" then ignored it :-P\n\nSlightly off topic, but pagers like 'more' does not understand colors\neither. But color.ui = auto does not know what and prints color code\nanyway. It would be nice if we had some configuration to describe\n\"this pager can show colors, that pager does not\" so I don't have to\nmaintain separate .gitconfig files on two platforms.\n-- \nDuy\n"},{"id":"292763","messageId":"20160801172851.k2vtqwik2hkblr4e@sigill.intra.peff.net","threadId":"42982","inReplyTo":"20160801010557.22191-3-e@80x24.org","subject":"Re: [PATCH 2/2] pager: implement core.pagerEnv in config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-01T17:28:51Z","receivedAt":"2016-08-01T17:57:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2016 at 01:05:57AM +0000, Eric Wong wrote:\n\n> This allows overriding the build-time PAGER_ENV variable\n> at run-time.\n> \n> Inspired by part 1 of an idea from Kyle J. McKay at:\n> https://public-inbox.org/git/62DB6DEF-8B39-4481-BA06-245BF45233E5@gmail.com/\n\nThis commit message is missing the \"why\" (I tried to get it from the\nreferenced email, but I am still confused).\n\nWhat does this buy you over:\n\n GIT_PAGER='less -whatever-options-you-like'\n\n? Sure, you have to say \"less\" there and not just \"if we happen to be\nusing less, use these options with it\". But that distinction is\nimportant for a build-time default, not for a run-time one. And by\npointing people to GIT_PAGER, they can do a lot _more_ than they can\nwith PAGER_ENV, including the full power of the shell (brian gave an\nexample of \"par | less\" earlier; I use \"diff-highlight | less\").\n\n-Peff\n"},{"id":"292765","messageId":"20160801175216.payv2zrc3mkvkbmz@sigill.intra.peff.net","threadId":"42982","inReplyTo":"CACsJy8AEFFTnGw01mTBrZu8QnUWmQav7bhZUyw7o91ph7DUwaA@mail.gmail.com","subject":"Re: [PATCH 1/2] pager: move pager-specific setup into the build","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-01T17:52:17Z","receivedAt":"2016-08-01T17:59:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2016 at 07:46:34PM +0200, Duy Nguyen wrote:\n\n> On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong wrote:\n> > From: Junio C Hamano \n> >\n> > Allowing PAGER_ENV to be set at build-time allows us to move\n> > pager-specific knowledge out of our build. Currently, this\n> > allows us to set a better default for FreeBSD where more(1)\n> > is the same binary as less(1).\n> \n> Nice. I was just too lazy to do something like this and \"export\n> PAGER=less LESS=FRX\" then ignored it :-P\n> \n> Slightly off topic, but pagers like 'more' does not understand colors\n> either. But color.ui = auto does not know what and prints color code\n> anyway. It would be nice if we had some configuration to describe\n> \"this pager can show colors, that pager does not\" so I don't have to\n> maintain separate .gitconfig files on two platforms.\n\nIf you are interested, I suggest you read the thread linked earlier:\n\n https://public-inbox.org/git/52D87A79.6060600%40rawbw.com/T/#u\n\nwhich discusses this and other issues. But basically, I think you cannot\nreally solve this without getting intimate with each pager (which people\nseemed not to want to do).\n\n-Peff\n"},{"id":"292767","messageId":"20160801180707.rw6drvk5dsgy6joi@sigill.intra.peff.net","threadId":"42982","inReplyTo":"CACsJy8CLNtbAvRxVtX_9qZ0cahce7cFfk=wh78Wm99Qf5B8HRw@mail.gmail.com","subject":"Re: [PATCH 1/2] pager: move pager-specific setup into the build","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-01T18:07:07Z","receivedAt":"2016-08-01T18:07:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2016 at 08:01:13PM +0200, Duy Nguyen wrote:\n\n> > If you are interested, I suggest you read the thread linked earlier:\n> >\n> > https://public-inbox.org/git/52D87A79.6060600%40rawbw.com/T/#u\n> >\n> > which discusses this and other issues. But basically, I think you cannot\n> > really solve this without getting intimate with each pager (which people\n> > seemed not to want to do).\n> \n> Cooking pager specifics in git does sound bad. But it does not have to\n> be that way. What if we delegate the decision whether to color or not\n> to a script (e.g. by setting color.ui= \"script \")?\n> The script has all the info (env variables, uname, user preference...)\n> and can make a better decision than 'is stdout a tty?'. It's not about\n> out of the box experience, more towards customization (without\n> fragmenting .gitconfig files too much).\n\nIt sounds like we are solving two separate problems.\n\nI was mostly concerned with the out-of-the-box experience. E.g., you\nbuild git and run \"git log\" and it prints out gibberish, either because\nof your $PAGER or your $LESS settings (which you might have set years\nago).\n\nFor more advanced usage like yours, I'd shove any personal logic into a\nwrapper around your pager script. I think the particular decision you\nwant, though, is related to color.pager, which is outside that scope.\nI'm not sure exactly what your setup looks like, but I wonder if it\nwould be served by better config support (i.e., why do you need a script\nto dynamically look at your pager and see if color.pager should be\nturned on; can't you configure both at the same time?).\n\n-Peff\n"},{"id":"292770","messageId":"xmqq37mol5w6.fsf@gitster.mtv.corp.google.com","threadId":"42982","inReplyTo":"20160801172459.wz7wigmytxpfvs22@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] pager: move pager-specific setup into the build","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-01T18:07:37Z","receivedAt":"2016-08-01T18:15:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not too worried about spaces here. This is a resurrection of an old\n> discussion, and in all that time, I think the only realistic suggestions\n> for built-in values have been pretty tame.\n>\n> If this were used to parse arbitrary user-provided runtime values, I'd\n> be more concerned. But I'm not sure why we would need that. Your $EDITOR\n> example is arbitrary shell code, and we let the shell handle it (modulo\n> some efficiency shortcuts). Likewise, fancy runtime things should go in\n> GIT_PAGER, where you can not only set options with spaces, but do fancy\n> things like pipes, shell functions, etc.\n>\n> The use of stringify() here is funny to me; I think there is a cpp\n> tokenizing step in the middle that will do things like gobble up\n> whitespace (but I'm not sure if it has other possible effects). I think\n> our more usual method here would be to C-quote in the Makefile (with the\n> equivalent of 's/\\\\/\\\\\\\\/g; s/\"/\\\\\"/g'), and then pass it to the\n> compiler as a string literal, like -DPAGER_ENV=\\\"$(PAGER_ENV_CQ_SQ\\\".\n\nAll sensible arguments, including the rationale to reject 2/2.\n\nThanks.\n"},{"id":"292777","messageId":"CACsJy8CLNtbAvRxVtX_9qZ0cahce7cFfk=wh78Wm99Qf5B8HRw@mail.gmail.com","threadId":"42982","inReplyTo":"20160801175216.payv2zrc3mkvkbmz@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] pager: move pager-specific setup into the build","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-08-01T18:01:13Z","receivedAt":"2016-08-01T19:03:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Aug 1, 2016 at 7:52 PM, Jeff King wrote:\n> On Mon, Aug 01, 2016 at 07:46:34PM +0200, Duy Nguyen wrote:\n>\n>> On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong wrote:\n>> > From: Junio C Hamano \n>> >\n>> > Allowing PAGER_ENV to be set at build-time allows us to move\n>> > pager-specific knowledge out of our build. Currently, this\n>> > allows us to set a better default for FreeBSD where more(1)\n>> > is the same binary as less(1).\n>>\n>> Nice. I was just too lazy to do something like this and \"export\n>> PAGER=less LESS=FRX\" then ignored it :-P\n>>\n>> Slightly off topic, but pagers like 'more' does not understand colors\n>> either. But color.ui = auto does not know what and prints color code\n>> anyway. It would be nice if we had some configuration to describe\n>> \"this pager can show colors, that pager does not\" so I don't have to\n>> maintain separate .gitconfig files on two platforms.\n>\n> If you are interested, I suggest you read the thread linked earlier:\n>\n> https://public-inbox.org/git/52D87A79.6060600%40rawbw.com/T/#u\n>\n> which discusses this and other issues. But basically, I think you cannot\n> really solve this without getting intimate with each pager (which people\n> seemed not to want to do).\n\nCooking pager specifics in git does sound bad. But it does not have to\nbe that way. What if we delegate the decision whether to color or not\nto a script (e.g. by setting color.ui= \"script \")?\nThe script has all the info (env variables, uname, user preference...)\nand can make a better decision than 'is stdout a tty?'. It's not about\nout of the box experience, more towards customization (without\nfragmenting .gitconfig files too much).\n-- \nDuy\n"},{"id":"292804","messageId":"20160801214937.4752-1-e@80x24.org","threadId":"42982","inReplyTo":"20160801010557.22191-1-e@80x24.org","subject":"[PATCH 0/1 v2] add PAGER_ENV to build","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-01T21:49:36Z","receivedAt":"2016-08-01T22:00:35Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Changes from v1:\n\n* dropped stringify macro in favor for quoting in Makefile\n (diff below)\n I'm not sure I like this change, and might be inclined to\n go in the opposite direction of using the stringify macro\n more widely to simplify the Makefile; but that is a separate\n topic.\n\n* dropped 2/2, I don't have a good rationale for it, either,\n other than \"it seemed easy\" after 1/2 :>\n\nThe following changes since commit f8f7adce9fc50a11a764d57815602dcb818d1816:\n\n Sync with maint (2016-07-28 14:21:18 -0700)\n\nare available in the git repository at:\n\n git://bogomips.org/git-svn.git pager-env-v2\n\nfor you to fetch changes up to d3aed319c9abac006060bc81e865c93ff8363066:\n\n pager: move pager-specific setup into the build (2016-08-01 21:46:25 +0000)\n\n----------------------------------------------------------------\nJunio C Hamano (1):\n pager: move pager-specific setup into the build\n\n Makefile | 20 +++++++++++++++++++-\n config.mak.uname | 1 +\n git-sh-setup.sh | 8 +++++---\n pager.c | 29 +++++++++++++++++++++++++----\n 4 files changed, 50 insertions(+), 8 deletions(-)\n\ninterdiff from 1/1 v1:\n\ndiff --git a/Makefile b/Makefile\nindex fe469a6..0b36b5e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1591,7 +1591,6 @@ 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 # We must filter out any object files from $(GITLIBS),\n # as it is typically used like:\n@@ -1604,7 +1603,7 @@ PAGER_ENV_SQ = $(subst ','\\'',$(PAGER_ENV))\n LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)\n \n BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \\\n-\t$(COMPAT_CFLAGS) -DPAGER_ENV='$(PAGER_ENV_SQ)'\n+\t$(COMPAT_CFLAGS)\n LIB_OBJS += $(COMPAT_OBJS)\n \n # Quote for C\n@@ -1642,6 +1641,10 @@ ifdef DEFAULT_HELP_FORMAT\n BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='\"$(DEFAULT_HELP_FORMAT)\"'\n endif\n \n+PAGER_ENV_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(PAGER_ENV)))\"\n+PAGER_ENV_CQ_SQ = $(subst ','\\'',$(PAGER_ENV_CQ))\n+BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'\n+\n ALL_CFLAGS += $(BASIC_CFLAGS)\n ALL_LDFLAGS += $(BASIC_LDFLAGS)\n \ndiff --git a/pager.c b/pager.c\nindex 2f2cadc..cd1ac54 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -63,12 +63,9 @@ 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+\tconst char *pager_env = PAGER_ENV;\n \n \twhile (*pager_env) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-- \nEW\n"},{"id":"292805","messageId":"20160801214937.4752-2-e@80x24.org","threadId":"42982","inReplyTo":"20160801214937.4752-1-e@80x24.org","subject":"[PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-01T21:49:37Z","receivedAt":"2016-08-01T22:01:02Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"From: Junio C Hamano \n\nAllowing PAGER_ENV to be set at build-time allows us to move\npager-specific knowledge out of our build. Currently, this\nallows us to set a better default for FreeBSD where more(1)\nis the same binary as less(1).\n\nThis also prepares us for introducing a run-time config knob to\noverride the build-time environment in the next commit.\n\nOriginally-from:\n https://public-inbox.org/git/xmqq61piw4yf.fsf@gitster.dls.corp.google.com/\n\nSigned-off-by: Eric Wong \n---\n Makefile | 20 +++++++++++++++++++-\n config.mak.uname | 1 +\n git-sh-setup.sh | 8 +++++---\n pager.c | 29 +++++++++++++++++++++++++----\n 4 files changed, 50 insertions(+), 8 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6a13386..0b36b5e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -370,6 +370,14 @@ all::\n # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.\n #\n # Define HAVE_GETDELIM if your system has the getdelim() function.\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=FRX LV=-c\n+#\n+# to say \"export LESS=FRX (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@@ -1500,6 +1508,10 @@ ifeq ($(PYTHON_PATH),)\n NO_PYTHON = NoThanks\n endif\n \n+ifndef PAGER_ENV\n+PAGER_ENV = LESS=FRX LV=-c\n+endif\n+\n QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir\n QUIET_SUBDIR1 =\n \n@@ -1629,6 +1641,10 @@ ifdef DEFAULT_HELP_FORMAT\n BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='\"$(DEFAULT_HELP_FORMAT)\"'\n endif\n \n+PAGER_ENV_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(PAGER_ENV)))\"\n+PAGER_ENV_CQ_SQ = $(subst ','\\'',$(PAGER_ENV_CQ))\n+BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'\n+\n ALL_CFLAGS += $(BASIC_CFLAGS)\n ALL_LDFLAGS += $(BASIC_LDFLAGS)\n \n@@ -1753,7 +1769,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):$(SANE_TEXT_GREP)\n+\t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)\n define cmd_munge_script\n $(RM) $@ $@+ && \\\n sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n@@ -1766,6 +1782,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \\\n -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \\\n+ -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \\\n $@.sh >$@+\n endef\n \n@@ -2173,6 +2190,7 @@ GIT-BUILD-OPTIONS: FORCE\n \t@echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@+\n \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\n \t@echo NO_UNIX_SOCKETS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_UNIX_SOCKETS)))'\\' >>$@+\n+\t@echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n ifdef TEST_OUTPUT_DIRECTORY\n \t@echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n endif\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 17fed2f..2484dfb 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)\n \tHAVE_PATHS_H = YesPlease\n \tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n \tHAVE_BSD_SYSCTL = YesPlease\n+\tPAGER_ENV = LESS=FRX 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 0c34aa6..0f5a56f 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -163,9 +163,11 @@ git_pager() {\n \telse\n \t\tGIT_PAGER=cat\n \tfi\n-\t: \"${LESS=-FRX}\"\n-\t: \"${LV=-c}\"\n-\texport LESS LV\n+\tfor vardef in @@PAGER_ENV@@\n+\tdo\n+\t\tvar=${vardef%%=*}\n+\t\teval \": \\${$vardef} && export $var\"\n+\tdone\n \n \teval \"$GIT_PAGER\" '\"$@\"'\n }\ndiff --git a/pager.c b/pager.c\nindex 4bc0481..cd1ac54 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -63,14 +63,35 @@ const char *git_pager(int stdout_is_tty)\n \treturn pager;\n }\n \n+static void setup_pager_env(struct argv_array *env)\n+{\n+\tconst char *pager_env = 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 prepare_pager_args(struct child_process *pager_process, const char *pager)\n {\n \targv_array_push(&pager_process->args, pager);\n \tpager_process->use_shell = 1;\n-\tif (!getenv(\"LESS\"))\n-\t\targv_array_push(&pager_process->env_array, \"LESS=FRX\");\n-\tif (!getenv(\"LV\"))\n-\t\targv_array_push(&pager_process->env_array, \"LV=-c\");\n+\tsetup_pager_env(&pager_process->env_array);\n }\n \n void setup_pager(void)\n-- \nEW\n\n"},{"id":"292808","messageId":"20160801215904.5oym63j3ewqyqugx@sigill.intra.peff.net","threadId":"42982","inReplyTo":"20160801214937.4752-1-e@80x24.org","subject":"Re: [PATCH 0/1 v2] add PAGER_ENV to build","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-01T21:59:05Z","receivedAt":"2016-08-01T22:05:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2016 at 09:49:36PM +0000, Eric Wong wrote:\n\n> Changes from v1:\n> \n> * dropped stringify macro in favor for quoting in Makefile\n> (diff below)\n> I'm not sure I like this change, and might be inclined to\n> go in the opposite direction of using the stringify macro\n> more widely to simplify the Makefile; but that is a separate\n> topic.\n\nI think that's a dangerous direction. Try this:\n\n-- >8 --\ncat >foo.c <<\\EOF\n#include \n\n#define stringify_(x) #x\n#define stringify(x) stringify_(x)\n\nint main(void)\n{\n\tprintf(\"%s\", stringify(FOO));\n\treturn 0;\n}\nEOF\n\nwhile read -r input; do\n\tgcc -Wall -Werror -DFOO=\"$input\" foo.c\n\t./a.out\ndone\n-- 8< --\n\nand then try input like:\n\n this has a lot of spaces\n this has a \\backslash\n\nYou should see:\n\n this has a lot of spaces\n this has aackslash\n\nI'll grant that backslashes and runs of whitespace are not things we'd\nexpect to find in most of our build-time config, but it still seems like\na bad direction to go (and actually, I wouldn't be surprised if\nbackslashes do end up in some of our build-time variables on Windows).\n\n-Peff\n"},{"id":"292814","messageId":"20160801234608.rsrrsnzywt5k7uzl@sigill.intra.peff.net","threadId":"42982","inReplyTo":"xmqq1t28gkhf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-01T23:46:08Z","receivedAt":"2016-08-01T23:46:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2016 at 04:03:40PM -0700, Junio C Hamano wrote:\n\n> Eric Wong writes:\n> \n> > From: Junio C Hamano \n> >\n> > Allowing PAGER_ENV to be set at build-time allows us to move\n> > pager-specific knowledge out of our build. Currently, this\n> > allows us to set a better default for FreeBSD where more(1)\n> > is the same binary as less(1).\n> \n> Thanks for resurrecting, but I am not sure what \"a better default\"\n> is from the above description and with the patch. Even though a\n> naive reading of the above (i.e. \"less\" and \"more\" are the same)\n> makes me expect that the patch will give the same set of default\n> environment settings to those on FreeBSD, you give LESS=FRX and\n> MORE=-R, i.e. they are configured differently.\n\nI wondered about this, too. They are the same binary, but calling less\nas \"more\" (or setting LESS_IS_MORE) causes less to behave \"like more\".\nLooking at the manpage, none the usual FRX options is affected. So in\ntheory we could just set MORE=FRX on FreeBSD.\n\nThat would be bad a idea in general, as other non-less implementations\nof more might get confused. \"more\" is in POSIX, and so is $MORE (and it\ndoes not understand any of our options).\n\nYou could also make the knob \"MORE_IS_LESS\" or something, and just\nblindly copy $LESS to $MORE. That's a bad idea, though, if somebody does\nstick one of the incompatible flags in the build options.\n\n-Peff\n"},{"id":"292815","messageId":"20160801235651.GA7140@starla","threadId":"42982","inReplyTo":"xmqq1t28gkhf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-01T23:56:51Z","receivedAt":"2016-08-01T23:58:23Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Eric Wong writes:\n> > Allowing PAGER_ENV to be set at build-time allows us to move\n> > pager-specific knowledge out of our build. Currently, this\n> > allows us to set a better default for FreeBSD where more(1)\n> > is the same binary as less(1).\n> \n> Thanks for resurrecting, but I am not sure what \"a better default\"\n> is from the above description and with the patch. Even though a\n> naive reading of the above (i.e. \"less\" and \"more\" are the same)\n> makes me expect that the patch will give the same set of default\n> environment settings to those on FreeBSD, you give LESS=FRX and\n> MORE=-R, i.e. they are configured differently.\n\nPerhaps s/better/platform-appropriate/ ?\n\nI just copied your original patch in setting MORE=-R\n(but removed 'S' from LESS).\n\nSo v3 will be MORE=FRX, as less was added:\n\n commit 98170c0c3ba86eb1cc975e7848d075bf2abc1ed0\n Author: ps \n Date: Mon May 22 10:00:00 2000 +0000\n\n\tbmake glue for less.\n\nand more was nuked:\n\n commit cde9059fa3e4dc7e259c3864d7536252a5c580a0\n Author: ps \n Date: Mon May 29 13:31:51 2000 +0000\n\n\tNuke more from the repository.\n\nAnd \"git branch -r --contains\" on both of those commits says\nthey showed up in the 5.0 release. However, further\ninvestigation says more was even gone by the 4.1.0 release\n\n git show origin/release/4.1.0:usr.bin/more # non-existent tree\n git show origin/release/4.0.0:usr.bin/more # tree still exists\n\nBut, \"git show origin/release/4.0.0:usr.bin/more/option.c\"\nreveals more from those days wouldn't handle -R anyways,\nand hopefully nobody is still running 4.0.0...\n\nref: git://github.com/freebsd/freebsd.git\n\n> > This also prepares us for introducing a run-time config knob to\n> > override the build-time environment in the next commit.\n> \n> This is now gone, judging from 1/1 on the subject line being not\n> 1/2, right?\n\nOops, yes :x\n\n> > Originally-from:\n> > https://public-inbox.org/git/xmqq61piw4yf.fsf@gitster.dls.corp.google.com/\n"},{"id":"292816","messageId":"xmqq1t28gkhf.fsf@gitster.mtv.corp.google.com","threadId":"42982","inReplyTo":"20160801214937.4752-2-e@80x24.org","subject":"Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-01T23:03:40Z","receivedAt":"2016-08-02T00:00:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> From: Junio C Hamano \n>\n> Allowing PAGER_ENV to be set at build-time allows us to move\n> pager-specific knowledge out of our build. Currently, this\n> allows us to set a better default for FreeBSD where more(1)\n> is the same binary as less(1).\n\nThanks for resurrecting, but I am not sure what \"a better default\"\nis from the above description and with the patch. Even though a\nnaive reading of the above (i.e. \"less\" and \"more\" are the same)\nmakes me expect that the patch will give the same set of default\nenvironment settings to those on FreeBSD, you give LESS=FRX and\nMORE=-R, i.e. they are configured differently.\n\n> This also prepares us for introducing a run-time config knob to\n> override the build-time environment in the next commit.\n\nThis is now gone, judging from 1/1 on the subject line being not\n1/2, right?\n\n>\n> Originally-from:\n> https://public-inbox.org/git/xmqq61piw4yf.fsf@gitster.dls.corp.google.com/\n>\n> Signed-off-by: Eric Wong \n> ---\n> Makefile | 20 +++++++++++++++++++-\n> config.mak.uname | 1 +\n> git-sh-setup.sh | 8 +++++---\n> pager.c | 29 +++++++++++++++++++++++++----\n> 4 files changed, 50 insertions(+), 8 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 6a13386..0b36b5e 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -370,6 +370,14 @@ all::\n> # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.\n> #\n> # Define HAVE_GETDELIM if your system has the getdelim() function.\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=FRX LV=-c\n> +#\n> +# to say \"export LESS=FRX (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> @@ -1500,6 +1508,10 @@ ifeq ($(PYTHON_PATH),)\n> NO_PYTHON = NoThanks\n> endif\n> \n> +ifndef PAGER_ENV\n> +PAGER_ENV = LESS=FRX LV=-c\n> +endif\n> +\n> QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir\n> QUIET_SUBDIR1 =\n> \n> @@ -1629,6 +1641,10 @@ ifdef DEFAULT_HELP_FORMAT\n> BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='\"$(DEFAULT_HELP_FORMAT)\"'\n> endif\n> \n> +PAGER_ENV_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(PAGER_ENV)))\"\n> +PAGER_ENV_CQ_SQ = $(subst ','\\'',$(PAGER_ENV_CQ))\n> +BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'\n> +\n> ALL_CFLAGS += $(BASIC_CFLAGS)\n> ALL_LDFLAGS += $(BASIC_LDFLAGS)\n> \n> @@ -1753,7 +1769,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):$(SANE_TEXT_GREP)\n> +\t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)\n> define cmd_munge_script\n> $(RM) $@ $@+ && \\\n> sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n> @@ -1766,6 +1782,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n> -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \\\n> -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n> -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \\\n> + -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \\\n> $@.sh >$@+\n> endef\n> \n> @@ -2173,6 +2190,7 @@ GIT-BUILD-OPTIONS: FORCE\n> \t@echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@+\n> \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\n> \t@echo NO_UNIX_SOCKETS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_UNIX_SOCKETS)))'\\' >>$@+\n> +\t@echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n> ifdef TEST_OUTPUT_DIRECTORY\n> \t@echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n> endif\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 17fed2f..2484dfb 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)\n> \tHAVE_PATHS_H = YesPlease\n> \tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n> \tHAVE_BSD_SYSCTL = YesPlease\n> +\tPAGER_ENV = LESS=FRX LV=-c MORE=-R\n> endif\n> ifeq ($(uname_S),OpenBSD)\n> \tNO_STRCASESTR = YesPlease\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index 0c34aa6..0f5a56f 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -163,9 +163,11 @@ git_pager() {\n> \telse\n> \t\tGIT_PAGER=cat\n> \tfi\n> -\t: \"${LESS=-FRX}\"\n> -\t: \"${LV=-c}\"\n> -\texport LESS LV\n> +\tfor vardef in @@PAGER_ENV@@\n> +\tdo\n> +\t\tvar=${vardef%%=*}\n> +\t\teval \": \\${$vardef} && export $var\"\n> +\tdone\n> \n> \teval \"$GIT_PAGER\" '\"$@\"'\n> }\n> diff --git a/pager.c b/pager.c\n> index 4bc0481..cd1ac54 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -63,14 +63,35 @@ const char *git_pager(int stdout_is_tty)\n> \treturn pager;\n> }\n> \n> +static void setup_pager_env(struct argv_array *env)\n> +{\n> +\tconst char *pager_env = 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 prepare_pager_args(struct child_process *pager_process, const char *pager)\n> {\n> \targv_array_push(&pager_process->args, pager);\n> \tpager_process->use_shell = 1;\n> -\tif (!getenv(\"LESS\"))\n> -\t\targv_array_push(&pager_process->env_array, \"LESS=FRX\");\n> -\tif (!getenv(\"LV\"))\n> -\t\targv_array_push(&pager_process->env_array, \"LV=-c\");\n> +\tsetup_pager_env(&pager_process->env_array);\n> }\n> \n> void setup_pager(void)\n"},{"id":"292866","messageId":"xmqqeg66dgb5.fsf@gitster.mtv.corp.google.com","threadId":"42982","inReplyTo":"20160801234608.rsrrsnzywt5k7uzl@sigill.intra.peff.net","subject":"Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T21:14:22Z","receivedAt":"2016-08-02T21:14:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 01, 2016 at 04:03:40PM -0700, Junio C Hamano wrote:\n>\n>> Eric Wong writes:\n>> \n>> > From: Junio C Hamano \n>> >\n>> > Allowing PAGER_ENV to be set at build-time allows us to move\n>> > pager-specific knowledge out of our build. Currently, this\n>> > allows us to set a better default for FreeBSD where more(1)\n>> > is the same binary as less(1).\n>> \n>> Thanks for resurrecting, but I am not sure what \"a better default\"\n>> is from the above description and with the patch. Even though a\n>> naive reading of the above (i.e. \"less\" and \"more\" are the same)\n>> makes me expect that the patch will give the same set of default\n>> environment settings to those on FreeBSD, you give LESS=FRX and\n>> MORE=-R, i.e. they are configured differently.\n>\n> I wondered about this, too. They are the same binary, but calling less\n> as \"more\" (or setting LESS_IS_MORE) causes less to behave \"like more\".\n\nI guessed that, but if that is the case, \"more is the same binary\"\nis irrelevant. \"more\" behaves differently from \"less\" might be, but\nwhat \"less\" does is much less important than \"more needs this default\nsetting to work pleasantly\", which is what is missing.\n\nSo I'd say\n\n Allowing PAGER_ENV to be set at build-time allows us to move\n pager-specific knowledge out of our build. This allows us to\n set a better default for FreeBSD more(1), which misbehaves if\n MORE environment variable is left empty $in_such_and_such_way,\n by defaulting it to -R.\n\nwithout even mentioning anything about \"less\" may be a more\nunderstandable justification for a patch that sets MORE only on\nFreeBSD.\n\n\n"},{"id":"292867","messageId":"xmqqa8gudg8n.fsf@gitster.mtv.corp.google.com","threadId":"42982","inReplyTo":"20160801235651.GA7140@starla","subject":"Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T21:15:52Z","receivedAt":"2016-08-02T21:16:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> So v3 will be MORE=FRX, as less was added:\n>\n> commit 98170c0c3ba86eb1cc975e7848d075bf2abc1ed0\n> Author: ps \n> Date: Mon May 22 10:00:00 2000 +0000\n>\n> \tbmake glue for less.\n>\n> and more was nuked:\n>\n> commit cde9059fa3e4dc7e259c3864d7536252a5c580a0\n> Author: ps \n> Date: Mon May 29 13:31:51 2000 +0000\n>\n> \tNuke more from the repository.\n>\n> And \"git branch -r --contains\" on both of those commits says\n> they showed up in the 5.0 release. However, further\n> investigation says more was even gone by the 4.1.0 release\n>\n> git show origin/release/4.1.0:usr.bin/more # non-existent tree\n> git show origin/release/4.0.0:usr.bin/more # tree still exists\n>\n> But, \"git show origin/release/4.0.0:usr.bin/more/option.c\"\n> reveals more from those days wouldn't handle -R anyways,\n> and hopefully nobody is still running 4.0.0...\n\nOK. And they can always set $MORE on their own to work it around,\nor we can do a release-dependent tweak when somebody screams.\n\n"},{"id":"292917","messageId":"20160803161911.dxucq7f2pvnoovoc@sigill.intra.peff.net","threadId":"42982","inReplyTo":"20160801214937.4752-2-e@80x24.org","subject":"Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-03T16:19:11Z","receivedAt":"2016-08-03T17:05:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2016 at 09:49:37PM +0000, Eric Wong wrote:\n\n> +static void setup_pager_env(struct argv_array *env)\n> +{\n> +\tconst char *pager_env = 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\nargv_array handles its own allocation, so this leaks the detached\nstrbuf.\n\nYou'd want:\n\n argv_array_push(env, buf.buf);\n strbuf_release(&buf);\n\nor just:\n\n argv_array_pushf(env, \"%.*s\", (int)(cp - pager_env), pager_env);\n\nAlso:\n\n> +\t\tstrbuf_reset(&buf);\n\nshould this be strbuf_release()? If we didn't follow the conditional\nabove (because getenv() told us the variable was already set), then we\nwould not do do the detach/release there, and would finish the loop with\nmemory still allocated by \"buf\".\n\n-Peff\n"},{"id":"292996","messageId":"xmqq4m718tay.fsf@gitster.mtv.corp.google.com","threadId":"42982","inReplyTo":"20160803161911.dxucq7f2pvnoovoc@sigill.intra.peff.net","subject":"Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-03T20:57:09Z","receivedAt":"2016-08-03T21:01:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 01, 2016 at 09:49:37PM +0000, Eric Wong wrote:\n>\n> You'd want:\n>\n> argv_array_pushf(env, \"%.*s\", (int)(cp - pager_env), pager_env);\n>\n> Also:\n>\n>> +\t\tstrbuf_reset(&buf);\n>\n> should this be strbuf_release()? If we didn't follow the conditional\n> above (because getenv() told us the variable was already set), then we\n> would not do do the detach/release there, and would finish the loop with\n> memory still allocated by \"buf\".\n\nAll bugs are from my original, I think. Here is a proposed squash.\n\n * This shouldn't make much difference for @@PAGER_ENV@@, but not\n quoting the default assignment, i.e.\n\n \t: \"${VAR=VAL}\" && export VAR\n\n is bad manners. cf. 01247e02 (sh-setup: enclose setting of\n ${VAR=default} in double-quotes, 2016-06-19)\n\n * Again, this shouldn't make much difference for PAGER_ENV, but\n let's follow the \"reset per iteration, release at the end\"\n pattern to avoid repeated allocation.\n\n * Let's avoid a hand-rolled \"skip blanks\" loop and let strspn() do\n it.\n\n\n git-sh-setup.sh | 2 +-\n pager.c | 15 ++++++---------\n 2 files changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex b6b75e9..cda32d0 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -163,7 +163,7 @@ git_pager() {\n \tfor vardef in @@PAGER_ENV@@\n \tdo\n \t\tvar=${vardef%%=*}\n-\t\teval \": \\${$vardef} && export $var\"\n+\t\teval \": \\\"\\${$vardef}\\\" && export $var\"\n \tdone\n \n \teval \"$GIT_PAGER\" '\"$@\"'\ndiff --git a/pager.c b/pager.c\nindex cd1ac54..7199c31 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -66,25 +66,22 @@ const char *git_pager(int stdout_is_tty)\n static void setup_pager_env(struct argv_array *env)\n {\n \tconst char *pager_env = PAGER_ENV;\n+\tstruct strbuf buf = STRBUF_INIT;\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\tif (!getenv(buf.buf))\n+\t\t\targv_array_pushf(env, \"%.*s\",\n+\t\t\t\t\t (int)(cp - pager_env), pager_env);\n+\t\tpager_env = cp + strspn(cp, \" \");\n \t\tstrbuf_reset(&buf);\n-\t\twhile (*cp && *cp == ' ')\n-\t\t\tcp++;\n-\t\tpager_env = cp;\n \t}\n+\tstrbuf_release(&buf);\n }\n \n void prepare_pager_args(struct child_process *pager_process, const char *pager)\n"},{"id":"292997","messageId":"20160803210821.GA17510@whir","threadId":"42982","inReplyTo":"xmqq4m718tay.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-03T21:08:21Z","receivedAt":"2016-08-03T21:09:13Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> All bugs are from my original, I think. Here is a proposed squash.\n\nThanks, I'll take the git-sh-setup changes.\n\nI actually just rewrote setup_pager_env using split_cmdline and\neliminated all the scary (to me) pointer arithmetic and avoided\nstrbuf, too.\n\nUnfortunately, my Internet connection on that machine dropped\nbefore I could finish testing and pushing :<\n\nThis was also the machine running http://czquwvybam4bgbro.onion/git/ :<\n"},{"id":"292998","messageId":"xmqqziot7dv6.fsf@gitster.mtv.corp.google.com","threadId":"42982","inReplyTo":"20160803210821.GA17510@whir","subject":"Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-03T21:15:57Z","receivedAt":"2016-08-03T21:18:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Junio C Hamano wrote:\n>> All bugs are from my original, I think. Here is a proposed squash.\n>\n> Thanks, I'll take the git-sh-setup changes.\n>\n> I actually just rewrote setup_pager_env using split_cmdline and\n> eliminated all the scary (to me) pointer arithmetic and avoided\n> strbuf, too.\n\nI actually do not have much faith in split_cmdline() in that I\ncannot quite read and follow the flow of the logic in its\nimplementation, but it already is used widely so it must be OK, so I\nam fine if you use it as a black-box to make this code simpler ;-)\n\nI'll drop the squash then and replace it with your version when it\ncomes.\n\nThanks.\n"},{"id":"293003","messageId":"20160803210946.pjclbkz3qlsi2ezb@sigill.intra.peff.net","threadId":"42982","inReplyTo":"xmqq4m718tay.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-03T21:09:46Z","receivedAt":"2016-08-03T21:36:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 03, 2016 at 01:57:09PM -0700, Junio C Hamano wrote:\n\n> All bugs are from my original, I think. Here is a proposed squash.\n> \n> * This shouldn't make much difference for @@PAGER_ENV@@, but not\n> quoting the default assignment, i.e.\n> \n> \t: \"${VAR=VAL}\" && export VAR\n> \n> is bad manners. cf. 01247e02 (sh-setup: enclose setting of\n> ${VAR=default} in double-quotes, 2016-06-19)\n> \n> * Again, this shouldn't make much difference for PAGER_ENV, but\n> let's follow the \"reset per iteration, release at the end\"\n> pattern to avoid repeated allocation.\n> \n> * Let's avoid a hand-rolled \"skip blanks\" loop and let strspn() do\n> it.\n\nSounds good.\n\n> diff --git a/pager.c b/pager.c\n> index cd1ac54..7199c31 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -66,25 +66,22 @@ const char *git_pager(int stdout_is_tty)\n> static void setup_pager_env(struct argv_array *env)\n> {\n> \tconst char *pager_env = PAGER_ENV;\n> +\tstruct strbuf buf = STRBUF_INIT;\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\nI thought you'd need a strbuf_reset() here, but I guess it is handled by\nthe one at the end of the loop. That's OK because there are no other\nloop exits, though IMHO putting it at the top makes the reusable-strbuf\nidiom easier to understand.\n\n-Peff\n"},{"id":"293061","messageId":"20160804034301.GA31427@starla","threadId":"42982","inReplyTo":"xmqqziot7dv6.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v3] pager: move pager-specific setup into the build","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-04T03:43:01Z","receivedAt":"2016-08-04T03:43:10Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"From: Junio C Hamano \n\nAllowing PAGER_ENV to be set at build-time allows us to move\npager-specific knowledge out of our build. This allows us to\nset a better default for FreeBSD more(1), which misbehaves if\nMORE environment variable is left empty, but accepts the same\nvariables as less(1)\n\nOriginally-from:\n https://public-inbox.org/git/xmqq61piw4yf.fsf@gitster.dls.corp.google.com/\n\nSigned-off-by: Eric Wong \n---\n v3 changes (from v2, d3aed319c9abac006060bc81e865c93ff8363066)\n - Squashed git-sh-setup quoting fix from Junio\n - set MORE=FRX on FreeBSD to match LESS\n - simplify setup_pager_env to avoid leaks using split_cmdline\n\n The following changes since commit f8f7adce9fc50a11a764d57815602dcb818d1816:\n\n Sync with maint (2016-07-28 14:21:18 -0700)\n\n are available in the git repository at:\n\n git://bogomips.org/git-svn.git pager-env-v3\n\n for you to fetch changes up to 18c69fb16d866f42b53242142229de4801964d37:\n\n pager: move pager-specific setup into the build (2016-08-04 03:17:16 +0000)\n\n ----------------------------------------------------------------\n Junio C Hamano (1):\n pager: move pager-specific setup into the build\n\n Makefile | 20 +++++++++++++++++++-\n config.mak.uname | 1 +\n git-sh-setup.sh | 8 +++++---\n pager.c | 32 ++++++++++++++++++++++++++++----\n 4 files changed, 53 insertions(+), 8 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6a13386..0b36b5e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -370,6 +370,14 @@ all::\n # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.\n #\n # Define HAVE_GETDELIM if your system has the getdelim() function.\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=FRX LV=-c\n+#\n+# to say \"export LESS=FRX (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@@ -1500,6 +1508,10 @@ ifeq ($(PYTHON_PATH),)\n NO_PYTHON = NoThanks\n endif\n \n+ifndef PAGER_ENV\n+PAGER_ENV = LESS=FRX LV=-c\n+endif\n+\n QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir\n QUIET_SUBDIR1 =\n \n@@ -1629,6 +1641,10 @@ ifdef DEFAULT_HELP_FORMAT\n BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='\"$(DEFAULT_HELP_FORMAT)\"'\n endif\n \n+PAGER_ENV_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(PAGER_ENV)))\"\n+PAGER_ENV_CQ_SQ = $(subst ','\\'',$(PAGER_ENV_CQ))\n+BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'\n+\n ALL_CFLAGS += $(BASIC_CFLAGS)\n ALL_LDFLAGS += $(BASIC_LDFLAGS)\n \n@@ -1753,7 +1769,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):$(SANE_TEXT_GREP)\n+\t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)\n define cmd_munge_script\n $(RM) $@ $@+ && \\\n sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n@@ -1766,6 +1782,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \\\n -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \\\n+ -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \\\n $@.sh >$@+\n endef\n \n@@ -2173,6 +2190,7 @@ GIT-BUILD-OPTIONS: FORCE\n \t@echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@+\n \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\n \t@echo NO_UNIX_SOCKETS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_UNIX_SOCKETS)))'\\' >>$@+\n+\t@echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n ifdef TEST_OUTPUT_DIRECTORY\n \t@echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n endif\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 17fed2f..b232908 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)\n \tHAVE_PATHS_H = YesPlease\n \tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n \tHAVE_BSD_SYSCTL = YesPlease\n+\tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 0c34aa6..a8a4576 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -163,9 +163,11 @@ git_pager() {\n \telse\n \t\tGIT_PAGER=cat\n \tfi\n-\t: \"${LESS=-FRX}\"\n-\t: \"${LV=-c}\"\n-\texport LESS LV\n+\tfor vardef in @@PAGER_ENV@@\n+\tdo\n+\t\tvar=${vardef%%=*}\n+\t\teval \": \\\"\\${$vardef}\\\" && export $var\"\n+\tdone\n \n \teval \"$GIT_PAGER\" '\"$@\"'\n }\ndiff --git a/pager.c b/pager.c\nindex 4bc0481..6470b81 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -63,14 +63,38 @@ const char *git_pager(int stdout_is_tty)\n \treturn pager;\n }\n \n+static void setup_pager_env(struct argv_array *env)\n+{\n+\tconst char **argv;\n+\tint i;\n+\tchar *pager_env = xstrdup(PAGER_ENV);\n+\tint n = split_cmdline(pager_env, &argv);\n+\n+\tif (n < 0)\n+\t\tdie(\"malformed build-time PAGER_ENV: %s\",\n+\t\t\tsplit_cmdline_strerror(n));\n+\n+\tfor (i = 0; i < n; i++) {\n+\t\tchar *cp = strchr(argv[i], '=');\n+\n+\t\tif (!cp)\n+\t\t\tdie(\"malformed build-time PAGER_ENV\");\n+\n+\t\t*cp = '\\0';\n+\t\tif (!getenv(argv[i])) {\n+\t\t\t*cp = '=';\n+\t\t\targv_array_push(env, argv[i]);\n+\t\t}\n+\t}\n+\tfree(pager_env);\n+\tfree(argv);\n+}\n+\n void prepare_pager_args(struct child_process *pager_process, const char *pager)\n {\n \targv_array_push(&pager_process->args, pager);\n \tpager_process->use_shell = 1;\n-\tif (!getenv(\"LESS\"))\n-\t\targv_array_push(&pager_process->env_array, \"LESS=FRX\");\n-\tif (!getenv(\"LV\"))\n-\t\targv_array_push(&pager_process->env_array, \"LV=-c\");\n+\tsetup_pager_env(&pager_process->env_array);\n }\n \n void setup_pager(void)\n-- \nEW\n"},{"id":"293065","messageId":"20160804053405.ifjjryejgbwkkatt@sigill.intra.peff.net","threadId":"42982","inReplyTo":"20160804034301.GA31427@starla","subject":"Re: [PATCH v3] pager: move pager-specific setup into the build","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-04T05:34:05Z","receivedAt":"2016-08-04T05:34:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 04, 2016 at 03:43:01AM +0000, Eric Wong wrote:\n\n> +PAGER_ENV_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(PAGER_ENV)))\"\n> +PAGER_ENV_CQ_SQ = $(subst ','\\'',$(PAGER_ENV_CQ))\n> +BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'\n\nHere we set up CQ_SQ, but there is no PAGER_ENV_SQ.\n\nAnd then...\n\n> @@ -1753,7 +1769,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):$(SANE_TEXT_GREP)\n> +\t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)\n> define cmd_munge_script\n> $(RM) $@ $@+ && \\\n> sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n> @@ -1766,6 +1782,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n> -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \\\n> -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n> -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \\\n> + -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \\\n> $@.sh >$@+\n> endef\n\nHere we depend on writing PAGER_ENV_SQ, which will be blank (and\ngit-sh-setup is broken as a result).\n\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 17fed2f..b232908 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)\n> \tHAVE_PATHS_H = YesPlease\n> \tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n> \tHAVE_BSD_SYSCTL = YesPlease\n> +\tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n> endif\n\nIs it worth setting up PAGER_ENV's default values before including\nconfig.mak.*, and then using \"+=\" here? That avoids this line getting\nout of sync with the usual defaults.\n\n> +static void setup_pager_env(struct argv_array *env)\n> +{\n\nI know you said you don't like string parsing in C. Here is a patch (on\ntop of yours) that converts the parsing to shell, and generates a\npre-built array-of-struct (this is similar to the big series I posted\nlong ago, but just touching this one spot, not invading the whole\nMakefile). Feel free to ignore it as over-engineered, but I thought I'd\nthrow it out there in case it appeals.\n\n-- >8 --\ndiff --git a/.gitignore b/.gitignore\nindex 05cb58a..b0fd5ec 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -180,6 +180,7 @@\n /gitweb/static/gitweb.js\n /gitweb/static/gitweb.min.*\n /common-cmds.h\n+/pager-env.h\n *.tar.gz\n *.dsc\n *.deb\ndiff --git a/Makefile b/Makefile\nindex 0b36b5e..2e009cd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -680,6 +680,7 @@ XDIFF_LIB = xdiff/lib.a\n VCSSVN_LIB = vcs-svn/lib.a\n \n GENERATED_H += common-cmds.h\n+GENERATED_H += pager-env.h\n \n LIB_H = $(shell $(FIND) . \\\n \t-name .git -prune -o \\\n@@ -1641,9 +1642,7 @@ ifdef DEFAULT_HELP_FORMAT\n BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='\"$(DEFAULT_HELP_FORMAT)\"'\n endif\n \n-PAGER_ENV_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(PAGER_ENV)))\"\n-PAGER_ENV_CQ_SQ = $(subst ','\\'',$(PAGER_ENV_CQ))\n-BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'\n+PAGER_ENV_SQ = $(subst ','\\'',$(PAGER_ENV))\n \n ALL_CFLAGS += $(BASIC_CFLAGS)\n ALL_LDFLAGS += $(BASIC_LDFLAGS)\n@@ -1767,6 +1766,10 @@ common-cmds.h: generate-cmdlist.sh command-list.txt\n common-cmds.h: $(wildcard Documentation/git-*.txt)\n \t$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ && mv $@+ $@\n \n+pager-env.h: generate-pager-env.sh GIT-BUILD-OPTIONS\n+\t$(QUIET_GEN)$(SHELL_PATH) ./generate-pager-env.sh '$(PAGER_ENV_SQ)' >$@+ && \\\n+\tmv $@+ $@\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):$(SANE_TEXT_GREP):$(PAGER_ENV)\ndiff --git a/generate-pager-env.sh b/generate-pager-env.sh\nnew file mode 100644\nindex 0000000..5b67b59\n--- /dev/null\n+++ b/generate-pager-env.sh\n@@ -0,0 +1,28 @@\n+#!/bin/sh\n+\n+c_quote () {\n+\tprintf '%s' \"$@\" | sed 's/\\\\/\\\\\\\\/g; s/\"/\\\\\"/g;'\n+}\n+\n+cat <<\\EOF &&\n+#ifndef PAGER_ENV_H\n+#define PAGER_ENV_H\n+\n+static struct pager_env {\n+\tconst char *key;\n+\tconst char *value;\n+} pager_env[] = {\n+EOF\n+\n+# $* does whitespace splitting\n+for i in $*; do\n+\tkey=${i%%=*}\n+\tvalue=${i#*=}\n+\tprintf '\\t{ \"%s\", \"%s\" },\\n' \"$(c_quote \"$key\")\" \"$(c_quote \"$value\")\"\n+done &&\n+\n+cat <<\\EOF\n+};\n+\n+#endif /* PAGER_ENV */\n+EOF\ndiff --git a/pager.c b/pager.c\nindex 6470b81..03c1a4a 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 \"pager-env.h\"\n \n #ifndef DEFAULT_PAGER\n #define DEFAULT_PAGER \"less\"\n@@ -65,29 +66,12 @@ const char *git_pager(int stdout_is_tty)\n \n static void setup_pager_env(struct argv_array *env)\n {\n-\tconst char **argv;\n \tint i;\n-\tchar *pager_env = xstrdup(PAGER_ENV);\n-\tint n = split_cmdline(pager_env, &argv);\n-\n-\tif (n < 0)\n-\t\tdie(\"malformed build-time PAGER_ENV: %s\",\n-\t\t\tsplit_cmdline_strerror(n));\n-\n-\tfor (i = 0; i < n; i++) {\n-\t\tchar *cp = strchr(argv[i], '=');\n-\n-\t\tif (!cp)\n-\t\t\tdie(\"malformed build-time PAGER_ENV\");\n-\n-\t\t*cp = '\\0';\n-\t\tif (!getenv(argv[i])) {\n-\t\t\t*cp = '=';\n-\t\t\targv_array_push(env, argv[i]);\n-\t\t}\n+\tfor (i = 0; i < ARRAY_SIZE(pager_env); i++) {\n+\t\tstruct pager_env *p = &pager_env[i];\n+\t\tif (!getenv(p->key))\n+\t\t\targv_array_pushf(env, \"%s=%s\", p->key, p->value);\n \t}\n-\tfree(pager_env);\n-\tfree(argv);\n }\n \n void prepare_pager_args(struct child_process *pager_process, const char *pager)\n"},{"id":"293075","messageId":"20160804113410.GA13908@starla","threadId":"42982","inReplyTo":"20160804053405.ifjjryejgbwkkatt@sigill.intra.peff.net","subject":"Re: [PATCH v3] pager: move pager-specific setup into the build","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-04T11:34:10Z","receivedAt":"2016-08-04T11:36:01Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Thu, Aug 04, 2016 at 03:43:01AM +0000, Eric Wong wrote:\n> \n> > +PAGER_ENV_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(PAGER_ENV)))\"\n> > +PAGER_ENV_CQ_SQ = $(subst ','\\'',$(PAGER_ENV_CQ))\n> > +BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'\n> \n> Here we set up CQ_SQ, but there is no PAGER_ENV_SQ.\n> \n> And then...\n\n> > @@ -1766,6 +1782,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n> > -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \\\n> > -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n> > -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \\\n> > + -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \\\n> > $@.sh >$@+\n> > endef\n> \n> Here we depend on writing PAGER_ENV_SQ, which will be blank (and\n> git-sh-setup is broken as a result).\n\nGood catch! And the reason we didn't notice git-sh-setup is\nbroken is nobody uses git_pager in-tree from that, anymore.\nHowever, I suspect we'll have to support it indefinitely due to\ncustom scripts and contrib/examples.\n\nMade the following change for v4:\n\ndiff --git a/Makefile b/Makefile\nindex 0b36b5e..fc9b017 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1641,6 +1641,7 @@ ifdef DEFAULT_HELP_FORMAT\n BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='\"$(DEFAULT_HELP_FORMAT)\"'\n endif\n \n+PAGER_ENV_SQ = $(subst ','\\'',$(PAGER_ENV))\n PAGER_ENV_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(PAGER_ENV)))\"\n PAGER_ENV_CQ_SQ = $(subst ','\\'',$(PAGER_ENV_CQ))\n BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex e4fc5c8..c8dc665 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -49,6 +49,19 @@ test_expect_success TTY 'LESS and LV envvars are set for pagination' '\n \tgrep ^LV= pager-env.out\n '\n \n+test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' '\n+\t(\n+\t\tsane_unset LESS LV &&\n+\t\tPAGER=\"env >pager-env.out; wc\" &&\n+\t\texport PAGER &&\n+\t\tPATH=\"$(git --exec-path):$PATH\" &&\n+\t\texport PATH &&\n+\t\ttest_terminal sh -c \". git-sh-setup && git_pager\"\n+\t) &&\n+\tgrep ^LESS= pager-env.out &&\n+\tgrep ^LV= pager-env.out\n+'\n+\n test_expect_success TTY 'some commands do not use a pager' '\n \trm -f paginated.out &&\n \ttest_terminal git rev-list HEAD &&\n\n> > --- a/config.mak.uname\n> > +++ b/config.mak.uname\n> > @@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)\n> > \tHAVE_PATHS_H = YesPlease\n> > \tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n> > \tHAVE_BSD_SYSCTL = YesPlease\n> > +\tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n> > endif\n> \n> Is it worth setting up PAGER_ENV's default values before including\n> config.mak.*, and then using \"+=\" here? That avoids this line getting\n> out of sync with the usual defaults.\n\nGood point, but it makes ordering problematic for folks\nwho want to override it config.mak or command-line.\n\nWe may have to do something like we do for BASIC_CFLAGS and\nsuch, but I'm not sure it's worth the effort when somebody\ndoesn't wants a different value for one of the flags.\n\n> > +static void setup_pager_env(struct argv_array *env)\n> > +{\n> \n> I know you said you don't like string parsing in C. Here is a patch (on\n> top of yours) that converts the parsing to shell, and generates a\n> pre-built array-of-struct (this is similar to the big series I posted\n> long ago, but just touching this one spot, not invading the whole\n> Makefile). Feel free to ignore it as over-engineered, but I thought I'd\n> throw it out there in case it appeals.\n\nYeah, but I'd rather not introduce more complexity into the\nbuild process, either (unless it's a performance-sensitive part,\nwhich this is not). Also, while my original 2/2 to make it\nconfigurable at runtime was discarded, I wouldn't rule out\nsomebody making a compelling case for it and it would be\nan easier change from the parse-at-runtime code.\n"},{"id":"293077","messageId":"20160804114025.GA14475@starla","threadId":"42982","inReplyTo":"20160804034301.GA31427@starla","subject":"[PATCH v4] pager: move pager-specific setup into the build","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-04T11:40:25Z","receivedAt":"2016-08-04T11:50:59Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Allowing PAGER_ENV to be set at build-time allows us to move\npager-specific knowledge out of our build. This allows us to\nset a better default for FreeBSD more(1), which pretends not to\nunderstand ANSI color escapes if the MORE environment variable\nis left empty, but accepts the same variables as less(1)\n\nOriginally-from:\n https://public-inbox.org/git/xmqq61piw4yf.fsf@gitster.dls.corp.google.com/\n\nHelped-by: Junio C Hamano \nHelped-by: Jeff King \nSigned-off-by: Eric Wong \n---\n v4 changes: (diff @ <20160804113410.GA13908@starla>)\n - reworded commit messages and took ownership as suggested\n privately by Junio\n - fixed git-sh-setup and add test for untested git_pager\n\n The following changes since commit f8f7adce9fc50a11a764d57815602dcb818d1816:\n\n Sync with maint (2016-07-28 14:21:18 -0700)\n\n are available in the git repository at:\n\n git://bogomips.org/git-svn.git pager-env-v4\n\n for you to fetch changes up to 3b8e70c37e96b4e19475fb6dc480a82b292bd28f:\n\n pager: move pager-specific setup into the build (2016-08-04 11:31:28 +0000)\n\n ----------------------------------------------------------------\n Eric Wong (1):\n pager: move pager-specific setup into the build\n\n Makefile | 21 ++++++++++++++++++++-\n config.mak.uname | 1 +\n git-sh-setup.sh | 8 +++++---\n pager.c | 32 ++++++++++++++++++++++++++++----\n t/t7006-pager.sh | 13 +++++++++++++\n 5 files changed, 67 insertions(+), 8 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6a13386..fc9b017 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -370,6 +370,14 @@ all::\n # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.\n #\n # Define HAVE_GETDELIM if your system has the getdelim() function.\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=FRX LV=-c\n+#\n+# to say \"export LESS=FRX (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@@ -1500,6 +1508,10 @@ ifeq ($(PYTHON_PATH),)\n NO_PYTHON = NoThanks\n endif\n \n+ifndef PAGER_ENV\n+PAGER_ENV = LESS=FRX LV=-c\n+endif\n+\n QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir\n QUIET_SUBDIR1 =\n \n@@ -1629,6 +1641,11 @@ ifdef DEFAULT_HELP_FORMAT\n BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='\"$(DEFAULT_HELP_FORMAT)\"'\n endif\n \n+PAGER_ENV_SQ = $(subst ','\\'',$(PAGER_ENV))\n+PAGER_ENV_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(PAGER_ENV)))\"\n+PAGER_ENV_CQ_SQ = $(subst ','\\'',$(PAGER_ENV_CQ))\n+BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'\n+\n ALL_CFLAGS += $(BASIC_CFLAGS)\n ALL_LDFLAGS += $(BASIC_LDFLAGS)\n \n@@ -1753,7 +1770,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):$(SANE_TEXT_GREP)\n+\t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)\n define cmd_munge_script\n $(RM) $@ $@+ && \\\n sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n@@ -1766,6 +1783,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \\\n -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \\\n+ -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \\\n $@.sh >$@+\n endef\n \n@@ -2173,6 +2191,7 @@ GIT-BUILD-OPTIONS: FORCE\n \t@echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@+\n \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@+\n \t@echo NO_UNIX_SOCKETS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_UNIX_SOCKETS)))'\\' >>$@+\n+\t@echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n ifdef TEST_OUTPUT_DIRECTORY\n \t@echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n endif\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 17fed2f..b232908 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)\n \tHAVE_PATHS_H = YesPlease\n \tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n \tHAVE_BSD_SYSCTL = YesPlease\n+\tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 0c34aa6..a8a4576 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -163,9 +163,11 @@ git_pager() {\n \telse\n \t\tGIT_PAGER=cat\n \tfi\n-\t: \"${LESS=-FRX}\"\n-\t: \"${LV=-c}\"\n-\texport LESS LV\n+\tfor vardef in @@PAGER_ENV@@\n+\tdo\n+\t\tvar=${vardef%%=*}\n+\t\teval \": \\\"\\${$vardef}\\\" && export $var\"\n+\tdone\n \n \teval \"$GIT_PAGER\" '\"$@\"'\n }\ndiff --git a/pager.c b/pager.c\nindex 4bc0481..6470b81 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -63,14 +63,38 @@ const char *git_pager(int stdout_is_tty)\n \treturn pager;\n }\n \n+static void setup_pager_env(struct argv_array *env)\n+{\n+\tconst char **argv;\n+\tint i;\n+\tchar *pager_env = xstrdup(PAGER_ENV);\n+\tint n = split_cmdline(pager_env, &argv);\n+\n+\tif (n < 0)\n+\t\tdie(\"malformed build-time PAGER_ENV: %s\",\n+\t\t\tsplit_cmdline_strerror(n));\n+\n+\tfor (i = 0; i < n; i++) {\n+\t\tchar *cp = strchr(argv[i], '=');\n+\n+\t\tif (!cp)\n+\t\t\tdie(\"malformed build-time PAGER_ENV\");\n+\n+\t\t*cp = '\\0';\n+\t\tif (!getenv(argv[i])) {\n+\t\t\t*cp = '=';\n+\t\t\targv_array_push(env, argv[i]);\n+\t\t}\n+\t}\n+\tfree(pager_env);\n+\tfree(argv);\n+}\n+\n void prepare_pager_args(struct child_process *pager_process, const char *pager)\n {\n \targv_array_push(&pager_process->args, pager);\n \tpager_process->use_shell = 1;\n-\tif (!getenv(\"LESS\"))\n-\t\targv_array_push(&pager_process->env_array, \"LESS=FRX\");\n-\tif (!getenv(\"LV\"))\n-\t\targv_array_push(&pager_process->env_array, \"LV=-c\");\n+\tsetup_pager_env(&pager_process->env_array);\n }\n \n void setup_pager(void)\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex e4fc5c8..c8dc665 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -49,6 +49,19 @@ test_expect_success TTY 'LESS and LV envvars are set for pagination' '\n \tgrep ^LV= pager-env.out\n '\n \n+test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' '\n+\t(\n+\t\tsane_unset LESS LV &&\n+\t\tPAGER=\"env >pager-env.out; wc\" &&\n+\t\texport PAGER &&\n+\t\tPATH=\"$(git --exec-path):$PATH\" &&\n+\t\texport PATH &&\n+\t\ttest_terminal sh -c \". git-sh-setup && git_pager\"\n+\t) &&\n+\tgrep ^LESS= pager-env.out &&\n+\tgrep ^LV= pager-env.out\n+'\n+\n test_expect_success TTY 'some commands do not use a pager' '\n \trm -f paginated.out &&\n \ttest_terminal git rev-list HEAD &&\n-- \nEW\n"},{"id":"293108","messageId":"20160804175328.tkjkoo34b43bvwsb@sigill.intra.peff.net","threadId":"42982","inReplyTo":"20160804113410.GA13908@starla","subject":"Re: [PATCH v3] pager: move pager-specific setup into the build","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-04T17:53:28Z","receivedAt":"2016-08-04T17:53:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 04, 2016 at 11:34:10AM +0000, Eric Wong wrote:\n\n> > > --- a/config.mak.uname\n> > > +++ b/config.mak.uname\n> > > @@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)\n> > > \tHAVE_PATHS_H = YesPlease\n> > > \tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n> > > \tHAVE_BSD_SYSCTL = YesPlease\n> > > +\tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n> > > endif\n> > \n> > Is it worth setting up PAGER_ENV's default values before including\n> > config.mak.*, and then using \"+=\" here? That avoids this line getting\n> > out of sync with the usual defaults.\n> \n> Good point, but it makes ordering problematic for folks\n> who want to override it config.mak or command-line.\n\nI'm not sure it changes much for them. Their \"=\" in config.mak, etc,\nwould override our default, and anything on the command line overrides\nall of the in-Makefile stuff anyway. The only difference would be if\nthey use \"+=\" in config.mak, but there I think it would be an\nimprovement.\n\nI'm OK to leave it as-is until somebody actually cares, though.\n\n> > I know you said you don't like string parsing in C. Here is a patch (on\n> > top of yours) that converts the parsing to shell, and generates a\n> > pre-built array-of-struct (this is similar to the big series I posted\n> > long ago, but just touching this one spot, not invading the whole\n> > Makefile). Feel free to ignore it as over-engineered, but I thought I'd\n> > throw it out there in case it appeals.\n> \n> Yeah, but I'd rather not introduce more complexity into the\n> build process, either (unless it's a performance-sensitive part,\n> which this is not). Also, while my original 2/2 to make it\n> configurable at runtime was discarded, I wouldn't rule out\n> somebody making a compelling case for it and it would be\n> an easier change from the parse-at-runtime code.\n\nYeah, I had similar thoughts while writing it.\n\nYour v4 patch looks fine to me.\n\n-Peff\n"}]}