Re: [PATCH 2/3] setup_pager: set MORE=R

2014-02-04 Thread Kyle J. McKay
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

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-02-04 Thread Yuri
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 fo

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-02-04 Thread Junio C Hamano
Jeff King 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

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-02-04 Thread Jeff King
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 3c6b385c655a52fd9db176c

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-02-04 Thread Yuri
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;32m

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-02-04 Thread Jeff King
On Tue, Feb 04, 2014 at 02:17:57PM -0800, Junio C Hamano wrote: > Jeff King 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 year

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-02-04 Thread Junio C Hamano
Jeff King 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" exi

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-02-04 Thread Jeff King
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 hav

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-23 Thread Junio C Hamano
Jeff King 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 majordom

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-22 Thread Jeff King
On Tue, Jan 21, 2014 at 12:42:58AM -0800, Kyle J. McKay wrote: > 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 >

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-21 Thread Junio C Hamano
Jeff King 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) { >> +st

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-21 Thread Junio C Hamano
Jeff King 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 s

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-21 Thread Kyle J. McKay
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

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-20 Thread Jeff King
On Fri, Jan 17, 2014 at 03:42:32PM -0800, Junio C Hamano wrote: > Junio C Hamano 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 tha

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-20 Thread Jeff King
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))

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-20 Thread Jeff King
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_UNDERST

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-17 Thread Junio C Hamano
Junio C Hamano 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

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-17 Thread Junio C Hamano
Junio C Hamano writes: > Jeff King 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

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-17 Thread Junio C Hamano
Jeff King 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 P

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-17 Thread Junio C Hamano
"Kyle J. McKay" 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 9

Re: [PATCH 2/3] setup_pager: set MORE=R

2014-01-16 Thread Kyle J. McKay
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 +++

[PATCH 2/3] setup_pager: set MORE=R

2014-01-16 Thread Jeff King
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. Unlike "less", we may be dealing with various versions of "more" that do not understand the "R" option, but do know how to read optio