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

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

[Eric Wong]

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

[PATCH 1/2] pager: move pager-specific setup into the build

[Eric Wong]

From: Junio C Hamano <gitster@pobox.com>

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 <e@80x24.org>
---
 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

[PATCH 2/2] pager: implement core.pagerEnv in config

[Eric Wong]

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 <e@80x24.org>
---
 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

Re: [PATCH 1/2] pager: move pager-specific setup into the build

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] pager: move pager-specific setup into the build

[Eric Wong]

"brian m. carlson" <sandals@crustytoothpaste.net> wrote:
> On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:
> > +		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)...

Re: [PATCH 1/2] pager: move pager-specific setup into the build

[Jakub Narębski]

W dniu 01.08.2016 o 09:00, Eric Wong pisze:
> "brian m. carlson" <sandals@crustytoothpaste.net> wrote:
>> On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:
>>> +		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

Re: [PATCH 1/2] pager: move pager-specific setup into the build

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]

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:
> > "brian m. carlson" <sandals@crustytoothpaste.net> wrote:
> >> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] pager: move pager-specific setup into the build

[Jeff King]

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:
> > +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

Re: [PATCH 2/2] pager: implement core.pagerEnv in config

[Jeff King]

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

Re: [PATCH 1/2] pager: move pager-specific setup into the build

[Duy Nguyen]

On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong <e@80x24.org> wrote:
> From: Junio C Hamano <gitster@pobox.com>
>
> 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

Re: [PATCH 1/2] pager: move pager-specific setup into the build

[Jeff King]

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

> On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong <e@80x24.org> wrote:
> > From: Junio C Hamano <gitster@pobox.com>
> >
> > 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

Re: [PATCH 1/2] pager: move pager-specific setup into the build

[Duy Nguyen]

On Mon, Aug 1, 2016 at 7:52 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Aug 01, 2016 at 07:46:34PM +0200, Duy Nguyen wrote:
>
>> On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong <e@80x24.org> wrote:
>> > From: Junio C Hamano <gitster@pobox.com>
>> >
>> > 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

Re: [PATCH 1/2] pager: move pager-specific setup into the build

[Jeff King]

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

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

Re: [PATCH 1/2] pager: move pager-specific setup into the build

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

All sensible arguments, including the rationale to reject 2/2.

Thanks.

[PATCH 0/1 v2] add PAGER_ENV to build

[Eric Wong]

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

[PATCH 1/1 v2] pager: move pager-specific setup into the build

[Eric Wong]

From: Junio C Hamano <gitster@pobox.com>

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 <e@80x24.org>
---
 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

Re: [PATCH 0/1 v2] add PAGER_ENV to build

[Jeff King]

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

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Junio C Hamano]

Eric Wong <e@80x24.org> writes:

> From: Junio C Hamano <gitster@pobox.com>
>
> 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).

Thanks for resurrecting, but I am not sure what "a better default"
is from the above description and with the patch.  Even though a
naive reading of the above (i.e. "less" and "more" are the same)
makes me expect that the patch will give the same set of default
environment settings to those on FreeBSD, you give LESS=FRX and
MORE=-R, i.e. they are configured differently.

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

This is now gone, judging from 1/1 on the subject line being not
1/2, right?

>
> Originally-from:
>  https://public-inbox.org/git/xmqq61piw4yf.fsf@gitster.dls.corp.google.com/
>
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
>  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)

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Jeff King]

On Mon, Aug 01, 2016 at 04:03:40PM -0700, Junio C Hamano wrote:

> Eric Wong <e@80x24.org> writes:
> 
> > From: Junio C Hamano <gitster@pobox.com>
> >
> > 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).
> 
> Thanks for resurrecting, but I am not sure what "a better default"
> is from the above description and with the patch.  Even though a
> naive reading of the above (i.e. "less" and "more" are the same)
> makes me expect that the patch will give the same set of default
> environment settings to those on FreeBSD, you give LESS=FRX and
> MORE=-R, i.e. they are configured differently.

I wondered about this, too. They are the same binary, but calling less
as "more" (or setting LESS_IS_MORE) causes less to behave "like more".
Looking at the manpage, none the usual FRX options is affected. So in
theory we could just set MORE=FRX on FreeBSD.

That would be bad a idea in general, as other non-less implementations
of more might get confused.  "more" is in POSIX, and so is $MORE (and it
does not understand any of our options).

You could also make the knob "MORE_IS_LESS" or something, and just
blindly copy $LESS to $MORE. That's a bad idea, though, if somebody does
stick one of the incompatible flags in the build options.

-Peff

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> > 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).
> 
> Thanks for resurrecting, but I am not sure what "a better default"
> is from the above description and with the patch.  Even though a
> naive reading of the above (i.e. "less" and "more" are the same)
> makes me expect that the patch will give the same set of default
> environment settings to those on FreeBSD, you give LESS=FRX and
> MORE=-R, i.e. they are configured differently.

Perhaps s/better/platform-appropriate/ ?

I just copied your original patch in setting MORE=-R
(but removed 'S' from LESS).

So v3 will be MORE=FRX, as less was added:

    commit 98170c0c3ba86eb1cc975e7848d075bf2abc1ed0
    Author: ps <ps@FreeBSD.org>
    Date:   Mon May 22 10:00:00 2000 +0000

	bmake glue for less.

and more was nuked:

    commit cde9059fa3e4dc7e259c3864d7536252a5c580a0
    Author: ps <ps@FreeBSD.org>
    Date:   Mon May 29 13:31:51 2000 +0000

	Nuke more from the repository.

And "git branch -r --contains" on both of those commits says
they showed up in the 5.0 release.  However, further
investigation says more was even gone by the 4.1.0 release

  git show origin/release/4.1.0:usr.bin/more # non-existent tree
  git show origin/release/4.0.0:usr.bin/more # tree still exists

But, "git show origin/release/4.0.0:usr.bin/more/option.c"
reveals more from those days wouldn't handle -R anyways,
and hopefully nobody is still running 4.0.0...

ref: git://github.com/freebsd/freebsd.git

> > This also prepares us for introducing a run-time config knob to
> > override the build-time environment in the next commit.
> 
> This is now gone, judging from 1/1 on the subject line being not
> 1/2, right?

Oops, yes :x

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

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 01, 2016 at 04:03:40PM -0700, Junio C Hamano wrote:
>
>> Eric Wong <e@80x24.org> writes:
>> 
>> > From: Junio C Hamano <gitster@pobox.com>
>> >
>> > 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).
>> 
>> Thanks for resurrecting, but I am not sure what "a better default"
>> is from the above description and with the patch.  Even though a
>> naive reading of the above (i.e. "less" and "more" are the same)
>> makes me expect that the patch will give the same set of default
>> environment settings to those on FreeBSD, you give LESS=FRX and
>> MORE=-R, i.e. they are configured differently.
>
> I wondered about this, too. They are the same binary, but calling less
> as "more" (or setting LESS_IS_MORE) causes less to behave "like more".

I guessed that, but if that is the case, "more is the same binary"
is irrelevant.  "more" behaves differently from "less" might be, but
what "less" does is much less important than "more needs this default
setting to work pleasantly", which is what is missing.

So I'd say

    Allowing PAGER_ENV to be set at build-time allows us to move
    pager-specific knowledge out of our build.  This allows us to
    set a better default for FreeBSD more(1), which misbehaves if
    MORE environment variable is left empty $in_such_and_such_way,
    by defaulting it to -R.

without even mentioning anything about "less" may be a more
understandable justification for a patch that sets MORE only on
FreeBSD.

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Junio C Hamano]

Eric Wong <e@80x24.org> writes:

> So v3 will be MORE=FRX, as less was added:
>
>     commit 98170c0c3ba86eb1cc975e7848d075bf2abc1ed0
>     Author: ps <ps@FreeBSD.org>
>     Date:   Mon May 22 10:00:00 2000 +0000
>
> 	bmake glue for less.
>
> and more was nuked:
>
>     commit cde9059fa3e4dc7e259c3864d7536252a5c580a0
>     Author: ps <ps@FreeBSD.org>
>     Date:   Mon May 29 13:31:51 2000 +0000
>
> 	Nuke more from the repository.
>
> And "git branch -r --contains" on both of those commits says
> they showed up in the 5.0 release.  However, further
> investigation says more was even gone by the 4.1.0 release
>
>   git show origin/release/4.1.0:usr.bin/more # non-existent tree
>   git show origin/release/4.0.0:usr.bin/more # tree still exists
>
> But, "git show origin/release/4.0.0:usr.bin/more/option.c"
> reveals more from those days wouldn't handle -R anyways,
> and hopefully nobody is still running 4.0.0...

OK.  And they can always set $MORE on their own to work it around,
or we can do a release-dependent tweak when somebody screams.

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Jeff King]

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

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 01, 2016 at 09:49:37PM +0000, Eric Wong wrote:
>
> You'd want:
>
>   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".

All bugs are from my original, I think.  Here is a proposed squash.

 * This shouldn't make much difference for @@PAGER_ENV@@, but not
   quoting the default assignment, i.e.

   	: "${VAR=VAL}" && export VAR

   is bad manners.  cf. 01247e02 (sh-setup: enclose setting of
   ${VAR=default} in double-quotes, 2016-06-19)

 * Again, this shouldn't make much difference for PAGER_ENV, but
   let's follow the "reset per iteration, release at the end"
   pattern to avoid repeated allocation.

 * Let's avoid a hand-rolled "skip blanks" loop and let strspn() do
   it.


 git-sh-setup.sh |  2 +-
 pager.c         | 15 ++++++---------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index b6b75e9..cda32d0 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -163,7 +163,7 @@ git_pager() {
 	for vardef in @@PAGER_ENV@@
 	do
 		var=${vardef%%=*}
-		eval ": \${$vardef} && export $var"
+		eval ": \"\${$vardef}\" && export $var"
 	done
 
 	eval "$GIT_PAGER" '"$@"'
diff --git a/pager.c b/pager.c
index cd1ac54..7199c31 100644
--- a/pager.c
+++ b/pager.c
@@ -66,25 +66,22 @@ const char *git_pager(int stdout_is_tty)
 static void setup_pager_env(struct argv_array *env)
 {
 	const char *pager_env = PAGER_ENV;
+	struct strbuf buf = STRBUF_INIT;
 
 	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));
-		}
+		if (!getenv(buf.buf))
+			argv_array_pushf(env, "%.*s",
+					 (int)(cp - pager_env), pager_env);
+		pager_env = cp + strspn(cp, " ");
 		strbuf_reset(&buf);
-		while (*cp && *cp == ' ')
-			cp++;
-		pager_env = cp;
 	}
+	strbuf_release(&buf);
 }
 
 void prepare_pager_args(struct child_process *pager_process, const char *pager)

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> All bugs are from my original, I think.  Here is a proposed squash.

Thanks, I'll take the git-sh-setup changes.

I actually just rewrote setup_pager_env using split_cmdline and
eliminated all the scary (to me) pointer arithmetic and avoided
strbuf, too.

Unfortunately, my Internet connection on that machine dropped
before I could finish testing and pushing :<

This was also the machine running http://czquwvybam4bgbro.onion/git/ :<

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Jeff King]

On Wed, Aug 03, 2016 at 01:57:09PM -0700, Junio C Hamano wrote:

> All bugs are from my original, I think.  Here is a proposed squash.
> 
>  * This shouldn't make much difference for @@PAGER_ENV@@, but not
>    quoting the default assignment, i.e.
> 
>    	: "${VAR=VAL}" && export VAR
> 
>    is bad manners.  cf. 01247e02 (sh-setup: enclose setting of
>    ${VAR=default} in double-quotes, 2016-06-19)
> 
>  * Again, this shouldn't make much difference for PAGER_ENV, but
>    let's follow the "reset per iteration, release at the end"
>    pattern to avoid repeated allocation.
> 
>  * Let's avoid a hand-rolled "skip blanks" loop and let strspn() do
>    it.

Sounds good.

> diff --git a/pager.c b/pager.c
> index cd1ac54..7199c31 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -66,25 +66,22 @@ const char *git_pager(int stdout_is_tty)
>  static void setup_pager_env(struct argv_array *env)
>  {
>  	const char *pager_env = PAGER_ENV;
> +	struct strbuf buf = STRBUF_INIT;
>  
>  	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);

I thought you'd need a strbuf_reset() here, but I guess it is handled by
the one at the end of the loop. That's OK because there are no other
loop exits, though IMHO putting it at the top makes the reusable-strbuf
idiom easier to understand.

-Peff

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Junio C Hamano]

Eric Wong <e@80x24.org> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>> All bugs are from my original, I think.  Here is a proposed squash.
>
> Thanks, I'll take the git-sh-setup changes.
>
> I actually just rewrote setup_pager_env using split_cmdline and
> eliminated all the scary (to me) pointer arithmetic and avoided
> strbuf, too.

I actually do not have much faith in split_cmdline() in that I
cannot quite read and follow the flow of the logic in its
implementation, but it already is used widely so it must be OK, so I
am fine if you use it as a black-box to make this code simpler ;-)

I'll drop the squash then and replace it with your version when it
comes.

Thanks.

[PATCH v3] pager: move pager-specific setup into the build

[Eric Wong]

From: Junio C Hamano <gitster@pobox.com>

Allowing PAGER_ENV to be set at build-time allows us to move
pager-specific knowledge out of our build.  This allows us to
set a better default for FreeBSD more(1), which misbehaves if
MORE environment variable is left empty, but accepts the same
variables as less(1)

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

Signed-off-by: Eric Wong <e@80x24.org>
---
  v3 changes (from v2, d3aed319c9abac006060bc81e865c93ff8363066)
  - Squashed git-sh-setup quoting fix from Junio
  - set MORE=FRX on FreeBSD to match LESS
  - simplify setup_pager_env to avoid leaks using split_cmdline

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

  for you to fetch changes up to 18c69fb16d866f42b53242142229de4801964d37:

    pager: move pager-specific setup into the build (2016-08-04 03:17:16 +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          | 32 ++++++++++++++++++++++++++++----
   4 files changed, 53 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..b232908 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=FRX
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 0c34aa6..a8a4576 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..6470b81 100644
--- a/pager.c
+++ b/pager.c
@@ -63,14 +63,38 @@ const char *git_pager(int stdout_is_tty)
 	return pager;
 }
 
+static void setup_pager_env(struct argv_array *env)
+{
+	const char **argv;
+	int i;
+	char *pager_env = xstrdup(PAGER_ENV);
+	int n = split_cmdline(pager_env, &argv);
+
+	if (n < 0)
+		die("malformed build-time PAGER_ENV: %s",
+			split_cmdline_strerror(n));
+
+	for (i = 0; i < n; i++) {
+		char *cp = strchr(argv[i], '=');
+
+		if (!cp)
+			die("malformed build-time PAGER_ENV");
+
+		*cp = '\0';
+		if (!getenv(argv[i])) {
+			*cp = '=';
+			argv_array_push(env, argv[i]);
+		}
+	}
+	free(pager_env);
+	free(argv);
+}
+
 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

Re: [PATCH v3] pager: move pager-specific setup into the build

[Jeff King]

On Thu, Aug 04, 2016 at 03:43:01AM +0000, Eric Wong wrote:

> +PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
> +PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
> +BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'

Here we set up CQ_SQ, but there is no PAGER_ENV_SQ.

And then...

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

Here we depend on writing PAGER_ENV_SQ, which will be blank (and
git-sh-setup is broken as a result).

> diff --git a/config.mak.uname b/config.mak.uname
> index 17fed2f..b232908 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=FRX
>  endif

Is it worth setting up PAGER_ENV's default values before including
config.mak.*, and then using "+=" here? That avoids this line getting
out of sync with the usual defaults.

> +static void setup_pager_env(struct argv_array *env)
> +{

I know you said you don't like string parsing in C. Here is a patch (on
top of yours) that converts the parsing to shell, and generates a
pre-built array-of-struct (this is similar to the big series I posted
long ago, but just touching this one spot, not invading the whole
Makefile). Feel free to ignore it as over-engineered, but I thought I'd
throw it out there in case it appeals.

-- >8 --
diff --git a/.gitignore b/.gitignore
index 05cb58a..b0fd5ec 100644
--- a/.gitignore
+++ b/.gitignore
@@ -180,6 +180,7 @@
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
 /common-cmds.h
+/pager-env.h
 *.tar.gz
 *.dsc
 *.deb
diff --git a/Makefile b/Makefile
index 0b36b5e..2e009cd 100644
--- a/Makefile
+++ b/Makefile
@@ -680,6 +680,7 @@ XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
+GENERATED_H += pager-env.h
 
 LIB_H = $(shell $(FIND) . \
 	-name .git -prune -o \
@@ -1641,9 +1642,7 @@ 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)'
+PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
 
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
@@ -1767,6 +1766,10 @@ common-cmds.h: generate-cmdlist.sh command-list.txt
 common-cmds.h: $(wildcard Documentation/git-*.txt)
 	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ && mv $@+ $@
 
+pager-env.h: generate-pager-env.sh GIT-BUILD-OPTIONS
+	$(QUIET_GEN)$(SHELL_PATH) ./generate-pager-env.sh '$(PAGER_ENV_SQ)' >$@+ && \
+	mv $@+ $@
+
 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):$(PAGER_ENV)
diff --git a/generate-pager-env.sh b/generate-pager-env.sh
new file mode 100644
index 0000000..5b67b59
--- /dev/null
+++ b/generate-pager-env.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+c_quote () {
+	printf '%s' "$@" | sed 's/\\/\\\\/g; s/"/\\"/g;'
+}
+
+cat <<\EOF &&
+#ifndef PAGER_ENV_H
+#define PAGER_ENV_H
+
+static struct pager_env {
+	const char *key;
+	const char *value;
+} pager_env[] = {
+EOF
+
+# $* does whitespace splitting
+for i in $*; do
+	key=${i%%=*}
+	value=${i#*=}
+	printf '\t{ "%s", "%s" },\n' "$(c_quote "$key")" "$(c_quote "$value")"
+done &&
+
+cat <<\EOF
+};
+
+#endif /* PAGER_ENV */
+EOF
diff --git a/pager.c b/pager.c
index 6470b81..03c1a4a 100644
--- a/pager.c
+++ b/pager.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "pager-env.h"
 
 #ifndef DEFAULT_PAGER
 #define DEFAULT_PAGER "less"
@@ -65,29 +66,12 @@ const char *git_pager(int stdout_is_tty)
 
 static void setup_pager_env(struct argv_array *env)
 {
-	const char **argv;
 	int i;
-	char *pager_env = xstrdup(PAGER_ENV);
-	int n = split_cmdline(pager_env, &argv);
-
-	if (n < 0)
-		die("malformed build-time PAGER_ENV: %s",
-			split_cmdline_strerror(n));
-
-	for (i = 0; i < n; i++) {
-		char *cp = strchr(argv[i], '=');
-
-		if (!cp)
-			die("malformed build-time PAGER_ENV");
-
-		*cp = '\0';
-		if (!getenv(argv[i])) {
-			*cp = '=';
-			argv_array_push(env, argv[i]);
-		}
+	for (i = 0; i < ARRAY_SIZE(pager_env); i++) {
+		struct pager_env *p = &pager_env[i];
+		if (!getenv(p->key))
+			argv_array_pushf(env, "%s=%s", p->key, p->value);
 	}
-	free(pager_env);
-	free(argv);
 }
 
 void prepare_pager_args(struct child_process *pager_process, const char *pager)

Re: [PATCH v3] pager: move pager-specific setup into the build

[Eric Wong]

Jeff King <peff@peff.net> wrote:
> On Thu, Aug 04, 2016 at 03:43:01AM +0000, Eric Wong wrote:
> 
> > +PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
> > +PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
> > +BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
> 
> Here we set up CQ_SQ, but there is no PAGER_ENV_SQ.
> 
> And then...

> > @@ -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
> 
> Here we depend on writing PAGER_ENV_SQ, which will be blank (and
> git-sh-setup is broken as a result).

Good catch!  And the reason we didn't notice git-sh-setup is
broken is nobody uses git_pager in-tree from that, anymore.
However, I suspect we'll have to support it indefinitely due to
custom scripts and contrib/examples.

Made the following change for v4:

diff --git a/Makefile b/Makefile
index 0b36b5e..fc9b017 100644
--- a/Makefile
+++ b/Makefile
@@ -1641,6 +1641,7 @@ ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
 endif
 
+PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
 PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
 PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
 BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index e4fc5c8..c8dc665 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -49,6 +49,19 @@ test_expect_success TTY 'LESS and LV envvars are set for pagination' '
 	grep ^LV= pager-env.out
 '
 
+test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' '
+	(
+		sane_unset LESS LV &&
+		PAGER="env >pager-env.out; wc" &&
+		export PAGER &&
+		PATH="$(git --exec-path):$PATH" &&
+		export PATH &&
+		test_terminal sh -c ". git-sh-setup && git_pager"
+	) &&
+	grep ^LESS= pager-env.out &&
+	grep ^LV= pager-env.out
+'
+
 test_expect_success TTY 'some commands do not use a pager' '
 	rm -f paginated.out &&
 	test_terminal git rev-list HEAD &&

> > --- 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=FRX
> >  endif
> 
> Is it worth setting up PAGER_ENV's default values before including
> config.mak.*, and then using "+=" here? That avoids this line getting
> out of sync with the usual defaults.

Good point, but it makes ordering problematic for folks
who want to override it config.mak or command-line.

We may have to do something like we do for BASIC_CFLAGS and
such, but I'm not sure it's worth the effort when somebody
doesn't wants a different value for one of the flags.

> > +static void setup_pager_env(struct argv_array *env)
> > +{
> 
> I know you said you don't like string parsing in C. Here is a patch (on
> top of yours) that converts the parsing to shell, and generates a
> pre-built array-of-struct (this is similar to the big series I posted
> long ago, but just touching this one spot, not invading the whole
> Makefile). Feel free to ignore it as over-engineered, but I thought I'd
> throw it out there in case it appeals.

Yeah, but I'd rather not introduce more complexity into the
build process, either (unless it's a performance-sensitive part,
which this is not).  Also, while my original 2/2 to make it
configurable at runtime was discarded, I wouldn't rule out
somebody making a compelling case for it and it would be
an easier change from the parse-at-runtime code.

[PATCH v4] pager: move pager-specific setup into the build

[Eric Wong]

Allowing PAGER_ENV to be set at build-time allows us to move
pager-specific knowledge out of our build.  This allows us to
set a better default for FreeBSD more(1), which pretends not to
understand ANSI color escapes if the MORE environment variable
is left empty, but accepts the same variables as less(1)

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

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Eric Wong <e@80x24.org>
---
  v4 changes: (diff @ <20160804113410.GA13908@starla>)
  - reworded commit messages and took ownership as suggested
    privately by Junio
  - fixed git-sh-setup and add test for untested git_pager

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

  for you to fetch changes up to 3b8e70c37e96b4e19475fb6dc480a82b292bd28f:

    pager: move pager-specific setup into the build (2016-08-04 11:31:28 +0000)

  ----------------------------------------------------------------
  Eric Wong (1):
        pager: move pager-specific setup into the build

 Makefile         | 21 ++++++++++++++++++++-
 config.mak.uname |  1 +
 git-sh-setup.sh  |  8 +++++---
 pager.c          | 32 ++++++++++++++++++++++++++++----
 t/t7006-pager.sh | 13 +++++++++++++
 5 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 6a13386..fc9b017 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,11 @@ ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
 endif
 
+PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
+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 +1770,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 +1783,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 +2191,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..b232908 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=FRX
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 0c34aa6..a8a4576 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..6470b81 100644
--- a/pager.c
+++ b/pager.c
@@ -63,14 +63,38 @@ const char *git_pager(int stdout_is_tty)
 	return pager;
 }
 
+static void setup_pager_env(struct argv_array *env)
+{
+	const char **argv;
+	int i;
+	char *pager_env = xstrdup(PAGER_ENV);
+	int n = split_cmdline(pager_env, &argv);
+
+	if (n < 0)
+		die("malformed build-time PAGER_ENV: %s",
+			split_cmdline_strerror(n));
+
+	for (i = 0; i < n; i++) {
+		char *cp = strchr(argv[i], '=');
+
+		if (!cp)
+			die("malformed build-time PAGER_ENV");
+
+		*cp = '\0';
+		if (!getenv(argv[i])) {
+			*cp = '=';
+			argv_array_push(env, argv[i]);
+		}
+	}
+	free(pager_env);
+	free(argv);
+}
+
 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)
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index e4fc5c8..c8dc665 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -49,6 +49,19 @@ test_expect_success TTY 'LESS and LV envvars are set for pagination' '
 	grep ^LV= pager-env.out
 '
 
+test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' '
+	(
+		sane_unset LESS LV &&
+		PAGER="env >pager-env.out; wc" &&
+		export PAGER &&
+		PATH="$(git --exec-path):$PATH" &&
+		export PATH &&
+		test_terminal sh -c ". git-sh-setup && git_pager"
+	) &&
+	grep ^LESS= pager-env.out &&
+	grep ^LV= pager-env.out
+'
+
 test_expect_success TTY 'some commands do not use a pager' '
 	rm -f paginated.out &&
 	test_terminal git rev-list HEAD &&
-- 
EW

Re: [PATCH v3] pager: move pager-specific setup into the build

[Jeff King]

On Thu, Aug 04, 2016 at 11:34:10AM +0000, Eric Wong wrote:

> > > --- 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=FRX
> > >  endif
> > 
> > Is it worth setting up PAGER_ENV's default values before including
> > config.mak.*, and then using "+=" here? That avoids this line getting
> > out of sync with the usual defaults.
> 
> Good point, but it makes ordering problematic for folks
> who want to override it config.mak or command-line.

I'm not sure it changes much for them. Their "=" in config.mak, etc,
would override our default, and anything on the command line overrides
all of the in-Makefile stuff anyway. The only difference would be if
they use "+=" in config.mak, but there I think it would be an
improvement.

I'm OK to leave it as-is until somebody actually cares, though.

> > I know you said you don't like string parsing in C. Here is a patch (on
> > top of yours) that converts the parsing to shell, and generates a
> > pre-built array-of-struct (this is similar to the big series I posted
> > long ago, but just touching this one spot, not invading the whole
> > Makefile). Feel free to ignore it as over-engineered, but I thought I'd
> > throw it out there in case it appeals.
> 
> Yeah, but I'd rather not introduce more complexity into the
> build process, either (unless it's a performance-sensitive part,
> which this is not).  Also, while my original 2/2 to make it
> configurable at runtime was discarded, I wouldn't rule out
> somebody making a compelling case for it and it would be
> an easier change from the parse-at-runtime code.

Yeah, I had similar thoughts while writing it.

Your v4 patch looks fine to me.

-Peff