[PATCH RESEND] Do not override LESS

[PATCH RESEND] Do not override LESS

[Anders Melchiorsen]

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

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

Re: [PATCH RESEND] Do not override LESS

[Junio C Hamano]

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.

Re: [PATCH RESEND] Do not override LESS

[Anders Melchiorsen]

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

Re: [PATCH RESEND] Do not override LESS

[Junio C Hamano]

Anders Melchiorsen <mail@cup.kalibalik.dk> writes:

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

Re: [PATCH RESEND] Do not override LESS

[Anders Melchiorsen]

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.

Re: [PATCH RESEND] Do not override LESS

[Jonathan Nieder]

Hi,

Anders Melchiorsen wrote:

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

[...]
> >> 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.
 
> 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(-)

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

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

[Jonathan Nieder]

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

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

Re: [PATCH RESEND] Do not override LESS

[Junio C Hamano]

Jonathan Nieder <jrnieder@uchicago.edu> writes:

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