Re: [PATCH 2/3] setup_pager: set MORE=R
On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote: does complicate the point of my series, which was to add more intimate logic about how we handle LESS. ... return !x || strchr(x, 'R'); [...] I am not sure if it is even a good idea for us to have so intimate logic for various pagers in the first place. I'd seriously wonder if it is better to take this position: A platform packager who sets the default pager and/or the default environment for the pager at the build time, or an individual user who tells your Git what pager you want to use and/or with what setting that pager should be run under with environment variables. These people ought to know far better than Git what their specific choices do. Do not try to second-guess them. Sorry for letting this topic lapse; I wanted to look at some possible Makefile improvements, which I'll be posting in a moment. I did want to address this point, though. If we are just talking about packagers and defaults (e.g., setting MORE=R on FreeBSD), then I agree that the right tool for the job is empowering the packagers, and the Makefile knob we've discussed does that. But there's another set of people that I was intending to help with the patch, which is people that have set up LESS, and did not necessarily care about the R flag in the past (e.g., for many years before git came along, I set LESS=giM, and never even knew that R existed). Since git comes out of the box these days with color and the pager turned on, that means people with such a setup see broken output from day one. And I think it is Git's fault here, not the user or the packager. Our defaults assume that the user's pager can handle color, and that is not the default for many pagers, including our default less! The user did not turn off R here; they simply set other options they cared about, and our hacky workaround to auto-enable R did not kick in. It seems a shame to me that we cannot do better for such users. However, the level of intimacy with the pager is getting to be a bit too much for my taste, and the saving grace is that not that many people set LESS themselves (and git is widely enough known that R is a common recommendation when people do). So it's probably the lesser of two evils to ignore the situation, and let people who run into it find the answers on the web. So I think there is nothing to be done. But I did want to mention it in case somebody else can come up with some clever solution. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
Jeff King p...@peff.net writes: But there's another set of people that I was intending to help with the patch, which is people that have set up LESS, and did not necessarily care about the R flag in the past (e.g., for many years before git came along, I set LESS=giM, and never even knew that R existed). Since git comes out of the box these days with color and the pager turned on, that means people with such a setup see broken output from day one. And I think it is Git's fault here, not the user or the packager. I am not particularly itnterested in whose fault it is ;-) If the user sets LESS himself, he knows how to set it (and if he is setting it automatically for all of his sessions, he knows where to do so), and would know better than Git about less, his pager of choice. If he did not know about R and did not see color, that is even better. Now he knows and his update to his LESS settings will let him view colors in outputs from programs that are not Git. So I think there is nothing to be done. But I did want to mention it in case somebody else can come up with some clever solution. :) Sure. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On Tue, Feb 04, 2014 at 02:17:57PM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: But there's another set of people that I was intending to help with the patch, which is people that have set up LESS, and did not necessarily care about the R flag in the past (e.g., for many years before git came along, I set LESS=giM, and never even knew that R existed). Since git comes out of the box these days with color and the pager turned on, that means people with such a setup see broken output from day one. And I think it is Git's fault here, not the user or the packager. I am not particularly itnterested in whose fault it is ;-) If the user sets LESS himself, he knows how to set it (and if he is setting it automatically for all of his sessions, he knows where to do so), and would know better than Git about less, his pager of choice. If he did not know about R and did not see color, that is even better. Now he knows and his update to his LESS settings will let him view colors in outputs from programs that are not Git. Right. If git just disabled the color, I think that would be sane (and that is what my patch was shooting for). But somebody who sees: $ git log ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m does not necessarily know what is going on. They do not know that it is a less problem, nor that their less settings are relevant. They only see that Git is broken out of the box. Anyway, I will leave it at that. It's an unfortunate problem, but one not worth fixing. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On 02/04/2014 14:25, Jeff King wrote: Right. If git just disabled the color, I think that would be sane (and that is what my patch was shooting for). But somebody who sees: $ git log ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m does not necessarily know what is going on. They do not know that it is a less problem, nor that their less settings are relevant. They only see that Git is broken out of the box. Maybe, instead of doing all the elaborate guess and assumption work, have configure script check if the current PAGER supports colors and build git accordingly? configure could run the pager as one of its tests, and determine if ESC appears on the output. Yuri -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On Tue, Feb 04, 2014 at 02:45:11PM -0800, Yuri wrote: On 02/04/2014 14:25, Jeff King wrote: Right. If git just disabled the color, I think that would be sane (and that is what my patch was shooting for). But somebody who sees: $ git log ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m does not necessarily know what is going on. They do not know that it is a less problem, nor that their less settings are relevant. They only see that Git is broken out of the box. Maybe, instead of doing all the elaborate guess and assumption work, have configure script check if the current PAGER supports colors and build git accordingly? configure could run the pager as one of its tests, and determine if ESC appears on the output. But this is not a build-time problem, but rather a run-time problem. We do not know what the environment of the user will be at run-time. The safest thing would be to turn off auto-color when LESS (or any of the pager environment variables) is set at all (and not worry about whether R is set; only know that _we_ are not setting it, so we cannot count on it). But that would potentially inconvenience a lot of people whose default color would suddenly go away. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
Jeff King p...@peff.net writes: The safest thing would be to turn off auto-color when LESS (or any of the pager environment variables) is set at all (and not worry about whether R is set; only know that _we_ are not setting it, so we cannot count on it). But that would potentially inconvenience a lot of people whose default color would suddenly go away. That is just as safe as disabling color for everybody, isn't it? Half of existing users have LESS with R, and the other half do not have LESS at all. The former will be harmed, the latter will not see any difference. Oh, and then new users who do not know R for LESS will not even notice that Git could support coloured output. Those among them who read manpages and find --color option will then see ESC[33m in their output and we are back to where we started X-. So I think we are already at the safest place. Those who see ESC[33m will know they are missing some good stuff and can ask around, which is better than doing anything else at this point. I think that is the same conclusion as yours, there is nothing to be done. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On 02/04/2014 14:48, Jeff King wrote: But this is not a build-time problem, but rather a run-time problem. We do not know what the environment of the user will be at run-time. Ok, git can test the pager on the given system before its first use and remember the result in ~/.git for later use for example. Such 'experimental' approach is maybe more reliable. Yuri -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On Feb 4, 2014, at 14:12, Jeff King wrote: On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote: does complicate the point of my series, which was to add more intimate logic about how we handle LESS. ... return !x || strchr(x, 'R'); [...] I am not sure if it is even a good idea for us to have so intimate logic for various pagers in the first place. I'd seriously wonder if it is better to take this position: A platform packager who sets the default pager and/or the default environment for the pager at the build time, or an individual user who tells your Git what pager you want to use and/or with what setting that pager should be run under with environment variables. These people ought to know far better than Git what their specific choices do. Do not try to second-guess them. Sorry for letting this topic lapse; I wanted to look at some possible Makefile improvements, which I'll be posting in a moment. I did want to address this point, though. If we are just talking about packagers and defaults (e.g., setting MORE=R on FreeBSD), then I agree that the right tool for the job is empowering the packagers, and the Makefile knob we've discussed does that. [snip] It seems a shame to me that we cannot do better for such users. However, the level of intimacy with the pager is getting to be a bit too much for my taste, and the saving grace is that not that many people set LESS themselves (and git is widely enough known that R is a common recommendation when people do). So it's probably the lesser of two evils to ignore the situation, and let people who run into it find the answers on the web. So I think there is nothing to be done. But I did want to mention it in case somebody else can come up with some clever solution. :) I think we can do better without adding intimate pager knowledge. And at the same time make it a simple matter of tweaking the Makefile to deal with new pagers on specific systems. There are two parts to the proposal. Part 1 Add a new configuration item which I will call pager.env for now that can have multiple values of the form ENV=value (whether multiple lines or whitespace separated on a single line to be decided later). On a system where more can support color by setting MORE=-R it might look like this: [pager] env = LESS=-FRSX LV=-c MORE=-R On a system where more does not it might look like this: [pager] env = LESS=-FRSX LV=-c The default value of pager.env is to be configured in a system- specific way (i.e. Makefile knob) at build time. Then, if Git is going to output color AND start a pager (that logic remains unchanged by this proposal), then it processes the pager.env value by examining each ENV=value setting and if the environment variable ENV is NOT already set, then sets it to value before starting the pager. This is mostly a clean up and shouldn't really be controversial since before commit e54c1f2d2 the system basically behaved like this where pager.env was always set to LESS=FRSX and after it behaves as though pager.env is always set to LESS=FRSX LV=-c. Part 2 Alongside the current false/never, always, true/auto settings for color.ui a new carefully setting is introduced and the color.ui default if not set is changed from auto (commit 4c7f1819) to the new carefully setting. Why a new setting? So that people who have explicitly set color.ui to auto (or one of the other values) will not be affected by the proposed new logic. Another new configuration item which I will call pager.carefully for now is introduced that again can have multiple values of the form name=ENV. It might look like this: [pager] carefully = less=LESS LV=lv more=MORE Again the default value of pager.carefully can be configured in a system-specific way (i.e. Makefile knob) at build time. If color.ui is set to carefully, then the logic is as follows: a) Decide whether to output color the same way as for color.ui=auto b) Select the pager the same way as for color.ui=auto c) If (a) and (b) have selected a pager AND enabled color output then check to see if the selected pager appears in pager.carefully as one of the name values (perhaps a suffix match on the first whitespace- separated word of the selected pager value). d) If the selected pager does not appear in pager.carefully, disable color output. e) If the selected pager appears in pager.carefully, but the ENV associated with it does NOT appear in pager.env, disable color output. f) If the pager appears in pager.carefully, but the ENV associated with it is already set, disable color output. g) Otherwise, go ahead with color output as the change in Part 1 above will properly set the pager's options to enable it. This has the following advantages: 1. No intimate pager knowledge. 2. Color output will be selected on supported systems by
Re: [PATCH 2/3] setup_pager: set MORE=R
Jeff King p...@peff.net writes: ..., but it feels awfully wrong to be so intimate with a subprogram that we do not control. Yeah, I think we are in agreement on that point. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On Jan 20, 2014, at 21:30, Jeff King wrote: Ugh. Having just read the LESS discussion, it makes me wonder if the strchr(getenv(LESS), 'R') check I add elsewhere in the series is sufficient. I suspect in practice it is good enough, but I would not be surprised if there is a way to fool it with a sufficiently complex LESS variable. Maybe we should just assume it is enough, and people with crazy LESS settings can set color.pager as appropriate? This attempts to show colors but doesn't work properly: LESS=-+R git -c color.ui=auto -c color.pager=true log --oneline -- graph but this does LESS=-R git -c color.ui=auto -c color.pager=true log --oneline -- graph so yeah, just checking for 'R' in $LESS is only approximate. :) Also -r is good enough to show colors too... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
Jeff King p...@peff.net writes: ... does complicate the point of my series, which was to add more intimate logic about how we handle LESS. ... return !x || strchr(x, 'R'); } [...] } but we are still hard-coding a lot of intelligence about less here. I am not sure if it is even a good idea for us to have so intimate logic for various pagers in the first place. I'd seriously wonder if it is better to take this position: A platform packager who sets the default pager and/or the default environment for the pager at the build time, or an individual user who tells your Git what pager you want to use and/or with what setting that pager should be run under with environment variables. These people ought to know far better than Git what their specific choices do. Do not try to second-guess them. The potential breakage caused by setting MORE=R unconditionally is a good example. A careless intimate logic may think that any pager that is called 'more' would behave like traditional 'more', breaking half the 'more' user population while catering to the other half. I wonder if the abstraction provided by the Makefile variable is really worthwhile. Anybody adding a new line to it would also want to tweak pager_can_handle_color to add similar logic. And that is why I am not enthused by the idea of adding such logic in the first place. I view the Makefile customization as a way for the packager to offer a sensible default for their platform without touching the code, which is slightly different from your 1. below. Taking a step back for a moment, we are getting two things out of such a Makefile variable: 1. An easy way for packager to add intelligence about common pagers on their system. 2. DRY between git-sh-setup and the C code. I do like (1), but I do not know if we want to try to encode the can handle color logic into a Makefile variable. What would it look like? For (2), an alternate method would be to simply provide git pager as C code, which spawns the appropriate pager as the C code would. Then git-sh-setup can easily build around that. And as to 2., if the answer to the other issue do we want our code to be intimately aware of pager-specific quirks, or do we just want to give packagers a knob to express their choice of the default? resolves to the former, I would think that git pager would be not just a workable alternative, but would be the only viable one. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
Jeff King p...@peff.net writes:
Perhaps instead of just dumping it into a macro, we could actually parse
it at compile time into C code, and store the result as a header file.
Then that header file becomes the proper dependency, and this run-time
error:
+while (*pager_env) {
+struct strbuf buf = STRBUF_INIT;
+const char *cp = strchrnul(pager_env, '=');
+
+if (!*cp)
+die(malformed build-time PAGER_ENV);
would become a compile-time error. We could do the same for the shell
code, too.
I'm thinking something like:
Heh, great minds think alike. I did start writing a toy-patch with
a new file called pager-env.h, before discarding it and sending a
simpler alternative which you are responding to.
I do not mind the approach at all; the primary reason I stopped
doing that was because the amount of code to get quotes right turned
out to be sufficiently big to obscure the real point of the how
about doing it this way illustration patch.
diff --git a/Makefile b/Makefile
index b4af1e2..3a8d15e 100644
--- a/Makefile
+++ b/Makefile
@@ -2182,6 +2182,16 @@ GIT-LDFLAGS: FORCE
echo $$FLAGS GIT-LDFLAGS; \
fi
+GIT-PAGER-ENV:
+ @PAGER_ENV='$(PAGER_ENV)'; \
+ if test x$$PAGER_ENV != x`cat GIT-PAGER-ENV 2/dev/null`; then \
+ echo $$PAGER_ENV GIT-PAGER-ENV; \
+ fi
+
+pager-env.h: GIT-PAGER-ENV mkcarray
+ $(SHELL_PATH) mkcarray pager_env $ $@+
+ mv $@+ $@
+
# We need to apply sq twice, once to protect from the shell
# that runs GIT-BUILD-OPTIONS, and then again to protect it
# and the first level quoting from the shell that runs echo.
diff --git a/mkcarray b/mkcarray
index e69de29..5962440 100644
--- a/mkcarray
+++ b/mkcarray
@@ -0,0 +1,21 @@
+name=$1; shift
+guard=$(echo $name | tr a-z A-Z)
+
+cat -EOF
+#ifndef ${guard}_H
+#define ${guard}_H
+
+static const char *${name} = {
+EOF
+
+for i in $(cat); do
+ # XXX c-quote the values?
+ printf '\t%s,\n' $i
+done
+
+cat EOF
+ NULL
+};
+
+#endif /* ${guard}_H */
+EOF
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On Thu, Jan 16, 2014 at 11:26:50PM -0800, Kyle J. McKay wrote: --- a/pager.c +++ b/pager.c @@ -87,6 +87,10 @@ void setup_pager(void) argv_array_push(env, LESS=FRSX); if (!getenv(LV)) argv_array_push(env, LV=-c); +#ifdef PAGER_MORE_UNDERSTANDS_R +if (!getenv(MORE)) +argv_array_push(env, MORE=R); +#endif How about adding a leading - to both the LESS and MORE settings? Since you're in there patching... :) I do not have any problem with that, but would very much prefer it as a separate patch, in case there are any fallouts. And while the less man page does not have that wording, it does show this: LESS=-options; export LESS and this: LESS=-Dn9.1$-Ds4.1 Ugh. Having just read the LESS discussion, it makes me wonder if the strchr(getenv(LESS), 'R') check I add elsewhere in the series is sufficient. I suspect in practice it is good enough, but I would not be surprised if there is a way to fool it with a sufficiently complex LESS variable. Maybe we should just assume it is enough, and people with crazy LESS settings can set color.pager as appropriate? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On Fri, Jan 17, 2014 at 11:17:01AM -0800, Junio C Hamano wrote:
+#ifdef PAGER_MORE_UNDERSTANDS_R
+ if (!getenv(MORE))
+ argv_array_push(env, MORE=R);
+#endif
pager_process.env = argv_array_detach(env, NULL);
if (start_command(pager_process))
Let me repeat from $gmane/240110:
- Can we generalize this a bit so that a builder can pass a list
of var=val pairs and demote the existing LESS=FRSX to just a
canned setting of such a mechanism?
As we need to maintain this set these environments when unset here
and also in git-sh-setup.sh, I think it is about time to do that
clean-up. Duplicating two settings was borderline OK, but seeing
the third added fairly soon after the second was added tells me that
the clean-up must come before adding the third.
Wow, I somehow _completely_ missed that thread. When I looked at the
code with LV, I assumed it was just something that had happened long ago
and I had forgotten about. I didn't even bother reading git log.
Yeah, I agree that git-sh-setup should be consistent with what the C
porcelains do. However, adding build-time variables like:
PAGER_ENV = LESS=-FRSX LV=-c
does complicate the point of my series, which was to add more intimate
logic about how we handle LESS. With the current code, I can know that
an unset LESS variable means we will set R ourselves and turn on
color. We can get around that with something like:
int pager_can_handle_color(void)
{
const char *pager = get_pager();
if (!strcmp(pager, less)) {
const char *x = getenv(LESS);
if (!x) {
const char *e;
for (e = pager_env; *e; e++)
if ((x = skip_prefix(e, LESS=)))
break;
}
return !x || strchr(x, 'R');
}
[...]
}
but we are still hard-coding a lot of intelligence about less here. I
wonder if the abstraction provided by the Makefile variable is really
worthwhile. Anybody adding a new line to it would also want to tweak
pager_can_handle_color to add similar logic.
Taking a step back for a moment, we are getting two things out of such a
Makefile variable:
1. An easy way for packager to add intelligence about common pagers on
their system.
2. DRY between git-sh-setup and the C code.
I do like (1), but I do not know if we want to try to encode the can
handle color logic into a Makefile variable. What would it look like?
For (2), an alternate method would be to simply provide git pager as C
code, which spawns the appropriate pager as the C code would. Then
git-sh-setup can easily build around that.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On Fri, Jan 17, 2014 at 03:42:32PM -0800, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
Perhaps we can start it like this. Then pager.c can iterate over
the PAGER_ENV string, parse out LESS=, LV=, etc., and do its thing.
We would also need to muck with git-sh-setup.sh but that file is
already preprocessed in the Makefile, so we should be able to do
something similar to # @@BROKEN_PATH_FIX@@ there.
And here is such an attempt. There may be some missing dependencies
that need to be covered with a mechanism like GIT-BUILD-OPTIONS,
though.
Perhaps instead of just dumping it into a macro, we could actually parse
it at compile time into C code, and store the result as a header file.
Then that header file becomes the proper dependency, and this run-time
error:
+ while (*pager_env) {
+ struct strbuf buf = STRBUF_INIT;
+ const char *cp = strchrnul(pager_env, '=');
+
+ if (!*cp)
+ die(malformed build-time PAGER_ENV);
would become a compile-time error. We could do the same for the shell
code, too.
I'm thinking something like:
diff --git a/Makefile b/Makefile
index b4af1e2..3a8d15e 100644
--- a/Makefile
+++ b/Makefile
@@ -2182,6 +2182,16 @@ GIT-LDFLAGS: FORCE
echo $$FLAGS GIT-LDFLAGS; \
fi
+GIT-PAGER-ENV:
+ @PAGER_ENV='$(PAGER_ENV)'; \
+ if test x$$PAGER_ENV != x`cat GIT-PAGER-ENV 2/dev/null`; then \
+ echo $$PAGER_ENV GIT-PAGER-ENV; \
+ fi
+
+pager-env.h: GIT-PAGER-ENV mkcarray
+ $(SHELL_PATH) mkcarray pager_env $ $@+
+ mv $@+ $@
+
# We need to apply sq twice, once to protect from the shell
# that runs GIT-BUILD-OPTIONS, and then again to protect it
# and the first level quoting from the shell that runs echo.
diff --git a/mkcarray b/mkcarray
index e69de29..5962440 100644
--- a/mkcarray
+++ b/mkcarray
@@ -0,0 +1,21 @@
+name=$1; shift
+guard=$(echo $name | tr a-z A-Z)
+
+cat -EOF
+#ifndef ${guard}_H
+#define ${guard}_H
+
+static const char *${name} = {
+EOF
+
+for i in $(cat); do
+ # XXX c-quote the values?
+ printf '\t%s,\n' $i
+done
+
+cat EOF
+ NULL
+};
+
+#endif /* ${guard}_H */
+EOF
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
Kyle J. McKay mack...@gmail.com writes: On Jan 16, 2014, at 20:21, Jeff King wrote: When we run the pager, we always set LESS=R to tell the pager to pass through ANSI colors. On modern versions of FreeBSD, the system more can do the same trick. [snip] diff --git a/pager.c b/pager.c index 90d237e..2303164 100644 --- a/pager.c +++ b/pager.c @@ -87,6 +87,10 @@ void setup_pager(void) argv_array_push(env, LESS=FRSX); if (!getenv(LV)) argv_array_push(env, LV=-c); +#ifdef PAGER_MORE_UNDERSTANDS_R +if (!getenv(MORE)) +argv_array_push(env, MORE=R); +#endif How about adding a leading - to both the LESS and MORE settings? Since you're in there patching... :) The discussion we had when LV=-c was added, namely $gmane/240124, agrees. I however am perfectly fine to see it done as a separate clean-up patch. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
Jeff King p...@peff.net writes: diff --git a/pager.c b/pager.c index 90d237e..2303164 100644 --- a/pager.c +++ b/pager.c @@ -87,6 +87,10 @@ void setup_pager(void) argv_array_push(env, LESS=FRSX); if (!getenv(LV)) argv_array_push(env, LV=-c); +#ifdef PAGER_MORE_UNDERSTANDS_R + if (!getenv(MORE)) + argv_array_push(env, MORE=R); +#endif pager_process.env = argv_array_detach(env, NULL); if (start_command(pager_process)) Let me repeat from $gmane/240110: - Can we generalize this a bit so that a builder can pass a list of var=val pairs and demote the existing LESS=FRSX to just a canned setting of such a mechanism? As we need to maintain this set these environments when unset here and also in git-sh-setup.sh, I think it is about time to do that clean-up. Duplicating two settings was borderline OK, but seeing the third added fairly soon after the second was added tells me that the clean-up must come before adding the third. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
Junio C Hamano gits...@pobox.com writes: Jeff King p...@peff.net writes: diff --git a/pager.c b/pager.c index 90d237e..2303164 100644 --- a/pager.c +++ b/pager.c @@ -87,6 +87,10 @@ void setup_pager(void) argv_array_push(env, LESS=FRSX); if (!getenv(LV)) argv_array_push(env, LV=-c); +#ifdef PAGER_MORE_UNDERSTANDS_R +if (!getenv(MORE)) +argv_array_push(env, MORE=R); +#endif pager_process.env = argv_array_detach(env, NULL); if (start_command(pager_process)) Let me repeat from $gmane/240110: - Can we generalize this a bit so that a builder can pass a list of var=val pairs and demote the existing LESS=FRSX to just a canned setting of such a mechanism? As we need to maintain this set these environments when unset here and also in git-sh-setup.sh, I think it is about time to do that clean-up. Duplicating two settings was borderline OK, but seeing the third added fairly soon after the second was added tells me that the clean-up must come before adding the third. Perhaps we can start it like this. Then pager.c can iterate over the PAGER_ENV string, parse out LESS=, LV=, etc., and do its thing. We would also need to muck with git-sh-setup.sh but that file is already preprocessed in the Makefile, so we should be able to do something similar to # @@BROKEN_PATH_FIX@@ there. Makefile | 15 ++- config.mak.uname | 1 + 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index b4af1e2..c9e7847 100644 --- a/Makefile +++ b/Makefile @@ -342,6 +342,14 @@ all:: # Define DEFAULT_HELP_FORMAT to man, info or html # (defaults to man) if you want to have a different default when # git help is called without a parameter specifying the format. +# +# 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=-FRSX LV=-c +# +# to say export LESS=-FRSX (and LV=-c) if the environment variable +# LESS (and LV) is not set, respectively. GIT-VERSION-FILE: FORCE @$(SHELL_PATH) ./GIT-VERSION-GEN @@ -1506,6 +1514,10 @@ ifeq ($(PYTHON_PATH),) NO_PYTHON = NoThanks endif +ifndef PAGER_ENV +PAGER_ENV = LESS=-FRSX LV=-c +endif + QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir QUIET_SUBDIR1 = @@ -1585,11 +1597,12 @@ 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)) LIBS = $(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 diff --git a/config.mak.uname b/config.mak.uname index 7d31fad..8aea8a6 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -188,6 +188,7 @@ ifeq ($(uname_S),FreeBSD) endif PYTHON_PATH = /usr/local/bin/python HAVE_PATHS_H = YesPlease + PAGER_ENV = LESS=-FRSX LV=-c MORE=-R endif ifeq ($(uname_S),OpenBSD) NO_STRCASESTR = YesPlease -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
Junio C Hamano gits...@pobox.com writes:
Perhaps we can start it like this. Then pager.c can iterate over
the PAGER_ENV string, parse out LESS=, LV=, etc., and do its thing.
We would also need to muck with git-sh-setup.sh but that file is
already preprocessed in the Makefile, so we should be able to do
something similar to # @@BROKEN_PATH_FIX@@ there.
And here is such an attempt. There may be some missing dependencies
that need to be covered with a mechanism like GIT-BUILD-OPTIONS,
though.
Makefile | 18 --
config.mak.uname | 1 +
git-sh-setup.sh | 9 +
pager.c | 44 +---
4 files changed, 55 insertions(+), 17 deletions(-)
diff --git a/Makefile b/Makefile
index b4af1e2..690f4c6 100644
--- a/Makefile
+++ b/Makefile
@@ -342,6 +342,14 @@ all::
# Define DEFAULT_HELP_FORMAT to man, info or html
# (defaults to man) if you want to have a different default when
# git help is called without a parameter specifying the format.
+#
+# 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=-FRSX LV=-c
+#
+# to say export LESS=-FRSX (and LV=-c) if the environment variable
+# LESS (and LV) is not set, respectively.
GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1506,6 +1514,10 @@ ifeq ($(PYTHON_PATH),)
NO_PYTHON = NoThanks
endif
+ifndef PAGER_ENV
+PAGER_ENV = LESS=-FRSX LV=-c
+endif
+
QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir
QUIET_SUBDIR1 =
@@ -1585,11 +1597,12 @@ 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))
LIBS = $(GITLIBS) $(EXTLIBS)
BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
- $(COMPAT_CFLAGS)
+ $(COMPAT_CFLAGS) -DPAGER_ENV='$(PAGER_ENV)'
LIB_OBJS += $(COMPAT_OBJS)
# Quote for C
@@ -1739,7 +1752,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)
+ $(gitwebdir_SQ):$(PERL_PATH_SQ):$(PAGER_ENV)
define cmd_munge_script
$(RM) $@ $@+ \
sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -1751,6 +1764,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-e $(BROKEN_PATH_FIX) \
-e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
-e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
+-e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \
$@.sh $@+
endef
diff --git a/config.mak.uname b/config.mak.uname
index 7d31fad..8aea8a6 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -188,6 +188,7 @@ ifeq ($(uname_S),FreeBSD)
endif
PYTHON_PATH = /usr/local/bin/python
HAVE_PATHS_H = YesPlease
+ PAGER_ENV = LESS=-FRSX 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 fffa3c7..8fc1bbd 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -158,10 +158,11 @@ git_pager() {
else
GIT_PAGER=cat
fi
- : ${LESS=-FRSX}
- : ${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 0cc75a8..81a8af7 100644
--- a/pager.c
+++ b/pager.c
@@ -1,6 +1,7 @@
#include cache.h
#include run-command.h
#include sigchain.h
+#include argv-array.h
#ifndef DEFAULT_PAGER
#define DEFAULT_PAGER less
@@ -60,9 +61,37 @@ 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 setup_pager(void)
{
const char *pager = git_pager(isatty(1));
+ struct argv_array env = ARGV_ARRAY_INIT;
if (!pager || pager_in_use())
return;
@@ -80,17 +109,10 @@ void
Re: [PATCH 2/3] setup_pager: set MORE=R
On Jan 16, 2014, at 20:21, Jeff King wrote: When we run the pager, we always set LESS=R to tell the pager to pass through ANSI colors. On modern versions of FreeBSD, the system more can do the same trick. [snip] diff --git a/pager.c b/pager.c index 90d237e..2303164 100644 --- a/pager.c +++ b/pager.c @@ -87,6 +87,10 @@ void setup_pager(void) argv_array_push(env, LESS=FRSX); if (!getenv(LV)) argv_array_push(env, LV=-c); +#ifdef PAGER_MORE_UNDERSTANDS_R + if (!getenv(MORE)) + argv_array_push(env, MORE=R); +#endif How about adding a leading - to both the LESS and MORE settings? Since you're in there patching... :) The man page for more states: Options are also taken from the environment variable MORE (make sure to precede them with a dash (``-'')) but command line options will override them. And while the less man page does not have that wording, it does show this: LESS=-options; export LESS and this: LESS=-Dn9.1$-Ds4.1 So it looks like both LESS and MORE prefer to have their options start with a '-' more or less. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
