* [PATCH 0/2] add PAGER_ENV to build and core.pagerEnv to config @ 2016-08-01 1:05 Eric Wong 2016-08-01 1:05 ` [PATCH 1/2] pager: move pager-specific setup into the build Eric Wong ` (2 more replies) 0 siblings, 3 replies; 32+ messages in thread From: Eric Wong @ 2016-08-01 1:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Kyle J. McKay, git, 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(-) ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 1:05 [PATCH 0/2] add PAGER_ENV to build and core.pagerEnv to config Eric Wong @ 2016-08-01 1:05 ` Eric Wong 2016-08-01 1:43 ` brian m. carlson 2016-08-01 17:46 ` Duy Nguyen 2016-08-01 1:05 ` [PATCH 2/2] pager: implement core.pagerEnv in config Eric Wong 2016-08-01 21:49 ` [PATCH 0/1 v2] add PAGER_ENV to build Eric Wong 2 siblings, 2 replies; 32+ messages in thread From: Eric Wong @ 2016-08-01 1:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Kyle J. McKay, git, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 1:05 ` [PATCH 1/2] pager: move pager-specific setup into the build Eric Wong @ 2016-08-01 1:43 ` brian m. carlson 2016-08-01 7:00 ` Eric Wong 2016-08-01 17:24 ` Jeff King 2016-08-01 17:46 ` Duy Nguyen 1 sibling, 2 replies; 32+ messages in thread From: brian m. carlson @ 2016-08-01 1:43 UTC (permalink / raw) To: Eric Wong; +Cc: Junio C Hamano, Jeff King, Kyle J. McKay, git [-- 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 --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 1:43 ` brian m. carlson @ 2016-08-01 7:00 ` Eric Wong 2016-08-01 8:57 ` Jakub Narębski 2016-08-01 17:24 ` Jeff King 1 sibling, 1 reply; 32+ messages in thread From: Eric Wong @ 2016-08-01 7:00 UTC (permalink / raw) To: brian m. carlson; +Cc: Junio C Hamano, Jeff King, Kyle J. McKay, git "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)... ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 7:00 ` Eric Wong @ 2016-08-01 8:57 ` Jakub Narębski 2016-08-01 10:40 ` brian m. carlson 0 siblings, 1 reply; 32+ messages in thread From: Jakub Narębski @ 2016-08-01 8:57 UTC (permalink / raw) To: Eric Wong, brian m. carlson; +Cc: Junio C Hamano, Jeff King, Kyle J. McKay, git 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 8:57 ` Jakub Narębski @ 2016-08-01 10:40 ` brian m. carlson 0 siblings, 0 replies; 32+ messages in thread From: brian m. carlson @ 2016-08-01 10:40 UTC (permalink / raw) To: Jakub Narębski Cc: Eric Wong, Junio C Hamano, Jeff King, Kyle J. McKay, git [-- 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 --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 1:43 ` brian m. carlson 2016-08-01 7:00 ` Eric Wong @ 2016-08-01 17:24 ` Jeff King 2016-08-01 18:07 ` Junio C Hamano 1 sibling, 1 reply; 32+ messages in thread From: Jeff King @ 2016-08-01 17:24 UTC (permalink / raw) To: brian m. carlson, Eric Wong, Junio C Hamano, Kyle J. McKay, git 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 17:24 ` Jeff King @ 2016-08-01 18:07 ` Junio C Hamano 0 siblings, 0 replies; 32+ messages in thread From: Junio C Hamano @ 2016-08-01 18:07 UTC (permalink / raw) To: Jeff King; +Cc: brian m. carlson, Eric Wong, Kyle J. McKay, git 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 1:05 ` [PATCH 1/2] pager: move pager-specific setup into the build Eric Wong 2016-08-01 1:43 ` brian m. carlson @ 2016-08-01 17:46 ` Duy Nguyen 2016-08-01 17:52 ` Jeff King 1 sibling, 1 reply; 32+ messages in thread From: Duy Nguyen @ 2016-08-01 17:46 UTC (permalink / raw) To: Eric Wong; +Cc: Junio C Hamano, Jeff King, Kyle J. McKay, Git Mailing List 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 17:46 ` Duy Nguyen @ 2016-08-01 17:52 ` Jeff King 2016-08-01 18:01 ` Duy Nguyen 0 siblings, 1 reply; 32+ messages in thread From: Jeff King @ 2016-08-01 17:52 UTC (permalink / raw) To: Duy Nguyen; +Cc: Eric Wong, Junio C Hamano, Kyle J. McKay, Git Mailing List 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 17:52 ` Jeff King @ 2016-08-01 18:01 ` Duy Nguyen 2016-08-01 18:07 ` Jeff King 0 siblings, 1 reply; 32+ messages in thread From: Duy Nguyen @ 2016-08-01 18:01 UTC (permalink / raw) To: Jeff King; +Cc: Eric Wong, Junio C Hamano, Kyle J. McKay, Git Mailing List 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] pager: move pager-specific setup into the build 2016-08-01 18:01 ` Duy Nguyen @ 2016-08-01 18:07 ` Jeff King 0 siblings, 0 replies; 32+ messages in thread From: Jeff King @ 2016-08-01 18:07 UTC (permalink / raw) To: Duy Nguyen; +Cc: Eric Wong, Junio C Hamano, Kyle J. McKay, Git Mailing List 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 2/2] pager: implement core.pagerEnv in config 2016-08-01 1:05 [PATCH 0/2] add PAGER_ENV to build and core.pagerEnv to config Eric Wong 2016-08-01 1:05 ` [PATCH 1/2] pager: move pager-specific setup into the build Eric Wong @ 2016-08-01 1:05 ` Eric Wong 2016-08-01 17:28 ` Jeff King 2016-08-01 21:49 ` [PATCH 0/1 v2] add PAGER_ENV to build Eric Wong 2 siblings, 1 reply; 32+ messages in thread From: Eric Wong @ 2016-08-01 1:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Kyle J. McKay, git, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] pager: implement core.pagerEnv in config 2016-08-01 1:05 ` [PATCH 2/2] pager: implement core.pagerEnv in config Eric Wong @ 2016-08-01 17:28 ` Jeff King 0 siblings, 0 replies; 32+ messages in thread From: Jeff King @ 2016-08-01 17:28 UTC (permalink / raw) To: Eric Wong; +Cc: Junio C Hamano, Kyle J. McKay, git 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 0/1 v2] add PAGER_ENV to build 2016-08-01 1:05 [PATCH 0/2] add PAGER_ENV to build and core.pagerEnv to config Eric Wong 2016-08-01 1:05 ` [PATCH 1/2] pager: move pager-specific setup into the build Eric Wong 2016-08-01 1:05 ` [PATCH 2/2] pager: implement core.pagerEnv in config Eric Wong @ 2016-08-01 21:49 ` Eric Wong 2016-08-01 21:49 ` [PATCH 1/1 v2] pager: move pager-specific setup into the build Eric Wong 2016-08-01 21:59 ` [PATCH 0/1 v2] add PAGER_ENV to build Jeff King 2 siblings, 2 replies; 32+ messages in thread From: Eric Wong @ 2016-08-01 21:49 UTC (permalink / raw) To: Junio C Hamano Cc: Eric Wong, Jeff King, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-01 21:49 ` [PATCH 0/1 v2] add PAGER_ENV to build Eric Wong @ 2016-08-01 21:49 ` Eric Wong 2016-08-01 23:03 ` Junio C Hamano 2016-08-03 16:19 ` Jeff King 2016-08-01 21:59 ` [PATCH 0/1 v2] add PAGER_ENV to build Jeff King 1 sibling, 2 replies; 32+ messages in thread From: Eric Wong @ 2016-08-01 21:49 UTC (permalink / raw) To: Junio C Hamano Cc: Eric Wong, Jeff King, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-01 21:49 ` [PATCH 1/1 v2] pager: move pager-specific setup into the build Eric Wong @ 2016-08-01 23:03 ` Junio C Hamano 2016-08-01 23:46 ` Jeff King 2016-08-01 23:56 ` Eric Wong 2016-08-03 16:19 ` Jeff King 1 sibling, 2 replies; 32+ messages in thread From: Junio C Hamano @ 2016-08-01 23:03 UTC (permalink / raw) To: Eric Wong Cc: Jeff King, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-01 23:03 ` Junio C Hamano @ 2016-08-01 23:46 ` Jeff King 2016-08-02 21:14 ` Junio C Hamano 2016-08-01 23:56 ` Eric Wong 1 sibling, 1 reply; 32+ messages in thread From: Jeff King @ 2016-08-01 23:46 UTC (permalink / raw) To: Junio C Hamano Cc: Eric Wong, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-01 23:46 ` Jeff King @ 2016-08-02 21:14 ` Junio C Hamano 0 siblings, 0 replies; 32+ messages in thread From: Junio C Hamano @ 2016-08-02 21:14 UTC (permalink / raw) To: Jeff King Cc: Eric Wong, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-01 23:03 ` Junio C Hamano 2016-08-01 23:46 ` Jeff King @ 2016-08-01 23:56 ` Eric Wong 2016-08-02 21:15 ` Junio C Hamano 1 sibling, 1 reply; 32+ messages in thread From: Eric Wong @ 2016-08-01 23:56 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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/ ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-01 23:56 ` Eric Wong @ 2016-08-02 21:15 ` Junio C Hamano 0 siblings, 0 replies; 32+ messages in thread From: Junio C Hamano @ 2016-08-02 21:15 UTC (permalink / raw) To: Eric Wong Cc: Jeff King, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-01 21:49 ` [PATCH 1/1 v2] pager: move pager-specific setup into the build Eric Wong 2016-08-01 23:03 ` Junio C Hamano @ 2016-08-03 16:19 ` Jeff King 2016-08-03 20:57 ` Junio C Hamano 1 sibling, 1 reply; 32+ messages in thread From: Jeff King @ 2016-08-03 16:19 UTC (permalink / raw) To: Eric Wong Cc: Junio C Hamano, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-03 16:19 ` Jeff King @ 2016-08-03 20:57 ` Junio C Hamano 2016-08-03 21:08 ` Eric Wong 2016-08-03 21:09 ` [PATCH 1/1 v2] " Jeff King 0 siblings, 2 replies; 32+ messages in thread From: Junio C Hamano @ 2016-08-03 20:57 UTC (permalink / raw) To: Jeff King Cc: Eric Wong, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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) ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-03 20:57 ` Junio C Hamano @ 2016-08-03 21:08 ` Eric Wong 2016-08-03 21:15 ` Junio C Hamano 2016-08-03 21:09 ` [PATCH 1/1 v2] " Jeff King 1 sibling, 1 reply; 32+ messages in thread From: Eric Wong @ 2016-08-03 21:08 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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/ :< ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-03 21:08 ` Eric Wong @ 2016-08-03 21:15 ` Junio C Hamano 2016-08-04 3:43 ` [PATCH v3] " Eric Wong 0 siblings, 1 reply; 32+ messages in thread From: Junio C Hamano @ 2016-08-03 21:15 UTC (permalink / raw) To: Eric Wong Cc: Jeff King, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v3] pager: move pager-specific setup into the build 2016-08-03 21:15 ` Junio C Hamano @ 2016-08-04 3:43 ` Eric Wong 2016-08-04 5:34 ` Jeff King 2016-08-04 11:40 ` [PATCH v4] " Eric Wong 0 siblings, 2 replies; 32+ messages in thread From: Eric Wong @ 2016-08-04 3:43 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v3] pager: move pager-specific setup into the build 2016-08-04 3:43 ` [PATCH v3] " Eric Wong @ 2016-08-04 5:34 ` Jeff King 2016-08-04 11:34 ` Eric Wong 2016-08-04 11:40 ` [PATCH v4] " Eric Wong 1 sibling, 1 reply; 32+ messages in thread From: Jeff King @ 2016-08-04 5:34 UTC (permalink / raw) To: Eric Wong Cc: Junio C Hamano, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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) ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v3] pager: move pager-specific setup into the build 2016-08-04 5:34 ` Jeff King @ 2016-08-04 11:34 ` Eric Wong 2016-08-04 17:53 ` Jeff King 0 siblings, 1 reply; 32+ messages in thread From: Eric Wong @ 2016-08-04 11:34 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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. ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v3] pager: move pager-specific setup into the build 2016-08-04 11:34 ` Eric Wong @ 2016-08-04 17:53 ` Jeff King 0 siblings, 0 replies; 32+ messages in thread From: Jeff King @ 2016-08-04 17:53 UTC (permalink / raw) To: Eric Wong Cc: Junio C Hamano, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v4] pager: move pager-specific setup into the build 2016-08-04 3:43 ` [PATCH v3] " Eric Wong 2016-08-04 5:34 ` Jeff King @ 2016-08-04 11:40 ` Eric Wong 1 sibling, 0 replies; 32+ messages in thread From: Eric Wong @ 2016-08-04 11:40 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build 2016-08-03 20:57 ` Junio C Hamano 2016-08-03 21:08 ` Eric Wong @ 2016-08-03 21:09 ` Jeff King 1 sibling, 0 replies; 32+ messages in thread From: Jeff King @ 2016-08-03 21:09 UTC (permalink / raw) To: Junio C Hamano Cc: Eric Wong, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/1 v2] add PAGER_ENV to build 2016-08-01 21:49 ` [PATCH 0/1 v2] add PAGER_ENV to build Eric Wong 2016-08-01 21:49 ` [PATCH 1/1 v2] pager: move pager-specific setup into the build Eric Wong @ 2016-08-01 21:59 ` Jeff King 1 sibling, 0 replies; 32+ messages in thread From: Jeff King @ 2016-08-01 21:59 UTC (permalink / raw) To: Eric Wong Cc: Junio C Hamano, Kyle J. McKay, git, brian m. carlson, Nguyễn Thái Ngọc Duy, Jakub Narębski 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2016-08-04 17:53 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-01 1:05 [PATCH 0/2] add PAGER_ENV to build and core.pagerEnv to config Eric Wong 2016-08-01 1:05 ` [PATCH 1/2] pager: move pager-specific setup into the build Eric Wong 2016-08-01 1:43 ` brian m. carlson 2016-08-01 7:00 ` Eric Wong 2016-08-01 8:57 ` Jakub Narębski 2016-08-01 10:40 ` brian m. carlson 2016-08-01 17:24 ` Jeff King 2016-08-01 18:07 ` Junio C Hamano 2016-08-01 17:46 ` Duy Nguyen 2016-08-01 17:52 ` Jeff King 2016-08-01 18:01 ` Duy Nguyen 2016-08-01 18:07 ` Jeff King 2016-08-01 1:05 ` [PATCH 2/2] pager: implement core.pagerEnv in config Eric Wong 2016-08-01 17:28 ` Jeff King 2016-08-01 21:49 ` [PATCH 0/1 v2] add PAGER_ENV to build Eric Wong 2016-08-01 21:49 ` [PATCH 1/1 v2] pager: move pager-specific setup into the build Eric Wong 2016-08-01 23:03 ` Junio C Hamano 2016-08-01 23:46 ` Jeff King 2016-08-02 21:14 ` Junio C Hamano 2016-08-01 23:56 ` Eric Wong 2016-08-02 21:15 ` Junio C Hamano 2016-08-03 16:19 ` Jeff King 2016-08-03 20:57 ` Junio C Hamano 2016-08-03 21:08 ` Eric Wong 2016-08-03 21:15 ` Junio C Hamano 2016-08-04 3:43 ` [PATCH v3] " Eric Wong 2016-08-04 5:34 ` Jeff King 2016-08-04 11:34 ` Eric Wong 2016-08-04 17:53 ` Jeff King 2016-08-04 11:40 ` [PATCH v4] " Eric Wong 2016-08-03 21:09 ` [PATCH 1/1 v2] " Jeff King 2016-08-01 21:59 ` [PATCH 0/1 v2] add PAGER_ENV to build Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).