threads / patch / 15159

patchDo not override LESS

Subject: [PATCH RESEND] Do not override LESS

## tl;dr

8 messages between Aug 22, 2008 and Aug 24, 2008. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Anders Melchiorsen· Aug 22, 2008, 12:25 UTC · lore

Passing options to "less" with the LESS environment variable can interfere with existing environment variables. There are at least two problems, as the following examples show:

1. Alice is using git with colors. Now she decides to set LESS=i for
some reason. Suddenly, she sees codes in place of colors because LESS
is no longer set automatically.
2. Bob sets GIT_PAGER="less -RS", but does not set LESS. Git sets
LESS=FRSX before calling $GIT_PAGER. Now Bob wonders why his pager is
not always paging, when he explicitly tried to clear the F option.

By passing the options on the command line instead, both of these situations are handled.

Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---

Here is a resend, as I got no comments on the first try. The patch is rebased to the recent pager-swap setup.

For completeness, I note that this change will make existing setups with PAGER="less" behave differently, as they will no longer have -FRSX options set automatically. I tend to think that this is correct, since you then get what you ask for.

 pager.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
Show changes to pager.c +1 −3
diff --git a/pager.c b/pager.c
index aa0966c..9753fe9 100644
--- a/pager.c
+++ b/pager.c
@@ -20,8 +20,6 @@ static void pager_preexec(void)
 	FD_ZERO(&in);
 	FD_SET(0, &in);
 	select(1, &in, NULL, &in, NULL);
-
-	setenv("LESS", "FRSX", 0);
 }
 #endif
 
@@ -52,7 +50,7 @@ void setup_pager(void)
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!pager)
-		pager = "less";
+		pager = "less -FRSX";
 	else if (!*pager || !strcmp(pager, "cat"))
 		return;
 

-- 
1.5.6.4
Junio C Hamano· Aug 23, 2008, 05:35 UTC · re: Anders Melchiorsen · lore

Re: [PATCH RESEND] Do not override LESS

Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
Show 11 quoted lines
> Passing options to "less" with the LESS environment variable can
> interfere with existing environment variables. There are at least two
> problems, as the following examples show:
>
> 1. Alice is using git with colors. Now she decides to set LESS=i for
> some reason. Suddenly, she sees codes in place of colors because LESS
> is no longer set automatically.
>
> 2. Bob sets GIT_PAGER="less -RS", but does not set LESS. Git sets
> LESS=FRSX before calling $GIT_PAGER. Now Bob wonders why his pager is
> not always paging, when he explicitly tried to clear the F option.
3. Christ has been happily using git with his PAGER set to "less".  He
   suddenly notices that output from git linewraps and the pager does not
   exit when showing a short output, and gets very unhappy.
Anders Melchiorsen· Aug 23, 2008, 08:28 UTC · re: Junio C Hamano · lore

Re: [PATCH RESEND] Do not override LESS

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
>
>> Passing options to "less" with the LESS environment variable can
>> interfere with existing environment variables. There are at least two
>> problems, as the following examples show:
>>
>> 1. Alice is using git with colors. Now she decides to set LESS=i for
>> some reason. Suddenly, she sees codes in place of colors because LESS
>> is no longer set automatically.
>>
>> 2. Bob sets GIT_PAGER="less -RS", but does not set LESS. Git sets
>> LESS=FRSX before calling $GIT_PAGER. Now Bob wonders why his pager is
>> not always paging, when he explicitly tried to clear the F option.
>
> 3. Christ has been happily using git with his PAGER set to "less".  He
>    suddenly notices that output from git linewraps and the pager does not
>    exit when showing a short output, and gets very unhappy.

Well, I noted that point already, so I had hoped for a reply explaining why it is a big problem. Maybe setting PAGER="less" is more common than I think, as I have never seen it.

While I am wary of advocating a patch that makes Christ unhappy, the "3." issue is easily fixed by him setting GIT_PAGER="less -FRSX".

My concern is that without reading the source, it can be confusing to figure out what happens with less, $LESS and git. I think my patch improves on that. On the other hand, predictability is not really needed if the current setup DWIM. And maybe it does.

This is hardly a big issue either way, so now that I have response on the patch, I will not pursue it further.

Thanks, Anders.

Junio C Hamano· Aug 23, 2008, 09:08 UTC · re: Anders Melchiorsen · lore

Re: [PATCH RESEND] Do not override LESS

Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
Show 8 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> 3. Christ has been happily using git with his PAGER set to "less".  He
>>    suddenly notices that output from git linewraps and the pager does not
>>    exit when showing a short output, and gets very unhappy.
>
> Well, I noted that point already, so I had hoped for a reply
> explaining why it is a big problem.

It is a huge problem. It breaks people's existing perfectly well working setup.

And I do not think it is impossible to solve the issue without doing so.
> While I am wary of advocating a patch that makes Christ unhappy, the
> "3." issue is easily fixed by him setting GIT_PAGER="less -FRSX".

That's not a solution. Alice and Bob can also change their environment to their taste as well. Why punish existing users?

If the problem you are trying to solve is that there is no existing combination of the environment variables for them to do so, you can solve it by introducing a new configuration or environment to support such usage and documenting it, can't you?

Anders Melchiorsen· Aug 23, 2008, 10:21 UTC · re: Junio C Hamano · lore

Re: [PATCH RESEND] Do not override LESS

Junio C Hamano <gitster@pobox.com> writes:
> That's not a solution. Alice and Bob can also change their
> environment to their taste as well. Why punish existing users?

As I said, when you ask for "less", I tend to think it is correct to get "less". Not "LESS=FSRX less", with a special case if LESS is already defined.

> If the problem you are trying to solve is that there is no existing
> combination of the environment variables for them to do so, you can
> solve it by introducing a new configuration or environment to
> support such usage and documenting it, can't you?

I was trying to clean up things, not to make them even more confusing. If backwards compatibility is more important to you, I can understand.

Cheers, Anders.

Jonathan Nieder· Aug 24, 2008, 05:28 UTC · re: Anders Melchiorsen · lore

Re: [PATCH RESEND] Do not override LESS

Hi,
Anders Melchiorsen wrote:
Show 6 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
> >
> >> Passing options to "less" with the LESS environment variable can
> >> interfere with existing environment variables.
[...]
Show 11 quoted lines
> >> 2. Bob sets GIT_PAGER="less -RS", but does not set LESS. Git sets
> >> LESS=FRSX before calling $GIT_PAGER. Now Bob wonders why his pager is
> >> not always paging, when he explicitly tried to clear the F option.
> >
> > 3. Christ has been happily using git with his PAGER set to "less".  He
> >    suddenly notices that output from git linewraps and the pager does not
> >    exit when showing a short output, and gets very unhappy.
> 
> Well, I noted that point already, so I had hoped for a reply
> explaining why it is a big problem. Maybe setting PAGER="less" is more
> common than I think, as I have never seen it.
On system where less is not the default pager (e.g. Solaris), it is
very common.
Show 5 quoted lines
> While I am wary of advocating a patch that makes Christ unhappy, the
> "3." issue is easily fixed by him setting GIT_PAGER="less -FRSX".
> 
> My concern is that without reading the source, it can be confusing to
> figure out what happens with less, $LESS and git.
So perhaps it is a documentation problem.  How about this patch?
-- %< --
Subject: Documentation: clarify pager configuration
The unwary user may not know how to disable the -FRSX options.
Signed-off-by: Jonathan Nieder <jrnieder@uchicago.edu>
---
 Documentation/config.txt |    9 +++++++--
 Documentation/git.txt    |    3 ++-
 2 files changed, 9 insertions(+), 3 deletions(-)
Show changes to 2 files +9 −3

Documentation/config.txt, Documentation/git.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9020675..88638f7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -358,8 +358,13 @@ core.editor::
 	`EDITOR` environment variables and then finally `vi`.
 
 core.pager::
-	The command that git will use to paginate output.  Can be overridden
-	with the `GIT_PAGER` environment variable.
+	The command that git will use to paginate output.  Can
+	be overridden with the `GIT_PAGER` environment
+	variable.  Note that git sets the `LESS` environment
+	variable to `FRSX` if it is unset when it runs the
+	pager.  One can change these settings by setting the
+	`LESS` variable to some other value or by giving the
+	`core.pager` option a value such as "`less -+FRSX`".
 
 core.whitespace::
 	A comma separated list of common whitespace problems to
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 1bc295d..363a785 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -497,7 +497,8 @@ other
 'GIT_PAGER'::
 	This environment variable overrides `$PAGER`. If it is set
 	to an empty string or to the value "cat", git will not launch
-	a pager.
+	a pager.  See also the `core.pager` option in
+	linkgit:git-config[1].
 
 'GIT_SSH'::
 	If this environment variable is set then 'git-fetch'

-- 
1.6.0.481.g9ef3
Jonathan Nieder· Aug 24, 2008, 05:38 UTC · re: Jonathan Nieder · lore

[PATCH] Documentation: clarify pager.<cmd> configuration

It was not obvious from the text that pager.<cmd> is a boolean setting.

While we're changing the description, make some other improvements: lest we forget and fret, clarify that -p and pager.<cmd> do not kick in when stdout is not a tty; point to related core.pager and GIT_PAGER settings; use renamed --paginate option.

Signed-off-by: Jonathan Nieder <jrnieder@uchicago.edu>
---
 This is not related to the core.pager documentation patch I just
 sent; it just caught my eye as I was reading over the file.
 Documentation/config.txt |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
Show changes to Documentation/config.txt +5 −3
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 88638f7..b12e695 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -988,9 +988,11 @@ pack.packSizeLimit::
 	linkgit:git-repack[1].
 
 pager.<cmd>::
-	Allows to set your own pager preferences for each command, overriding
-	the default. If `\--pager` or `\--no-pager` is specified on the command
-	line, it takes precedence over this option.
+	Allows turning on or off pagination of the output of a
+	particular git subcommand when outputing to a tty.  If
+	`\--paginate` or `\--no-pager` is specified on the command line,
+	it takes precedence over this option.  To disable pagination for
+	all commands, set `core.pager` or 'GIT_PAGER' to "`cat`".
 
 pull.octopus::
 	The default merge strategy to use when pulling multiple branches

-- 
1.6.0.481.g9ef3
Junio C Hamano· Aug 24, 2008, 18:59 UTC · re: Jonathan Nieder · lore

Re: [PATCH RESEND] Do not override LESS

Jonathan Nieder <jrnieder@uchicago.edu> writes:
Show 6 quoted lines
> Anders Melchiorsen wrote:
> ...
>> My concern is that without reading the source, it can be confusing to
>> figure out what happens with less, $LESS and git.
>
> So perhaps it is a documentation problem.  How about this patch?
I think this makes sense.  Anders?

← back to recent threads