Re: [PATCH v2] pager: remove 'S' from $LESS by default
Jeff King p...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
Junio C Hamano gits...@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/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
Matthieu Moy matthieu@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
Matthieu Moy matthieu@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
Junio C Hamano gits...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@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/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
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 jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
- 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/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
Matthieu Moy matthieu@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@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);
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@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/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
