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

[Junio C Hamano <gitster@pobox.com>]

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)";
>  }