git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Kyle J. McKay" <mackyle@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org, Jonathan Nieder <jrnieder@gmail.com>,
	Yuri <yuri@rawbw.com>
Subject: Re: [PATCH 2/3] setup_pager: set MORE=R
Date: Tue, 4 Feb 2014 18:11:01 -0800	[thread overview]
Message-ID: <62DB6DEF-8B39-4481-BA06-245BF45233E5@gmail.com> (raw)
In-Reply-To: <20140204221220.GA5457@sigill.intra.peff.net>

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 default  
assuming the user has not set any pager-specific environment variables.
3. This should prevent the user from ever seeing the ugly ESC[33 and  
ESC[m sequences with the default settings.
4. A user who has color.ui=auto set is not affected by this change.
5. Support for additional color pagers is easily added by tweaking the  
Makefile.

It has the following disadvantage:

1. A user who has a pager-specific environment variable set for the  
default pager will not get color by default even if color would be  
supported without first setting color.ui=auto or using an equivalent.   
Essentially they will get the pre-1.8.4 behavior.

--Kyle

  parent reply	other threads:[~2014-02-05  2:11 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-01-17  0:34 'git log' escape symbols shown as ESC[33 and ESC[m Yuri
2014-01-17  1:47 ` Jeff King
2014-01-17  2:02   ` Yuri
2014-01-17  2:13     ` Jeff King
2014-01-17  2:28       ` Yuri
2014-01-17  2:32         ` Jeff King
2014-01-17  2:46           ` Yuri
2014-01-17  2:29       ` Jonathan Nieder
2014-01-17  2:35         ` Jeff King
2014-01-17  3:21           ` Jeff King
2014-01-17  4:14           ` [PATCH 0/3] improved out-of-the-box color settings Jeff King
2014-01-17  4:21             ` [PATCH 1/3] setup_pager: refactor LESS/LV environment setting Jeff King
2014-01-17  4:21             ` [PATCH 2/3] setup_pager: set MORE=R Jeff King
2014-01-17  7:26               ` Kyle J. McKay
2014-01-17 19:11                 ` Junio C Hamano
2014-01-21  5:30                 ` Jeff King
2014-01-21  8:42                   ` Kyle J. McKay
2014-01-23  2:14                     ` Jeff King
2014-01-23 17:22                       ` Junio C Hamano
2014-01-17 19:17               ` Junio C Hamano
2014-01-17 19:57                 ` Junio C Hamano
2014-01-17 23:42                   ` Junio C Hamano
2014-01-21  6:13                     ` Jeff King
2014-01-21 19:28                       ` Junio C Hamano
2014-01-21  5:49                 ` Jeff King
2014-01-21 19:23                   ` Junio C Hamano
2014-02-04 22:12                     ` Jeff King
2014-02-04 22:17                       ` Junio C Hamano
2014-02-04 22:25                         ` Jeff King
2014-02-04 22:45                           ` Yuri
2014-02-04 22:48                             ` Jeff King
2014-02-04 22:54                               ` Junio C Hamano
2014-02-04 23:00                               ` Yuri
2014-02-05  2:11                       ` Kyle J. McKay [this message]
2014-01-17  4:24             ` [PATCH 3/3] pager: disable colors for some known-bad configurations Jeff King
2014-01-17  9:13             ` [PATCH 0/3] improved out-of-the-box color settings Yuri
2014-01-17 20:15   ` 'git log' escape symbols shown as ESC[33 and ESC[m Yuri
2014-02-05  1:24   ` Yuri
2014-02-05  1:33     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=62DB6DEF-8B39-4481-BA06-245BF45233E5@gmail.com \
    --to=mackyle@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    --cc=yuri@rawbw.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).