[PATCH 0/2] add PAGER_ENV to build and core.pagerEnv to config

This series allows us to more easily configure platform-specific
defaults at build-time and also allow users to override them
at runtime via config.

Previous discussion in this thread:

	https://public-inbox.org/git/52D87A79.6060600@rawbw.com/T/

The following changes since commit f8f7adce9fc50a11a764d57815602dcb818d1816:

  Sync with maint (2016-07-28 14:21:18 -0700)

are available in the git repository at:

  git://bogomips.org/git-svn.git pager-env

for you to fetch changes up to 1563ef177f9c1ee990bb3547f16bd7568a17379a:

  pager: implement core.pagerEnv in config (2016-08-01 00:51:42 +0000)

----------------------------------------------------------------
Eric Wong (1):
      pager: implement core.pagerEnv in config

Junio C Hamano (1):
      pager: move pager-specific setup into the build

 Documentation/config.txt |  7 +++++++
 Makefile                 | 19 +++++++++++++++++--
 config.mak.uname         |  1 +
 git-sh-setup.sh          |  8 +++++---
 pager.c                  | 35 +++++++++++++++++++++++++++++++----
 t/t7006-pager.sh         | 14 ++++++++++++++
 6 files changed, 75 insertions(+), 9 deletions(-)

From: Junio C Hamano <redacted>

Allowing PAGER_ENV to be set at build-time allows us to move
pager-specific knowledge out of our build.  Currently, this
allows us to set a better default for FreeBSD where more(1)
is the same binary as less(1).

This also prepares us for introducing a run-time config knob to
override the build-time environment in the next commit.

Originally-from:
 https://public-inbox.org/git/xmqq61piw4yf.fsf@gitster.dls.corp.google.com/

Signed-off-by: Eric Wong <redacted>
---
 Makefile         | 19 +++++++++++++++++--
 config.mak.uname |  1 +
 git-sh-setup.sh  |  8 +++++---
 pager.c          | 32 ++++++++++++++++++++++++++++----
 4 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index 6a13386..fe469a6 100644
--- a/Makefile
+++ b/Makefile

@@ -370,6 +370,14 @@ all::
 # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.
 #
 # Define HAVE_GETDELIM if your system has the getdelim() function.
+#
+# Define PAGER_ENV to a SP separated VAR=VAL pairs to define
+# default environment variables to be passed when a pager is spawned, e.g.
+#
+#    PAGER_ENV = LESS=FRX LV=-c
+#
+# to say "export LESS=FRX (and LV=-c) if the environment variable
+# LESS (and LV) is not set, respectively".
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -1500,6 +1508,10 @@ ifeq ($(PYTHON_PATH),)
 NO_PYTHON = NoThanks
 endif
 
+ifndef PAGER_ENV
+PAGER_ENV = LESS=FRX LV=-c
+endif
+
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 

@@ -1579,6 +1591,7 @@ PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
 TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
 DIFF_SQ = $(subst ','\'',$(DIFF))
 PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
+PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
 
 # We must filter out any object files from $(GITLIBS),
 # as it is typically used like:

@@ -1591,7 +1604,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
 LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
-	$(COMPAT_CFLAGS)
+	$(COMPAT_CFLAGS) -DPAGER_ENV='$(PAGER_ENV_SQ)'
 LIB_OBJS += $(COMPAT_OBJS)
 
 # Quote for C

@@ -1753,7 +1766,7 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
-	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP)
+	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)
 define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \

@@ -1766,6 +1779,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
     -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
     -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \
+    -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \
     $@.sh >$@+
 endef
 

@@ -2173,6 +2187,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+
 	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
+	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif

diff --git a/config.mak.uname b/config.mak.uname
index 17fed2f..2484dfb 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)
 	HAVE_PATHS_H = YesPlease
 	GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
 	HAVE_BSD_SYSCTL = YesPlease
+	PAGER_ENV = LESS=FRX LV=-c MORE=-R
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 0c34aa6..0f5a56f 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh

@@ -163,9 +163,11 @@ git_pager() {
 	else
 		GIT_PAGER=cat
 	fi
-	: "${LESS=-FRX}"
-	: "${LV=-c}"
-	export LESS LV
+	for vardef in @@PAGER_ENV@@
+	do
+		var=${vardef%%=*}
+		eval ": \${$vardef} && export $var"
+	done
 
 	eval "$GIT_PAGER" '"$@"'
 }

diff --git a/pager.c b/pager.c
index 4bc0481..2f2cadc 100644
--- a/pager.c
+++ b/pager.c

@@ -63,14 +63,38 @@ const char *git_pager(int stdout_is_tty)
 	return pager;
 }
 
+#define stringify_(x) #x
+#define stringify(x) stringify_(x)
+
+static void setup_pager_env(struct argv_array *env)
+{
+	const char *pager_env = stringify(PAGER_ENV);
+
+	while (*pager_env) {
+		struct strbuf buf = STRBUF_INIT;
+		const char *cp = strchrnul(pager_env, '=');
+
+		if (!*cp)
+			die("malformed build-time PAGER_ENV");
+		strbuf_add(&buf, pager_env, cp - pager_env);
+		cp = strchrnul(pager_env, ' ');
+		if (!getenv(buf.buf)) {
+			strbuf_reset(&buf);
+			strbuf_add(&buf, pager_env, cp - pager_env);
+			argv_array_push(env, strbuf_detach(&buf, NULL));
+		}
+		strbuf_reset(&buf);
+		while (*cp && *cp == ' ')
+			cp++;
+		pager_env = cp;
+	}
+}
+
 void prepare_pager_args(struct child_process *pager_process, const char *pager)
 {
 	argv_array_push(&pager_process->args, pager);
 	pager_process->use_shell = 1;
-	if (!getenv("LESS"))
-		argv_array_push(&pager_process->env_array, "LESS=FRX");
-	if (!getenv("LV"))
-		argv_array_push(&pager_process->env_array, "LV=-c");
+	setup_pager_env(&pager_process->env_array);
 }
 
 void setup_pager(void)

-- 
EW

This allows overriding the build-time PAGER_ENV variable
at run-time.

Inspired by part 1 of an idea from Kyle J. McKay at:
https://public-inbox.org/git/62DB6DEF-8B39-4481-BA06-245BF45233E5@gmail.com/

Signed-off-by: Eric Wong <redacted>
---
 Documentation/config.txt |  7 +++++++
 pager.c                  |  5 ++++-
 t/t7006-pager.sh         | 14 ++++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8b1aee4..6c20269 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -714,6 +714,13 @@ Likewise, when the `LV` environment variable is unset, Git sets it
 to `-c`.  You can override this setting by exporting `LV` with
 another value or setting `core.pager` to `lv +c`.
 
+core.pagerEnv::
+	Environment for running `core.pager`.
++
+Defaults to the value set at build, usually `LESS=FRX LV=-c`.
+On platforms where `more` and `less` are the same binary,
+`LESS=FRX LV=-c MORE=FRX` is appropriate.
+
 core.whitespace::
 	A comma separated list of common whitespace problems to
 	notice.  'git diff' will use `color.diff.whitespace` to

diff --git a/pager.c b/pager.c
index 2f2cadc..cc2df7c 100644
--- a/pager.c
+++ b/pager.c

@@ -68,7 +68,10 @@ const char *git_pager(int stdout_is_tty)
 
 static void setup_pager_env(struct argv_array *env)
 {
-	const char *pager_env = stringify(PAGER_ENV);
+	const char *pager_env;
+
+	if (git_config_get_value("core.pagerenv", &pager_env))
+		pager_env = stringify(PAGER_ENV);
 
 	while (*pager_env) {
 		struct strbuf buf = STRBUF_INIT;

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index e4fc5c8..0c482fc 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh

@@ -456,4 +456,18 @@ test_expect_success 'command with underscores does not complain' '
 	test_cmp expect actual
 '
 
+test_expect_success TTY 'core.pagerEnv overrides build-time env' '
+	(
+		sane_unset LESS LV MORE &&
+		git config core.pagerEnv MORE=-R &&
+		PAGER="env >pager-env.out; wc" &&
+		export PAGER &&
+		test_terminal git log
+	) &&
+	git config --unset core.pagerEnv &&
+	grep ^MORE=-R pager-env.out &&
+	grep -v ^LESS= pager-env.out &&
+	grep -v ^LV= pager-env.out
+'
+
 test_done

-- 
EW

On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:

+static void setup_pager_env(struct argv_array *env)
+{
+	const char *pager_env = stringify(PAGER_ENV);
+
+	while (*pager_env) {
+		struct strbuf buf = STRBUF_INIT;
+		const char *cp = strchrnul(pager_env, '=');
+
+		if (!*cp)
+			die("malformed build-time PAGER_ENV");
+		strbuf_add(&buf, pager_env, cp - pager_env);
+		cp = strchrnul(pager_env, ' ');
+		if (!getenv(buf.buf)) {
+			strbuf_reset(&buf);
+			strbuf_add(&buf, pager_env, cp - pager_env);
+			argv_array_push(env, strbuf_detach(&buf, NULL));
+		}
+		strbuf_reset(&buf);
+		while (*cp && *cp == ' ')
+			cp++;
+		pager_env = cp;
+	}

So it looks like this function splits on spaces but doesn't provide any
escaping mechanism.  Is there any case in which we want to accept
environment variables containing whitespace?  I ask this as someone that
has EDITOR set to "gvim -f" on occasion and seeing how tools sometimes
handle that poorly.

Even without that, I think this series is probably an improvement over
the status quo.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

"brian m. carlson" [off-list ref] wrote:

On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:

quoted

+		while (*cp && *cp == ' ')
+			cp++;

So it looks like this function splits on spaces but doesn't provide any
escaping mechanism.  Is there any case in which we want to accept
environment variables containing whitespace?  I ask this as someone that
has EDITOR set to "gvim -f" on occasion and seeing how tools sometimes
handle that poorly.

Yes, it's only split on spaces right now.  While I don't think
there's any current case where spaces would be useful/desirable;
I suppose a 3rd patch in this series could add support for using
split_cmdline (from alias.c)...

W dniu 01.08.2016 o 09:00, Eric Wong pisze:

"brian m. carlson" [off-list ref] wrote:

quoted

On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:

quoted

+		while (*cp && *cp == ' ')
+			cp++;

So it looks like this function splits on spaces but doesn't provide any
escaping mechanism.  Is there any case in which we want to accept
environment variables containing whitespace?  I ask this as someone that
has EDITOR set to "gvim -f" on occasion and seeing how tools sometimes
handle that poorly.

This is to handle environment variables holding program options,
which are usually (but possibly not often) using single character
options bundled together, that is, not using spaces.

Moreover, it is about holding program options to pager.

Yes, it's only split on spaces right now.  While I don't think
there's any current case where spaces would be useful/desirable;
I suppose a 3rd patch in this series could add support for using
split_cmdline (from alias.c)...

Is there any pager that needs spaces in options-set environment
variable?  Does MORE allow option bundling?

-- 
Jakub Narębski

On Mon, Aug 01, 2016 at 10:57:02AM +0200, Jakub Narębski wrote:

W dniu 01.08.2016 o 09:00, Eric Wong pisze:

quoted

"brian m. carlson" [off-list ref] wrote:

quoted

So it looks like this function splits on spaces but doesn't provide any
escaping mechanism.  Is there any case in which we want to accept
environment variables containing whitespace?  I ask this as someone that
has EDITOR set to "gvim -f" on occasion and seeing how tools sometimes
handle that poorly.

This is to handle environment variables holding program options,
which are usually (but possibly not often) using single character
options bundled together, that is, not using spaces.

Moreover, it is about holding program options to pager.

I understand that.  My point is that we should consider corner cases
like how we're going to handle spaces.

quoted

Yes, it's only split on spaces right now.  While I don't think
there's any current case where spaces would be useful/desirable;
I suppose a 3rd patch in this series could add support for using
split_cmdline (from alias.c)...

Is there any pager that needs spaces in options-set environment
variable?  Does MORE allow option bundling?

We seem to accept GIT_PAGER="par | less" and par definitely accepts
spaces in its environment variables.  That seems to be a corner case,
though, and I haven't seen par practically used in years.

We may also want to consider EXINIT for people who pipe to vi.  Again,
I'm not sure this is very common; most people would use an .exrc or
.vimrc.

I'd say if we can't come up with any better examples, I'd skip handling
it for now.  I'll try to come up with a patch to add it later.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

On Mon, Aug 01, 2016 at 01:43:03AM +0000, brian m. carlson wrote:

On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:

quoted

+static void setup_pager_env(struct argv_array *env)
+{
+	const char *pager_env = stringify(PAGER_ENV);
+
+	while (*pager_env) {
+		struct strbuf buf = STRBUF_INIT;
+		const char *cp = strchrnul(pager_env, '=');
+
+		if (!*cp)
+			die("malformed build-time PAGER_ENV");
+		strbuf_add(&buf, pager_env, cp - pager_env);
+		cp = strchrnul(pager_env, ' ');
+		if (!getenv(buf.buf)) {
+			strbuf_reset(&buf);
+			strbuf_add(&buf, pager_env, cp - pager_env);
+			argv_array_push(env, strbuf_detach(&buf, NULL));
+		}
+		strbuf_reset(&buf);
+		while (*cp && *cp == ' ')
+			cp++;
+		pager_env = cp;
+	}

So it looks like this function splits on spaces but doesn't provide any
escaping mechanism.  Is there any case in which we want to accept
environment variables containing whitespace?  I ask this as someone that
has EDITOR set to "gvim -f" on occasion and seeing how tools sometimes
handle that poorly.

Even without that, I think this series is probably an improvement over
the status quo.

I'm not too worried about spaces here. This is a resurrection of an old
discussion, and in all that time, I think the only realistic suggestions
for built-in values have been pretty tame.

If this were used to parse arbitrary user-provided runtime values, I'd
be more concerned. But I'm not sure why we would need that. Your $EDITOR
example is arbitrary shell code, and we let the shell handle it (modulo
some efficiency shortcuts). Likewise, fancy runtime things should go in
GIT_PAGER, where you can not only set options with spaces, but do fancy
things like pipes, shell functions, etc.

The use of stringify() here is funny to me; I think there is a cpp
tokenizing step in the middle that will do things like gobble up
whitespace (but I'm not sure if it has other possible effects). I think
our more usual method here would be to C-quote in the Makefile (with the
equivalent of 's/\\/\\\\/g; s/"/\\"/g'), and then pass it to the
compiler as a string literal, like -DPAGER_ENV=\"$(PAGER_ENV_CQ_SQ\".

-Peff

On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong [off-list ref] wrote:

From: Junio C Hamano <redacted>

Allowing PAGER_ENV to be set at build-time allows us to move
pager-specific knowledge out of our build.  Currently, this
allows us to set a better default for FreeBSD where more(1)
is the same binary as less(1).

Nice. I was just too lazy to do something like this and "export
PAGER=less LESS=FRX" then ignored it :-P

Slightly off topic, but pagers like 'more' does not understand colors
either. But color.ui = auto does not know what and prints color code
anyway. It would be nice if we had some configuration to describe
"this pager can show colors, that pager does not" so I don't have to
maintain separate .gitconfig files on two platforms.
-- 
Duy

On Mon, Aug 01, 2016 at 01:05:57AM +0000, Eric Wong wrote:

This allows overriding the build-time PAGER_ENV variable
at run-time.

Inspired by part 1 of an idea from Kyle J. McKay at:
https://public-inbox.org/git/62DB6DEF-8B39-4481-BA06-245BF45233E5@gmail.com/

This commit message is missing the "why" (I tried to get it from the
referenced email, but I am still confused).

What does this buy you over:

  GIT_PAGER='less -whatever-options-you-like'

? Sure, you have to say "less" there and not just "if we happen to be
using less, use these options with it". But that distinction is
important for a build-time default, not for a run-time one. And by
pointing people to GIT_PAGER, they can do a lot _more_ than they can
with PAGER_ENV, including the full power of the shell (brian gave an
example of "par | less" earlier; I use "diff-highlight | less").

-Peff

On Mon, Aug 01, 2016 at 07:46:34PM +0200, Duy Nguyen wrote:

On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong [off-list ref] wrote:

quoted

From: Junio C Hamano <redacted>

Allowing PAGER_ENV to be set at build-time allows us to move
pager-specific knowledge out of our build.  Currently, this
allows us to set a better default for FreeBSD where more(1)
is the same binary as less(1).

Nice. I was just too lazy to do something like this and "export
PAGER=less LESS=FRX" then ignored it :-P

Slightly off topic, but pagers like 'more' does not understand colors
either. But color.ui = auto does not know what and prints color code
anyway. It would be nice if we had some configuration to describe
"this pager can show colors, that pager does not" so I don't have to
maintain separate .gitconfig files on two platforms.

If you are interested, I suggest you read the thread linked earlier:

  https://public-inbox.org/git/52D87A79.6060600%40rawbw.com/T/#u

which discusses this and other issues. But basically, I think you cannot
really solve this without getting intimate with each pager (which people
seemed not to want to do).

-Peff

On Mon, Aug 01, 2016 at 08:01:13PM +0200, Duy Nguyen wrote:

quoted

If you are interested, I suggest you read the thread linked earlier:

  https://public-inbox.org/git/52D87A79.6060600%40rawbw.com/T/#u

which discusses this and other issues. But basically, I think you cannot
really solve this without getting intimate with each pager (which people
seemed not to want to do).

Cooking pager specifics in git does sound bad. But it does not have to
be that way. What if we delegate the decision whether to color or not
to a script (e.g. by setting color.ui= "script <path to the script>")?
The script has all the info (env variables, uname, user preference...)
and can make a better decision than 'is stdout a tty?'. It's not about
out of the box experience, more towards customization (without
fragmenting .gitconfig files too much).

It sounds like we are solving two separate problems.

I was mostly concerned with the out-of-the-box experience. E.g., you
build git and run "git log" and it prints out gibberish, either because
of your $PAGER or your $LESS settings (which you might have set years
ago).

For more advanced usage like yours, I'd shove any personal logic into a
wrapper around your pager script.  I think the particular decision you
want, though, is related to color.pager, which is outside that scope.
I'm not sure exactly what your setup looks like, but I wonder if it
would be served by better config support (i.e., why do you need a script
to dynamically look at your pager and see if color.pager should be
turned on; can't you configure both at the same time?).

-Peff

On Mon, Aug 1, 2016 at 7:52 PM, Jeff King [off-list ref] wrote:

On Mon, Aug 01, 2016 at 07:46:34PM +0200, Duy Nguyen wrote:

quoted

On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong [off-list ref] wrote:

quoted

From: Junio C Hamano <redacted>

Allowing PAGER_ENV to be set at build-time allows us to move
pager-specific knowledge out of our build.  Currently, this
allows us to set a better default for FreeBSD where more(1)
is the same binary as less(1).

Nice. I was just too lazy to do something like this and "export
PAGER=less LESS=FRX" then ignored it :-P

Slightly off topic, but pagers like 'more' does not understand colors
either. But color.ui = auto does not know what and prints color code
anyway. It would be nice if we had some configuration to describe
"this pager can show colors, that pager does not" so I don't have to
maintain separate .gitconfig files on two platforms.

If you are interested, I suggest you read the thread linked earlier:

  https://public-inbox.org/git/52D87A79.6060600%40rawbw.com/T/#u

which discusses this and other issues. But basically, I think you cannot
really solve this without getting intimate with each pager (which people
seemed not to want to do).

Cooking pager specifics in git does sound bad. But it does not have to
be that way. What if we delegate the decision whether to color or not
to a script (e.g. by setting color.ui= "script <path to the script>")?
The script has all the info (env variables, uname, user preference...)
and can make a better decision than 'is stdout a tty?'. It's not about
out of the box experience, more towards customization (without
fragmenting .gitconfig files too much).
-- 
Duy

Changes from v1:

* dropped stringify macro in favor for quoting in Makefile
  (diff below)
  I'm not sure I like this change, and might be inclined to
  go in the opposite direction of using the stringify macro
  more widely to simplify the Makefile; but that is a separate
  topic.

* dropped 2/2, I don't have a good rationale for it, either,
  other than "it seemed easy" after 1/2 :>

The following changes since commit f8f7adce9fc50a11a764d57815602dcb818d1816:

  Sync with maint (2016-07-28 14:21:18 -0700)

are available in the git repository at:

  git://bogomips.org/git-svn.git pager-env-v2

for you to fetch changes up to d3aed319c9abac006060bc81e865c93ff8363066:

  pager: move pager-specific setup into the build (2016-08-01 21:46:25 +0000)

----------------------------------------------------------------
Junio C Hamano (1):
      pager: move pager-specific setup into the build

 Makefile         | 20 +++++++++++++++++++-
 config.mak.uname |  1 +
 git-sh-setup.sh  |  8 +++++---
 pager.c          | 29 +++++++++++++++++++++++++----
 4 files changed, 50 insertions(+), 8 deletions(-)

interdiff from 1/1 v1:

diff --git a/Makefile b/Makefile
index fe469a6..0b36b5e 100644
--- a/Makefile
+++ b/Makefile

@@ -1591,7 +1591,6 @@ PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
 TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
 DIFF_SQ = $(subst ','\'',$(DIFF))
 PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
-PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
 
 # We must filter out any object files from $(GITLIBS),
 # as it is typically used like:

@@ -1604,7 +1603,7 @@ PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
 LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
-	$(COMPAT_CFLAGS) -DPAGER_ENV='$(PAGER_ENV_SQ)'
+	$(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
 # Quote for C

@@ -1642,6 +1641,10 @@ ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
 endif
 
+PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
+PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
+BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 

diff --git a/pager.c b/pager.c
index 2f2cadc..cd1ac54 100644
--- a/pager.c
+++ b/pager.c

@@ -63,12 +63,9 @@ const char *git_pager(int stdout_is_tty)
 	return pager;
 }
 
-#define stringify_(x) #x
-#define stringify(x) stringify_(x)
-
 static void setup_pager_env(struct argv_array *env)
 {
-	const char *pager_env = stringify(PAGER_ENV);
+	const char *pager_env = PAGER_ENV;
 
 	while (*pager_env) {
 		struct strbuf buf = STRBUF_INIT;

-- 
EW

From: Junio C Hamano <redacted>

Allowing PAGER_ENV to be set at build-time allows us to move
pager-specific knowledge out of our build.  Currently, this
allows us to set a better default for FreeBSD where more(1)
is the same binary as less(1).

This also prepares us for introducing a run-time config knob to
override the build-time environment in the next commit.

Originally-from:
 https://public-inbox.org/git/xmqq61piw4yf.fsf@gitster.dls.corp.google.com/

Signed-off-by: Eric Wong <redacted>
---
 Makefile         | 20 +++++++++++++++++++-
 config.mak.uname |  1 +
 git-sh-setup.sh  |  8 +++++---
 pager.c          | 29 +++++++++++++++++++++++++----
 4 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 6a13386..0b36b5e 100644
--- a/Makefile
+++ b/Makefile

@@ -370,6 +370,14 @@ all::
 # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.
 #
 # Define HAVE_GETDELIM if your system has the getdelim() function.
+#
+# Define PAGER_ENV to a SP separated VAR=VAL pairs to define
+# default environment variables to be passed when a pager is spawned, e.g.
+#
+#    PAGER_ENV = LESS=FRX LV=-c
+#
+# to say "export LESS=FRX (and LV=-c) if the environment variable
+# LESS (and LV) is not set, respectively".
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -1500,6 +1508,10 @@ ifeq ($(PYTHON_PATH),)
 NO_PYTHON = NoThanks
 endif
 
+ifndef PAGER_ENV
+PAGER_ENV = LESS=FRX LV=-c
+endif
+
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 

@@ -1629,6 +1641,10 @@ ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
 endif
 
+PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
+PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
+BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 

@@ -1753,7 +1769,7 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
-	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP)
+	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)
 define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \

@@ -1766,6 +1782,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
     -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
     -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \
+    -e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \
     $@.sh >$@+
 endef
 

@@ -2173,6 +2190,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+
 	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
+	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif

diff --git a/config.mak.uname b/config.mak.uname
index 17fed2f..2484dfb 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)
 	HAVE_PATHS_H = YesPlease
 	GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
 	HAVE_BSD_SYSCTL = YesPlease
+	PAGER_ENV = LESS=FRX LV=-c MORE=-R
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 0c34aa6..0f5a56f 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh

@@ -163,9 +163,11 @@ git_pager() {
 	else
 		GIT_PAGER=cat
 	fi
-	: "${LESS=-FRX}"
-	: "${LV=-c}"
-	export LESS LV
+	for vardef in @@PAGER_ENV@@
+	do
+		var=${vardef%%=*}
+		eval ": \${$vardef} && export $var"
+	done
 
 	eval "$GIT_PAGER" '"$@"'
 }

diff --git a/pager.c b/pager.c
index 4bc0481..cd1ac54 100644
--- a/pager.c
+++ b/pager.c

@@ -63,14 +63,35 @@ const char *git_pager(int stdout_is_tty)
 	return pager;
 }
 
+static void setup_pager_env(struct argv_array *env)
+{
+	const char *pager_env = PAGER_ENV;
+
+	while (*pager_env) {
+		struct strbuf buf = STRBUF_INIT;
+		const char *cp = strchrnul(pager_env, '=');
+
+		if (!*cp)
+			die("malformed build-time PAGER_ENV");
+		strbuf_add(&buf, pager_env, cp - pager_env);
+		cp = strchrnul(pager_env, ' ');
+		if (!getenv(buf.buf)) {
+			strbuf_reset(&buf);
+			strbuf_add(&buf, pager_env, cp - pager_env);
+			argv_array_push(env, strbuf_detach(&buf, NULL));
+		}
+		strbuf_reset(&buf);
+		while (*cp && *cp == ' ')
+			cp++;
+		pager_env = cp;
+	}
+}
+
 void prepare_pager_args(struct child_process *pager_process, const char *pager)
 {
 	argv_array_push(&pager_process->args, pager);
 	pager_process->use_shell = 1;
-	if (!getenv("LESS"))
-		argv_array_push(&pager_process->env_array, "LESS=FRX");
-	if (!getenv("LV"))
-		argv_array_push(&pager_process->env_array, "LV=-c");
+	setup_pager_env(&pager_process->env_array);
 }
 
 void setup_pager(void)

-- 
EW

On Mon, Aug 01, 2016 at 09:49:36PM +0000, Eric Wong wrote:

Changes from v1:

* dropped stringify macro in favor for quoting in Makefile
  (diff below)
  I'm not sure I like this change, and might be inclined to
  go in the opposite direction of using the stringify macro
  more widely to simplify the Makefile; but that is a separate
  topic.

I think that's a dangerous direction. Try this:

-- >8 --
cat >foo.c <<\EOF
#include <stdio.h>

#define stringify_(x) #x
#define stringify(x) stringify_(x)

int main(void)
{
	printf("%s", stringify(FOO));
	return 0;
}
EOF

while read -r input; do
	gcc -Wall -Werror -DFOO="$input" foo.c
	./a.out
done
-- 8< --

and then try input like:

  this has          a lot of spaces
  this has a \backslash

You should see:

  this has a lot of spaces
  this has aackslash

I'll grant that backslashes and runs of whitespace are not things we'd
expect to find in most of our build-time config, but it still seems like
a bad direction to go (and actually, I wouldn't be surprised if
backslashes do end up in some of our build-time variables on Windows).

-Peff

On Mon, Aug 01, 2016 at 09:49:37PM +0000, Eric Wong wrote:

+static void setup_pager_env(struct argv_array *env)
+{
+	const char *pager_env = PAGER_ENV;
+
+	while (*pager_env) {
+		struct strbuf buf = STRBUF_INIT;
+		const char *cp = strchrnul(pager_env, '=');
+
+		if (!*cp)
+			die("malformed build-time PAGER_ENV");
+		strbuf_add(&buf, pager_env, cp - pager_env);
+		cp = strchrnul(pager_env, ' ');
+		if (!getenv(buf.buf)) {
+			strbuf_reset(&buf);
+			strbuf_add(&buf, pager_env, cp - pager_env);
+			argv_array_push(env, strbuf_detach(&buf, NULL));
+		}

argv_array handles its own allocation, so this leaks the detached
strbuf.

You'd want:

  argv_array_push(env, buf.buf);
  strbuf_release(&buf);

or just:

  argv_array_pushf(env, "%.*s", (int)(cp - pager_env), pager_env);

Also:

+		strbuf_reset(&buf);

should this be strbuf_release()? If we didn't follow the conditional
above (because getenv() told us the variable was already set), then we
would not do do the detach/release there, and would finish the loop with
memory still allocated by "buf".

-Peff