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

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

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

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

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,

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

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

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

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

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

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

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

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-21 Thread Junio C Hamano
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

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

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

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_UNDERSTANDS_R +if

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)) Let me repeat

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

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

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

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

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

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

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

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

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

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