Re: [PATCH] PAGER_ENV: remove 'S' from $LESS by default
Matthieu Moy 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. -- 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] PAGER_ENV: remove 'S' from $LESS by default
Junio C Hamano writes: > Jeff King 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/ -- 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] PAGER_ENV: remove 'S' from $LESS by default
Jeff King 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 -- 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] PAGER_ENV: remove 'S' from $LESS by default
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 -- 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] PAGER_ENV: remove 'S' from $LESS by default
David Kastrup writes: > Matthieu Moy writes: > >> David Kastrup 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/ -- 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] PAGER_ENV: remove 'S' from $LESS by default
Matthieu Moy writes: > David Kastrup 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 -- 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] PAGER_ENV: remove 'S' from $LESS by default
David Kastrup 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/
--
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] PAGER_ENV: remove 'S' from $LESS by default
Matthieu Moy 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
--
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
[PATCH] PAGER_ENV: remove 'S' from $LESS by default
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
---
> 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
--
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
