Harmful LESS flags

Harmful LESS flags

[d9ba]

hello list,

as mentioned earlier on IRC, I'm a bit concerned about the default LESS flags
used by git.

The S option causes git to cut off everything to the right

Consider this diff, printed by `git diff`

	 #!/usr/bin/env python
	-print('foo')
	+print('bar')

Looks ok to merge and run.

But, after disabling the pager:

	 #!/usr/bin/env python
	-print('foo')
	+print('bar') [lots of tabs] ; import os; os.system('aptitude install
subversion')

Oh no!

My workflow is to clone a project, read the whole source and review all diffs
after fetching them. After that is done I merge origin into my local
branch and
run the code on my system.

I've panic'd a bit after I've noticed the chopping.

It would be nice if we could change the flags to either

 a) avoid cutting off
 b) indicate something has been cut off (<- I prefer this)

I assume there are more people with a similar workflow who're still
unaware of
this feature.

I would joke about how 3 letter agencies introduced this flag to backdoor
open
source projects, but, well..

	Sincerely yours,
	a git user

Re: Harmful LESS flags

[Jonathan Nieder]

(cc-ing Mark Nudelman, less maintainer)
Hi,

d9ba@mailtor.net wrote:

> Consider this diff, printed by `git diff`
>
> 	 #!/usr/bin/env python
> 	-print('foo')
> 	+print('bar')
>
> Looks ok to merge and run.
>
> But, after disabling the pager:

Unfortunately there are other kinds of subtle bugs that can be hard to
see in a terminal, too.

[...]
> It would be nice if we could change the flags to either
>
>  a) avoid cutting off
>  b) indicate something has been cut off (<- I prefer this)

That sounds like a nice feature request for 'less': a marker on the
right margin when --chop-long-lines is in use and a line has been
chopped.  I don't see it at
http://www.greenwoodsoftware.com/less/bugs.html#enhance so maybe no
one else has thought of it yet.

Mark, what do you think?

Thanks,
Jonathan

Re: Harmful LESS flags

[David Kastrup]

d9ba@mailtor.net writes:

> It would be nice if we could change the flags to either
>
>  a) avoid cutting off
>  b) indicate something has been cut off (<- I prefer this)
>
> I assume there are more people with a similar workflow who're still
> unaware of this feature.
>
> I would joke about how 3 letter agencies introduced this flag to
> backdoor open source projects, but, well..

Most terminals are wider than three letters.

Still, it is a total nuisance.  I am constantly doing

-S RET

on my git output.  This should be left alone as an entirely personal
preference quite unrelated to Git.  There is no point in having Git
configure a default different from what is used elsewhere.

-- 
David Kastrup

Re: Harmful LESS flags

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> d9ba@mailtor.net writes:
>
>> It would be nice if we could change the flags to either
>>
>>  a) avoid cutting off
>>  b) indicate something has been cut off (<- I prefer this)
>>
>> I assume there are more people with a similar workflow who're still
>> unaware of this feature.
>>
>> I would joke about how 3 letter agencies introduced this flag to
>> backdoor open source projects, but, well..
>
> Most terminals are wider than three letters.

I am having a hard time to decide if you genuinely misread what you
are responding to, or if you are joking.  If the latter, I find the
joke mildly funny in a twisted way ;-)

But the tangent aside...

> Still, it is a total nuisance.  I am constantly doing
>
> -S RET
>
> on my git output.  This should be left alone as an entirely personal
> preference quite unrelated to Git.  There is no point in having Git
> configure a default different from what is used elsewhere.

I almost agree with the general principle of the last sentence, but
with a bit of reservation.  The default value for LESS (i.e. when
the user does not have any) we pass is FRSX, and the Porcelain
output these days is colored by default.  If we don't set a default
at all, the end-user experience for a newbie will be bad, especially
without "R".

Among the other three, F and X are to avoid a short output (e.g "git
show" on a one-liner with a short explanation) from asking for
confirmation to leave the pager and from clearing the screen upon
leaving the pager, and are generally accepted as good things (or at
least, we haven't seen much issue raised after we started passing
the default LESS for those who do not have their own in their
environment).

Use of S is very subjective.  While I personally do appreciate that
we have it by default, I can perfectly well understand why some
people do not want to see it in the default.  The best we can do is
to arrange so that people from one of the camps have their favorite
out of the box and those from the other camp need to tell Git that
they want to (or do not want to) fold long lines.

Traditionally, because the tool grew in a context of being used in a
project whose participants are at least not malicious, always having
to be on the lookout for fear of middle-of-line tabs hiding bad
contents near the right edges of lines has never been an issue.  If
somebody brought up a potential issue of such mode of attack back
then, Linus may have chosen the default differently.  I may have
myself chosen not to have S, if I were the maintainer when the LESS
default was originally introduced, and had I been made aware of this
issue.

I am not opposed to changing the default in the longer term, as long
as we have a solid transition plan to ensure that it won't disrupt
and/or upset existing users too much.

Re: Harmful LESS flags

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> Traditionally, because the tool grew in a context of being used in a
> project whose participants are at least not malicious, always having
> to be on the lookout for fear of middle-of-line tabs hiding bad
> contents near the right edges of lines has never been an issue.

My beef is not with "hiding bad contents" but with "hiding contents".
It makes the output useless for seeing what is actually happening as
soon as the option starts having an effect.

-- 
David Kastrup

Re: Harmful LESS flags

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Traditionally, because the tool grew in a context of being used in a
>> project whose participants are at least not malicious, always having
>> to be on the lookout for fear of middle-of-line tabs hiding bad
>> contents near the right edges of lines has never been an issue.
>
> My beef is not with "hiding bad contents" but with "hiding contents".
> It makes the output useless for seeing what is actually happening as
> soon as the option starts having an effect.

My suspicion is that one of the reasons why S was chosen to be in
the default was to mildly discourage people from busting the usual
line-length limit, but I am not Linus ;-)

Re: Harmful LESS flags

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Traditionally, because the tool grew in a context of being used in a
>>> project whose participants are at least not malicious, always having
>>> to be on the lookout for fear of middle-of-line tabs hiding bad
>>> contents near the right edges of lines has never been an issue.
>>
>> My beef is not with "hiding bad contents" but with "hiding contents".
>> It makes the output useless for seeing what is actually happening as
>> soon as the option starts having an effect.
>
> My suspicion is that one of the reasons why S was chosen to be in
> the default was to mildly discourage people from busting the usual
> line-length limit, but I am not Linus ;-)

Except that the busting becomes less rather than more conspicuous.

-- 
David Kastrup

Re: Harmful LESS flags

[Jeff King]

On Thu, Apr 24, 2014 at 12:29:21PM -0700, Junio C Hamano wrote:

> David Kastrup <dak@gnu.org> writes:
> 
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >> Traditionally, because the tool grew in a context of being used in a
> >> project whose participants are at least not malicious, always having
> >> to be on the lookout for fear of middle-of-line tabs hiding bad
> >> contents near the right edges of lines has never been an issue.
> >
> > My beef is not with "hiding bad contents" but with "hiding contents".
> > It makes the output useless for seeing what is actually happening as
> > soon as the option starts having an effect.
> 
> My suspicion is that one of the reasons why S was chosen to be in
> the default was to mildly discourage people from busting the usual
> line-length limit, but I am not Linus ;-)

I would think it's the opposite. Long lines look _horrible_ without
"-S", as they get wrapped at awkward points. Using "-S" means that long
lines don't bug you, unless you really want to scroll over and see the
content.

I really think the right solution here is to teach less to make it more
obvious that there is something worth scrolling over to. Here's a very
rough patch for less, if you want to see what I'm thinking of.

diff --git a/input.c b/input.c
index b211323..01aa411 100755
--- a/input.c
+++ b/input.c
@@ -178,6 +178,7 @@ get_forw_line:
 			 */
 			if (chopline || hshift > 0)
 			{
+				set_chopped_marker(ch_tell()-1);
 				do
 				{
 					if (ABORT_SIGS())
diff --git a/line.c b/line.c
index 1eb3914..b3358a0 100755
--- a/line.c
+++ b/line.c
@@ -1080,6 +1080,20 @@ set_status_col(c)
 	attr[0] = AT_NORMAL|AT_HILITE;
 }
 
+	public void
+set_chopped_marker(pos)
+	    POSITION pos;
+{
+	/*
+	 * Roll back output by one character; probably
+	 * we need to actually walk curr back further
+	 * for multibyte characters?
+	 */
+	column--;
+	curr--;
+	store_char('>', AT_NORMAL|AT_HILITE, NULL, pos);
+}
+
 /*
  * Get a character from the current line.
  * Return the character as the function return value,

-Peff

Re: Harmful LESS flags

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I would think it's the opposite. Long lines look _horrible_ without
> "-S", as they get wrapped at awkward points. Using "-S" means that long
> lines don't bug you, unless you really want to scroll over and see the
> content.
>
> I really think the right solution here is to teach less to make it more
> obvious that there is something worth scrolling over to. Here's a very
> rough patch for less, if you want to see what I'm thinking of.

Yes, I think that was suggested as an issue worth bringing up with
less maintainers earlier in the thread already (and that was why I
didn't repeat it).  If we were in the business of updating less to
suit many users' needs (the needs of our users included), we may
even want to advocate turning R on by default.

And I do agree that the "chopped marker" would be a very sensible
thing to show in the "-S" output; I would have chosen "$" myself for
that to match an existing practice in (setq truncate-lines t) in
Emacs, though.

Re: Harmful LESS flags

[David Kastrup]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 24, 2014 at 12:29:21PM -0700, Junio C Hamano wrote:
>
>> David Kastrup <dak@gnu.org> writes:
>> 
>> > Junio C Hamano <gitster@pobox.com> writes:
>> >
>> >> Traditionally, because the tool grew in a context of being used in a
>> >> project whose participants are at least not malicious, always having
>> >> to be on the lookout for fear of middle-of-line tabs hiding bad
>> >> contents near the right edges of lines has never been an issue.
>> >
>> > My beef is not with "hiding bad contents" but with "hiding contents".
>> > It makes the output useless for seeing what is actually happening as
>> > soon as the option starts having an effect.
>> 
>> My suspicion is that one of the reasons why S was chosen to be in
>> the default was to mildly discourage people from busting the usual
>> line-length limit, but I am not Linus ;-)
>
> I would think it's the opposite. Long lines look _horrible_ without
> "-S", as they get wrapped at awkward points. Using "-S" means that long
> lines don't bug you, unless you really want to scroll over and see the
> content.

I prefer horrible over useless.

> I really think the right solution here is to teach less to make it more
> obvious that there is something worth scrolling over to. Here's a very
> rough patch for less, if you want to see what I'm thinking of.

Still useless.  I'm not actually interested in a more prominent "I could
be useful" indicator.

-- 
David Kastrup

Re: Harmful LESS flags

[Jeff King]

On Thu, Apr 24, 2014 at 02:47:01PM -0700, Junio C Hamano wrote:

> And I do agree that the "chopped marker" would be a very sensible
> thing to show in the "-S" output; I would have chosen "$" myself for
> that to match an existing practice in (setq truncate-lines t) in
> Emacs, though.

Hmm. I do not use Emacs, but I explicitly avoided "$" because of its
end-of-line connotations. E.g., in "cat -A" it means the opposite: this
is the real "\n" end-of-line. But if there's existing precedent for "$",
that would be fine with me.

-Peff

Re: Harmful LESS flags

[Jeff King]

On Thu, Apr 24, 2014 at 11:48:30PM +0200, David Kastrup wrote:

> > I really think the right solution here is to teach less to make it more
> > obvious that there is something worth scrolling over to. Here's a very
> > rough patch for less, if you want to see what I'm thinking of.
> 
> Still useless.  I'm not actually interested in a more prominent "I could
> be useful" indicator.

So don't set -S, then.

There are two questions here:

  1. Can less do a better job of indicating what's in the input when -S
     is in effect?

  2. What should get put into $LESS by default?

I was specifically addressing (1). Your comment does not help at all
there.

It could have an impact on (2), but you didn't say anything besides "I
don't like it". That doesn't add anything to the conversation.

-Peff

Re: Harmful LESS flags

[David Kastrup]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 24, 2014 at 11:48:30PM +0200, David Kastrup wrote:
>
>> > I really think the right solution here is to teach less to make it more
>> > obvious that there is something worth scrolling over to. Here's a very
>> > rough patch for less, if you want to see what I'm thinking of.
>> 
>> Still useless.  I'm not actually interested in a more prominent "I could
>> be useful" indicator.
>
> So don't set -S, then.

I don't.  Git does it unasked for.

> There are two questions here:
>
>   1. Can less do a better job of indicating what's in the input when -S
>      is in effect?
>
>   2. What should get put into $LESS by default?
>
> I was specifically addressing (1). Your comment does not help at all
> there.
>
> It could have an impact on (2), but you didn't say anything besides "I
> don't like it". That doesn't add anything to the conversation.

No, I said it is useless, which is different from "I don't like it".
The information is not copy&pastable from a terminal window since it is
cut off.  It is also useless for review since one does not actually know
what's in there.  The only thing it has going for it is that it's
prettier than the actually usable information.  Which might sometimes be
nice if one is not interested overly in the payload, like when using
--graph.  But then even a graph display wants to get copy&pasted into
windows with a different size from the terminal window, like in
<URL:http://code.google.com/p/lilypond/issues/detail?id=3723#c7>.

-- 
David Kastrup

Re: Harmful LESS flags

[Jonathan Nieder]

Hi,

David Kastrup wrote:
> Jeff King <peff@peff.net> writes:

>> There are two questions here:
>>
>>   1. Can less do a better job of indicating what's in the input when -S
>>      is in effect?
>>
>>   2. What should get put into $LESS by default?
>>
>> I was specifically addressing (1). Your comment does not help at all
>> there.
>>
>> It could have an impact on (2), but you didn't say anything besides "I
>> don't like it". That doesn't add anything to the conversation.
>
> No, I said it is useless, which is different from "I don't like it".
> The information is not copy&pastable from a terminal window since it is
> cut off.  It is also useless for review since one does not actually know
> what's in there.  The only thing it has going for it is that it's
> prettier than the actually usable information.

I disagree with your characterization of what's useful here, but it
really doesn't matter.  Why are you still arguing?

I think it would be fine to change git's default for LESS to FRX and
document that change wherever the documentation currently mentions
FRSX, if someone wants to write a patch for it.  (Such a change would
sit in "pu" or "next" until after 2.0.0 is released, of course.)

In the meantime, when you're on machines using the current default,
you have two choices:

 a) set the LESS envvar in your .profile explicitly
 b) hit the two keys '-', shift+S when git opens a pager

The argument about safety is a red herring here, since it's always
possible that a patch will wrap to make new lines with '+' or '-' or
'@@' at the beginning that are equally confusing.

Hoping that clarifies,
Jonathan

Re: Harmful LESS flags

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> I am not opposed to changing the default in the longer term, as long
> as we have a solid transition plan to ensure that it won't disrupt
> and/or upset existing users too much.

I am personally in favor of changing the default to drop the S. Silently
hiding stuff from the user's eyes is really bad. With good coding
standard and reasonable terminal size, it actually doesn't matter. And
on projects actually containing very long lines (e.g. some people write
LaTeX code with whole paragraphs for each lines), showing only the
beginning of the line (i.e. the first line of the paragraph in my
LaTeX example) isn't very useful.

I do not think we particularly need a transition plan here: it's purely
a user-interface thing, not something that may break any script or other
tool. Changing the default and documenting the way to return to the old
default in release notes and in the manual would be sufficient IMHO.

GUI usually don't warn when the shape of a button is going to change in
the next version ...

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Harmful LESS flags

[Jonathan Nieder]

Hi,

Matthieu Moy wrote:

> I am personally in favor of changing the default to drop the S. Silently
> hiding stuff from the user's eyes is really bad. With good coding
> standard and reasonable terminal size, it actually doesn't matter.

Just for clarity: no, when we are talking about well formatted code,
-S is actually a way better interface.

That's because indentation matters and makes it easy to take in code
structure at a glance, long lines that get cut off by the margin stick
out like a sore thumb already, and lines wrapped at an arbitrary
character are even more distracting to the point of being useless.

In practice I believe the "Silently hiding stuff" concern is much
harder to solve.  In the case of malicious code that opened this
thread, I think a marker on the right margin would reveal the
whitespace more clearly than wrapping that the reader may or may not
notice.

Luckily, it is very easy to switch between the two views on the fly
--- in an already-open "less" window, you just type '-' + 'S'.  In the
spirit of not overriding tool defaults when there is not a strong
reason to do so, I agree that if someone writes a patch to drop
the 'S' I would probably like it.

[...]
> I do not think we particularly need a transition plan here: it's purely
> a user-interface thing, not something that may break any script or other
> tool.

Agreed ---- a note in release notes and making sure the documentation
reflects the new default should be enough.

Thanks and hope that helps,
Jonathan

Re: Harmful LESS flags

[David Kastrup]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Matthieu Moy wrote:
>
>> I am personally in favor of changing the default to drop the S. Silently
>> hiding stuff from the user's eyes is really bad. With good coding
>> standard and reasonable terminal size, it actually doesn't matter.
>
> Just for clarity: no, when we are talking about well formatted code,
> -S is actually a way better interface.

When we are talking about well-formatted code, -S does not matter either
which way.

> That's because indentation matters and makes it easy to take in code
> structure at a glance, long lines that get cut off by the margin stick
> out like a sore thumb already, and lines wrapped at an arbitrary
> character are even more distracting to the point of being useless.

Lines which are cut off are not "to the point of being useless", they
_are_ useless.

I am not arguing that wrapped lines are pretty.  And I also consider the
"malicious" or "hiding" angle at best a marginal concern.

Overriding less' defaults should only be done for unequivocal benefits,
and in this case I consider the result actually more of a detriment than
anything else.

-- 
David Kastrup

Re: Harmful LESS flags

[Jonathan Nieder]

David Kastrup wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Just for clarity: no, when we are talking about well formatted code,
>> -S is actually a way better interface.
>
> When we are talking about well-formatted code, -S does not matter either
> which way.

Sorry for the lack of clarity.  I believe well-formatted code can
contain long lines.  For example, sometimes a message + the printf to
print it and indentation move past the right margin.

If I wasn't talking about long lines, why would I have replied in the
first place?

[...]
> Overriding less' defaults should only be done for unequivocal benefits,

We agree here.  So, does someone who actually wants this change want to
propose a patch? :)

Hope that helps,
Jonathan

[PATCH] PAGER_ENV: remove 'S' from $LESS by default

[Matthieu Moy]

By default, Git used to set $LESS to -FRSX if $LESS was not set by the
user. The FRX flags actually make sense for Git (F and X because Git
sometimes pipes short output to less, and R because Git pipes colored
output). The S flag (chop long lines), on the other hand, is not related
to Git and is a matter of user preference. Git should not decide for the
user to change LESS's default.

More specifically, the S flag harms users who review untrusted code
within a pager, since a patch looking like:

-old code;
+new good code; [... lots of tabs ...] malicious code;

would appear identical to:

-old code;
+new good code;

Users who prefer the old behavior can still set the $LESS environment
variable to -FRSX explicitly, or set core.pager to 'less -S'.

The documentation in config.txt is made a bit longer to keep both an
example setting the 'S' flag (needed to recover the old behavior) and an
example showing how to unset a flag set by Git.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
> We agree here.  So, does someone who actually wants this change want to
> propose a patch? :)

Here you are.

 Documentation/config.txt | 13 ++++++++-----
 Makefile                 |  6 +++---
 perl/Git/SVN/Log.pm      |  2 +-
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e30561d..b7f92ac 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -560,14 +560,17 @@ core.pager::
 	configuration, then `$PAGER`, and then the default chosen at
 	compile time (usually 'less').
 +
-When the `LESS` environment variable is unset, Git sets it to `FRSX`
+When the `LESS` environment variable is unset, Git sets it to `FRX`
 (if `LESS` environment variable is set, Git does not change it at
 all).  If you want to selectively override Git's default setting
-for `LESS`, you can set `core.pager` to e.g. `less -+S`.  This will
+for `LESS`, you can set `core.pager` to e.g. `less -S`.  This will
 be passed to the shell by Git, which will translate the final
-command to `LESS=FRSX less -+S`. The environment tells the command
-to set the `S` option to chop long lines but the command line
-resets it to the default to fold long lines.
+command to `LESS=FRX less -S`. The environment does not set the
+`S` option but the command line does, instructing less to truncate
+long lines. Similarly, setting `core.pager` to `less -+F` will
+deactivate the `F` option specified by the environment from the
+command-line, deactivating the "quit if one screen" behavior of
+`less`.
 +
 Likewise, when the `LV` environment variable is unset, Git sets it
 to `-c`.  You can override this setting by exporting `LV` with
diff --git a/Makefile b/Makefile
index a3b298e..cd3cdf6 100644
--- a/Makefile
+++ b/Makefile
@@ -344,9 +344,9 @@ all::
 # Define PAGER_ENV to a SP separated VAR=VAL pairs to define
 # default environment variables to be passed when a pager is spawned, e.g.
 #
-#    PAGER_ENV = LESS=-FRSX LV=-c
+#    PAGER_ENV = LESS=-FRX LV=-c
 #
-# to say "export LESS=-FRSX (and LV=-c) if the environment variable
+# to say "export LESS=-FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
 
 GIT-VERSION-FILE: FORCE
@@ -1518,7 +1518,7 @@ NO_PYTHON = NoThanks
 endif
 
 ifndef PAGER_ENV
-PAGER_ENV = LESS=-FRSX LV=-c
+PAGER_ENV = LESS=-FRX LV=-c
 endif
 
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm
index 34f2869..6641053 100644
--- a/perl/Git/SVN/Log.pm
+++ b/perl/Git/SVN/Log.pm
@@ -116,7 +116,7 @@ sub run_pager {
 		return;
 	}
 	open STDIN, '<&', $rfd or fatal "Can't redirect stdin: $!";
-	$ENV{LESS} ||= 'FRSX';
+	$ENV{LESS} ||= 'FRX';
 	$ENV{LV} ||= '-c';
 	exec $pager or fatal "Can't run pager: $! ($pager)";
 }
-- 
1.9.2.698.ge58c0c2.dirty

Re: [PATCH] PAGER_ENV: remove 'S' from $LESS by default

[David Kastrup]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> By default, Git used to set $LESS to -FRSX if $LESS was not set by the
> user. The FRX flags actually make sense for Git (F and X because Git
> sometimes pipes short output to less, and R because Git pipes colored
> output). The S flag (chop long lines), on the other hand, is not related
> to Git and is a matter of user preference. Git should not decide for the
> user to change LESS's default.

>> We agree here.  So, does someone who actually wants this change want to
>> propose a patch? :)
>
> Here you are.
>
>  Documentation/config.txt | 13 ++++++++-----
>  Makefile                 |  6 +++---
>  perl/Git/SVN/Log.pm      |  2 +-
>  3 files changed, 12 insertions(+), 9 deletions(-)

There seem to be a few more occurences (git-sh-setup.sh and pager.c):

$ git grep FRSX
Documentation/RelNotes/1.6.5.txt: * mingw will also give FRSX as the default val
Documentation/config.txt:When the `LESS` environment variable is unset, Git sets
Documentation/config.txt:command to `LESS=FRSX less -+S`. The environment tells 
git-sh-setup.sh:        : ${LESS=-FRSX}
pager.c:                        env[i++] = "LESS=FRSX";
perl/Git/SVN/Log.pm:    $ENV{LESS} ||= 'FRSX';

Searching for LESS seems to implicate a few more possible candidates in
contrib/examples:

contrib/examples/git-log.sh:LESS=-S ${PAGER:-less}
contrib/examples/git-whatchanged.sh:LESS="$LESS -S" ${PAGER:-less}


-- 
David Kastrup

Re: [PATCH] PAGER_ENV: remove 'S' from $LESS by default

[Matthieu Moy]

David Kastrup <dak@gnu.org> writes:

> There seem to be a few more occurences (git-sh-setup.sh and pager.c):

Not since f82c3ffd862c7 (Wed Feb 5 2014, move LESS/LV pager environment
to Makefile).

> Searching for LESS seems to implicate a few more possible candidates in
> contrib/examples:
>
> contrib/examples/git-log.sh:LESS=-S ${PAGER:-less}
> contrib/examples/git-whatchanged.sh:LESS="$LESS -S" ${PAGER:-less}

Yes, I did see these, but I considered that contrib/examples/ should
remain a snapshot of what the commands used to look like at the time
they were shell scripts.

There's also user-manual.txt:

,----
| Basically, the initial version of `git log` was a shell script:
| 
| ----------------------------------------------------------------
| $ git-rev-list --pretty $(git-rev-parse --default HEAD "$@") | \
| 	LESS=-S ${PAGER:-less}
| ----------------------------------------------------------------
`----

that I left intact. I can change them too if people prefer.

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] PAGER_ENV: remove 'S' from $LESS by default

[David Kastrup]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> There seem to be a few more occurences (git-sh-setup.sh and pager.c):
>
> Not since f82c3ffd862c7 (Wed Feb 5 2014, move LESS/LV pager environment
> to Makefile).

The only upstream branch containing this commit is pu.  So this patch
should likely not go anywhere else for now.

-- 
David Kastrup

Re: [PATCH] PAGER_ENV: remove 'S' from $LESS by default

[Matthieu Moy]

David Kastrup <dak@gnu.org> writes:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> David Kastrup <dak@gnu.org> writes:
>>
>>> There seem to be a few more occurences (git-sh-setup.sh and pager.c):
>>
>> Not since f82c3ffd862c7 (Wed Feb 5 2014, move LESS/LV pager environment
>> to Makefile).
>
> The only upstream branch containing this commit is pu.  So this patch
> should likely not go anywhere else for now.

Oops, indeed, I made my patch on top of pu by mistake. Anyway, my patch
can wait for the other series to be merged.

Jeff, you're the author of f82c3ffd862c7, topic jk/makefile in git.git,
marked "expecting a reroll" by Junio. Any news from the series?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] PAGER_ENV: remove 'S' from $LESS by default

[Jeff King]

On Mon, Apr 28, 2014 at 02:22:21PM +0200, Matthieu Moy wrote:

> >> Not since f82c3ffd862c7 (Wed Feb 5 2014, move LESS/LV pager environment
> >> to Makefile).
> >
> > The only upstream branch containing this commit is pu.  So this patch
> > should likely not go anywhere else for now.
> 
> Oops, indeed, I made my patch on top of pu by mistake. Anyway, my patch
> can wait for the other series to be merged.
> 
> Jeff, you're the author of f82c3ffd862c7, topic jk/makefile in git.git,
> marked "expecting a reroll" by Junio. Any news from the series?

I am planning to revisit it eventually, but it's fairly low priority.
There is some pretty heavy refactoring in the series, and the PAGER_ENV
bits do not have to be held hostage to that refactoring (they are really
just demonstrating the refactoring).

I'd be OK with doing the moral equivalent for now (perhaps just taking
Junio's proposal[1]), and I can deal with the refactoring later when
re-rolling the Makefile series.

-Peff

[1] http://article.gmane.org/gmane.comp.version-control.git/240637

Re: [PATCH] PAGER_ENV: remove 'S' from $LESS by default

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Apr 28, 2014 at 02:22:21PM +0200, Matthieu Moy wrote:
>
> I'd be OK with doing the moral equivalent for now (perhaps just taking
> Junio's proposal[1]), and I can deal with the refactoring later when
> re-rolling the Makefile series.
>
> -Peff
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/240637

I doubt we would want to use the patch verbatim in that message; it
served its purpose well to illustrate that there may be other ways
to address the issue, but I agreed with the flaw in it you pointed
out in the thread [*1*]

I also agree that droppage of S does not have to wait for that
topic.


[Reference]
*1* http://thread.gmane.org/gmane.comp.version-control.git/240548/focus=240746

Re: Harmful LESS flags

[Mark Nudelman]

On 4/23/2014 5:11 PM, Jonathan Nieder wrote:
> That sounds like a nice feature request for 'less': a marker on the
> right margin when --chop-long-lines is in use and a line has been
> chopped.  I don't see it at
> http://www.greenwoodsoftware.com/less/bugs.html#enhance so maybe no
> one else has thought of it yet.
> 
> Mark, what do you think?
> 

Hi Jonathan,
This seems reasonable.  I actually thought that something like this was
already implemented, and displayed in the status column when -J is in
effect.  But I must have dreamed that or something.  I'll add this as an
enhancement request.

--Mark

Re: [PATCH] PAGER_ENV: remove 'S' from $LESS by default

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> On Mon, Apr 28, 2014 at 02:22:21PM +0200, Matthieu Moy wrote:
>>
>> I'd be OK with doing the moral equivalent for now (perhaps just taking
>> Junio's proposal[1]), and I can deal with the refactoring later when
>> re-rolling the Makefile series.
>>
>> -Peff
>>
>> [1] http://article.gmane.org/gmane.comp.version-control.git/240637
>
> I doubt we would want to use the patch verbatim in that message; it
> served its purpose well to illustrate that there may be other ways
> to address the issue, but I agreed with the flaw in it you pointed
> out in the thread [*1*]
>
> I also agree that droppage of S does not have to wait for that
> topic.

So, shall I rewrite my patch on top of master? (not hard, but there will
be a minor conflict to resolve when merging with Peff's cooking series).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] PAGER_ENV: remove 'S' from $LESS by default

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> I also agree that droppage of S does not have to wait for that
>> topic.
>
> So, shall I rewrite my patch on top of master? (not hard, but there will
> be a minor conflict to resolve when merging with Peff's cooking series).

Sure, the one near the tip of 'pu' can even be dropped, especially
when nobody is actively looking at it, if it turns out to be too
much of a nuisance.

Thanks.

[PATCH v2] pager: remove 'S' from $LESS by default

[Matthieu Moy]

By default, Git used to set $LESS to -FRSX if $LESS was not set by the
user. The FRX flags actually make sense for Git (F and X because Git
sometimes pipes short output to less, and R because Git pipes colored
output). The S flag (chop long lines), on the other hand, is not related
to Git and is a matter of user preference. Git should not decide for the
user to change LESS's default.

More specifically, the S flag harms users who review untrusted code
within a pager, since a patch looking like:

-old code;
+new good code; [... lots of tabs ...] malicious code;

would appear identical to:

-old code;
+new good code;

Users who prefer the old behavior can still set the $LESS environment
variable to -FRSX explicitly, or set core.pager to 'less -S'.

The documentation in config.txt is made a bit longer to keep both an
example setting the 'S' flag (needed to recover the old behavior) and an
example showing how to unset a flag set by Git.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
This is just a rewrite of PATCH v1 on top of master instead of pu.

 Documentation/config.txt | 13 ++++++++-----
 git-sh-setup.sh          |  2 +-
 pager.c                  |  2 +-
 perl/Git/SVN/Log.pm      |  2 +-
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 73c8973..5484d9d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -553,14 +553,17 @@ core.pager::
 	configuration, then `$PAGER`, and then the default chosen at
 	compile time (usually 'less').
 +
-When the `LESS` environment variable is unset, Git sets it to `FRSX`
+When the `LESS` environment variable is unset, Git sets it to `FRX`
 (if `LESS` environment variable is set, Git does not change it at
 all).  If you want to selectively override Git's default setting
-for `LESS`, you can set `core.pager` to e.g. `less -+S`.  This will
+for `LESS`, you can set `core.pager` to e.g. `less -S`.  This will
 be passed to the shell by Git, which will translate the final
-command to `LESS=FRSX less -+S`. The environment tells the command
-to set the `S` option to chop long lines but the command line
-resets it to the default to fold long lines.
+command to `LESS=FRX less -S`. The environment does not set the
+`S` option but the command line does, instructing less to truncate
+long lines. Similarly, setting `core.pager` to `less -+F` will
+deactivate the `F` option specified by the environment from the
+command-line, deactivating the "quit if one screen" behavior of
+`less`.
 +
 Likewise, when the `LV` environment variable is unset, Git sets it
 to `-c`.  You can override this setting by exporting `LV` with
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 5f28b32..9447980 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -160,7 +160,7 @@ git_pager() {
 	else
 		GIT_PAGER=cat
 	fi
-	: ${LESS=-FRSX}
+	: ${LESS=-FRX}
 	: ${LV=-c}
 	export LESS LV
 
diff --git a/pager.c b/pager.c
index 0cc75a8..f75e8ae 100644
--- a/pager.c
+++ b/pager.c
@@ -85,7 +85,7 @@ void setup_pager(void)
 		int i = 0;
 
 		if (!getenv("LESS"))
-			env[i++] = "LESS=FRSX";
+			env[i++] = "LESS=FRX";
 		if (!getenv("LV"))
 			env[i++] = "LV=-c";
 		env[i] = NULL;
diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm
index 34f2869..6641053 100644
--- a/perl/Git/SVN/Log.pm
+++ b/perl/Git/SVN/Log.pm
@@ -116,7 +116,7 @@ sub run_pager {
 		return;
 	}
 	open STDIN, '<&', $rfd or fatal "Can't redirect stdin: $!";
-	$ENV{LESS} ||= 'FRSX';
+	$ENV{LESS} ||= 'FRX';
 	$ENV{LV} ||= '-c';
 	exec $pager or fatal "Can't run pager: $! ($pager)";
 }
-- 
1.9.2.698.ge58c0c2.dirty

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> By default, Git used to set $LESS to -FRSX if $LESS was not set by the
> user. The FRX flags actually make sense for Git (F and X because Git
> sometimes pipes short output to less, and R because Git pipes colored
> output). The S flag (chop long lines), on the other hand, is not related
> to Git and is a matter of user preference. Git should not decide for the
> user to change LESS's default.

Git always pipes its output to less, not just "sometimes pipes short
ones" ;-)  "because the output may be short" would be more accurate
and would convey the same thing.

But that is just nitpicking.  The patch looks totally agreeable.

I am inclined to suggest queuing it for the first batch after 2.0
instead of directly applying to 'master', as we have past the point
we can expect to see reports of unexpected fallouts and fix the
issues in time for the final.

Thanks.

> More specifically, the S flag harms users who review untrusted code
> within a pager, since a patch looking like:
>
> -old code;
> +new good code; [... lots of tabs ...] malicious code;
>
> would appear identical to:
>
> -old code;
> +new good code;
>
> Users who prefer the old behavior can still set the $LESS environment
> variable to -FRSX explicitly, or set core.pager to 'less -S'.
>
> The documentation in config.txt is made a bit longer to keep both an
> example setting the 'S' flag (needed to recover the old behavior) and an
> example showing how to unset a flag set by Git.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> This is just a rewrite of PATCH v1 on top of master instead of pu.
>
>  Documentation/config.txt | 13 ++++++++-----
>  git-sh-setup.sh          |  2 +-
>  pager.c                  |  2 +-
>  perl/Git/SVN/Log.pm      |  2 +-
>  4 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 73c8973..5484d9d 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -553,14 +553,17 @@ core.pager::
>  	configuration, then `$PAGER`, and then the default chosen at
>  	compile time (usually 'less').
>  +
> -When the `LESS` environment variable is unset, Git sets it to `FRSX`
> +When the `LESS` environment variable is unset, Git sets it to `FRX`
>  (if `LESS` environment variable is set, Git does not change it at
>  all).  If you want to selectively override Git's default setting
> -for `LESS`, you can set `core.pager` to e.g. `less -+S`.  This will
> +for `LESS`, you can set `core.pager` to e.g. `less -S`.  This will
>  be passed to the shell by Git, which will translate the final
> -command to `LESS=FRSX less -+S`. The environment tells the command
> -to set the `S` option to chop long lines but the command line
> -resets it to the default to fold long lines.
> +command to `LESS=FRX less -S`. The environment does not set the
> +`S` option but the command line does, instructing less to truncate
> +long lines. Similarly, setting `core.pager` to `less -+F` will
> +deactivate the `F` option specified by the environment from the
> +command-line, deactivating the "quit if one screen" behavior of
> +`less`.
>  +
>  Likewise, when the `LV` environment variable is unset, Git sets it
>  to `-c`.  You can override this setting by exporting `LV` with
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 5f28b32..9447980 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -160,7 +160,7 @@ git_pager() {
>  	else
>  		GIT_PAGER=cat
>  	fi
> -	: ${LESS=-FRSX}
> +	: ${LESS=-FRX}
>  	: ${LV=-c}
>  	export LESS LV
>  
> diff --git a/pager.c b/pager.c
> index 0cc75a8..f75e8ae 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -85,7 +85,7 @@ void setup_pager(void)
>  		int i = 0;
>  
>  		if (!getenv("LESS"))
> -			env[i++] = "LESS=FRSX";
> +			env[i++] = "LESS=FRX";
>  		if (!getenv("LV"))
>  			env[i++] = "LV=-c";
>  		env[i] = NULL;
> diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm
> index 34f2869..6641053 100644
> --- a/perl/Git/SVN/Log.pm
> +++ b/perl/Git/SVN/Log.pm
> @@ -116,7 +116,7 @@ sub run_pager {
>  		return;
>  	}
>  	open STDIN, '<&', $rfd or fatal "Can't redirect stdin: $!";
> -	$ENV{LESS} ||= 'FRSX';
> +	$ENV{LESS} ||= 'FRX';
>  	$ENV{LV} ||= '-c';
>  	exec $pager or fatal "Can't run pager: $! ($pager)";
>  }

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> By default, Git used to set $LESS to -FRSX if $LESS was not set by the
>> user. The FRX flags actually make sense for Git (F and X because Git
>> sometimes pipes short output to less, and R because Git pipes colored
>> output). The S flag (chop long lines), on the other hand, is not related
>> to Git and is a matter of user preference. Git should not decide for the
>> user to change LESS's default.
>
> Git always pipes its output to less,

Err, no, not all commands use a pager.

> I am inclined to suggest queuing it for the first batch after 2.0
> instead of directly applying to 'master', as we have past the point
> we can expect to see reports of unexpected fallouts and fix the
> issues in time for the final.

Fine with me.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>>
>>> By default, Git used to set $LESS to -FRSX if $LESS was not set by the
>>> user. The FRX flags actually make sense for Git (F and X because Git
>>> sometimes pipes short output to less, and R because Git pipes colored
>>> output). The S flag (chop long lines), on the other hand, is not related
>>> to Git and is a matter of user preference. Git should not decide for the
>>> user to change LESS's default.
>>
>> Git always pipes its output to less,
>
> Err, no, not all commands use a pager.

Yeah, what I meant to say was that when it is told to use pager
(either by an explicit "git -p" or implicitly with the lack of "git
--no-pager"), it does not count the lines shown to avoid passing
short output to the pager.  "sometimes pipes" sounded to me as if we
attempt to do so and sometimes fail to.  But again, as I said, that
is just nitpicking.

>> I am inclined to suggest queuing it for the first batch after 2.0
>> instead of directly applying to 'master', as we have past the point
>> we can expect to see reports of unexpected fallouts and fix the
>> issues in time for the final.
>
> Fine with me.

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Jonathan Nieder]

Hi,

Matthieu Moy wrote:

> By default, Git used to set $LESS to -FRSX if $LESS was not set by the
> user. The FRX flags actually make sense for Git (F and X because Git
> sometimes pipes short output to less, and R because Git pipes colored
> output). The S flag (chop long lines), on the other hand, is not related
> to Git and is a matter of user preference. Git should not decide for the
> user to change LESS's default.

Thanks!  Sounds like a very good change.

(Nit: instead of "because Git sometimes pipes short output to less",
it would be clearer to say something like "when Git pipes short output
to less it is nice to exit and let the user type their next command".)

[...]
> The documentation in config.txt is made a bit longer to keep both an
> example setting the 'S' flag (needed to recover the old behavior) and an
> example showing how to unset a flag set by Git.

Interesting.  Looks good.

For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Matthieu Moy]

----- Original Message -----
> Hi,
> 
> Matthieu Moy wrote:
> 
> > By default, Git used to set $LESS to -FRSX if $LESS was not set by the
> > user. The FRX flags actually make sense for Git (F and X because Git
> > sometimes pipes short output to less, and R because Git pipes colored
> > output). The S flag (chop long lines), on the other hand, is not related
> > to Git and is a matter of user preference. Git should not decide for the
> > user to change LESS's default.
> 
> Thanks!  Sounds like a very good change.
> 
> (Nit: instead of "because Git sometimes pipes short output to less",
> it would be clearer to say something like "when Git pipes short output
> to less it is nice to exit and let the user type their next command".)

It's actually a bit more than this: X to avoid initializing the terminal
and F for the exit behavior you describe.

But since the change is actually not about F and X, I prefered keeping
the text about them as short as possible, so I prefer my version actually.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Junio C Hamano]

Matthieu Moy <matthieu.moy@grenoble-inp.fr> writes:

>> > By default, Git used to set $LESS to -FRSX if $LESS was not set by the
>> > user. The FRX flags actually make sense for Git (F and X because Git
>> > sometimes pipes short output to less, and R because Git pipes colored
>> > output). The S flag (chop long lines), on the other hand, is not related
>> > to Git and is a matter of user preference. Git should not decide for the
>> > user to change LESS's default.
>> 
>> Thanks!  Sounds like a very good change.
>> 
>> (Nit: instead of "because Git sometimes pipes short output to less",
>> it would be clearer to say something like "when Git pipes short output
>> to less it is nice to exit and let the user type their next command".)
>
> It's actually a bit more than this: X to avoid initializing the terminal
> and F for the exit behavior you describe.
>
> But since the change is actually not about F and X, I prefered keeping
> the text about them as short as possible, so I prefer my version actually.

True.

As some of you might know, the version I use for my regular work is
slightly ahead of 'next' (you can see where it is by running "git
log --oneline --first-parent master..pu" and find the first entry
marked as "Merge ... into jch").  After having this patch for a few
days in there and using it, I have to say that I like this change a
lot while viewing the "git log -p" output.

I still find the output from "git blame" disturbing, though.  The
first thing I do in "git blame" output is to scroll to the right in
order to identify the the area I am interested in, and this first
step is not negatively affected, because the right scrolled output 
automatically wraps long lines.

But my second step is to scroll back to the left edge to find the
commit object name and at that point, the new default output without
"S" gets somewhat annoying, because most of the output lines from
"git blame" are longer than my window width.

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> I still find the output from "git blame" disturbing, though.  The
> first thing I do in "git blame" output is to scroll to the right in
> order to identify the the area I am interested in, and this first
> step is not negatively affected, because the right scrolled output 
> automatically wraps long lines.
>
> But my second step is to scroll back to the left edge to find the
> commit object name and at that point, the new default output without
> "S" gets somewhat annoying, because most of the output lines from
> "git blame" are longer than my window width.

git blame sucks in anything but fullscreen either way.  It would help to
display _only_ the source code and have the other info as mouse-over,
but that's not something a pager can do.

It is a pity that the content can be columnized much worse than the
metadata: otherwise it would make much more sense to display the content
_first_ in line.  The metadata is useless without the content anyway.

-- 
David Kastrup

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Matthieu Moy]

David Kastrup <dak@gnu.org> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I still find the output from "git blame" disturbing, though.  The
>> first thing I do in "git blame" output is to scroll to the right in
>> order to identify the the area I am interested in, and this first
>> step is not negatively affected, because the right scrolled output 
>> automatically wraps long lines.
>>
>> But my second step is to scroll back to the left edge to find the
>> commit object name and at that point, the new default output without
>> "S" gets somewhat annoying, because most of the output lines from
>> "git blame" are longer than my window width.
>
> git blame sucks in anything but fullscreen either way.  It would help to
> display _only_ the source code and have the other info as mouse-over,
> but that's not something a pager can do.

Exactly. I personally never use "git blame" outside "git gui blame" for
this reason.

It's possible for a user to set pager.blame to "less -S" to get back to
the previous behavior only for blame.

The idea of having a separate default value for pager.blame (or set
$LESS differently for blame) crossed my mind, but I actually don't like
it, as it would make it harder for a user to fine-tune his configuration
manually (one would have to cancel all the corner-cases that Git would
set by default).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Jeff King]

On Tue, May 06, 2014 at 08:49:22PM +0200, Matthieu Moy wrote:

> Exactly. I personally never use "git blame" outside "git gui blame" for
> this reason.

I'd recommend "tig blame" for this, too, which behaves like "less -S"
with respect to long lines (and also makes it easy to jump to the full
diff, or restart the blame from the parent of the found commit).

> It's possible for a user to set pager.blame to "less -S" to get back to
> the previous behavior only for blame.
> 
> The idea of having a separate default value for pager.blame (or set
> $LESS differently for blame) crossed my mind, but I actually don't like
> it, as it would make it harder for a user to fine-tune his configuration
> manually (one would have to cancel all the corner-cases that Git would
> set by default).

Agreed. We already get some confusion from users with "git has set $LESS
for me".  Changing it to "git set up $LESS depending on which command is
running" seems like it would cause more of the same.

-Peff

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, May 06, 2014 at 08:49:22PM +0200, Matthieu Moy wrote:
> ...
>> The idea of having a separate default value for pager.blame (or set
>> $LESS differently for blame) crossed my mind, but I actually don't like
>> it, as it would make it harder for a user to fine-tune his configuration
>> manually (one would have to cancel all the corner-cases that Git would
>> set by default).
>
> Agreed. We already get some confusion from users with "git has set $LESS
> for me".  Changing it to "git set up $LESS depending on which command is
> running" seems like it would cause more of the same.

While I fully agree with the above conclusion, I just noticed that I
will be irritated enough to eventually set pager.blame myself, after
running a short "git blame -L1311,+7 git-p4.py", which is one of the
standard first steps for me to start reading patches submit on the
list.

Even with line wrapping, the output fits on a single page, so "F"
(aka "--quit-if-one-screen") kicks in, before giving me a chance to
scroll horizontally around.

Not objecting to the conclusion of the discussion with a concrete
counterproposal.  Just hoping somebody clever enough might come up
with a good trick to make things easier to use and explain, which I
however suspect may be an incompatible pair of goals.

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> While I fully agree with the above conclusion, I just noticed that I
> will be irritated enough to eventually set pager.blame myself, after
> running a short "git blame -L1311,+7 git-p4.py", which is one of the
> standard first steps for me to start reading patches submit on the
> list.

Perhaps it deserves a mention in the doc, e.g. squashing this on top of
my patch:

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b7f92ac..ebd1676 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -570,7 +570,9 @@ command to `LESS=FRX less -S`. The environment does not set the
 long lines. Similarly, setting `core.pager` to `less -+F` will
 deactivate the `F` option specified by the environment from the
 command-line, deactivating the "quit if one screen" behavior of
-`less`.
+`less`.  One can specifically activate some flags for particular
+commands: for example, setting `pager.blame` to `less -S` enables
+line truncation only for `git blame`.
 +
 Likewise, when the `LV` environment variable is unset, Git sets it
 to `-c`.  You can override this setting by exporting `LV` with

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2] pager: remove 'S' from $LESS by default

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Perhaps it deserves a mention in the doc, e.g. squashing this on top of
> my patch:

Thanks, will do.

>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index b7f92ac..ebd1676 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -570,7 +570,9 @@ command to `LESS=FRX less -S`. The environment does not set the
>  long lines. Similarly, setting `core.pager` to `less -+F` will
>  deactivate the `F` option specified by the environment from the
>  command-line, deactivating the "quit if one screen" behavior of
> -`less`.
> +`less`.  One can specifically activate some flags for particular
> +commands: for example, setting `pager.blame` to `less -S` enables
> +line truncation only for `git blame`.
>  +
>  Likewise, when the `LV` environment variable is unset, Git sets it
>  to `-c`.  You can override this setting by exporting `LV` with