[msysGit] [PATCH] pager: do not pass -R to less

[Erik Faye-Lund]

It appears there's an issue in the less we use in nsysGit that
causes it to misbehave (by missing lines from the output, among
other things) when using the -R flag with colors.

This patch resolves msysGit issue 484.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

As promised, here's the patch I wrote up for issue 484. I've been
running with this patch for a couple of days now, and haven't
experienced any issues with it.

I think it should be safe to skip -R on other platforms as well,
but I don't know for sure. If I'm wrong, I could rewrite the patch
with some #ifdef'ery...

pager.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/pager.c b/pager.c
index dac358f..24cccb8 100644
--- a/pager.c
+++ b/pager.c
@@ -86,7 +86,7 @@ void setup_pager(void)
pager_process.argv = pager_argv;
pager_process.in = -1;
if (!getenv("LESS")) {
- static const char *env[] = { "LESS=FRSX", NULL };
+ static const char *env[] = { "LESS=FSX", NULL };
pager_process.env = env;
}
#ifndef WIN32
--
1.7.1.1924.g6c1c4

[Johannes Schindelin]

Hi,

On Mon, 24 May 2010, Erik Faye-Lund wrote:

> I think it should be safe to skip -R on other platforms as well,
> but I don't know for sure.

On other platforms, -R is needed for --color to work.

The best thing would be to make LESS_OPTIONS be passed for MinGW as
-DLESS_OPTIONS=\"FSX\" and add an #ifndef ... #define ... "FSRX" #endif.

Ciao,
Dscho

[Erik Faye-Lund]

Thanks for the correction and the suggestion. Will adjust accordingly.

--
Erik "kusma" Faye-Lund

[Albert Dvornik]

> This patch resolves msysGit issue 484.

In addition to the patch for Git, we will need a msysgit.git patch for
/etc/profile, which sets up LESS for the Git/msysGit Bash.

--bert

[Johannes Schindelin]

Hi Albert,

If you make that a proper patch for easy application, I will apply it.

Thanks,
Dscho

[Albert Dvornik]

On Mon, May 24, 2010 at 8:45 AM, Erik Faye-Lund
<kusm...@googlemail.com> wrote:
> On Mon, May 24, 2010 at 2:41 PM, Johannes Schindelin
> <Johannes....@gmx.de> wrote:

[...]

>> The best thing would be to make LESS_OPTIONS be passed for MinGW as
>> -DLESS_OPTIONS=\"FSX\" and add an #ifndef ... #define ... "FSRX" #endif.

It would be the best thing... except that I just noticed that the
pager functions in git-sh-setup.sh and git-svn.sh also set LESS (to
-FRSX) if it's not set already. And presumably it would not be set
when running Git from a cmd.exe prompt.

I have no suggestions for how to (cleanly) get a msysGit-specific
value into those. "git var GIT_LESS_FLAGS" is way too specific, and
hardcoding is just plain ugly.

Thoughts?
--bert

P.S.

On Mon, May 24, 2010 at 11:02 AM, Johannes Schindelin
<Johannes....@gmx.de> wrote:
> On Mon, 24 May 2010, Albert Dvornik wrote:
[...]

>> In addition to the patch for Git, we will need a msysgit.git patch for
>> /etc/profile, which sets up LESS for the Git/msysGit Bash.
>
> If you make that a proper patch for easy application, I will apply it.

Will do (sometime this week).

[Johannes Schindelin]

Hi,

On Mon, 24 May 2010, Albert Dvornik wrote:

> On Mon, May 24, 2010 at 8:45 AM, Erik Faye-Lund
> <kusm...@googlemail.com> wrote:
> > On Mon, May 24, 2010 at 2:41 PM, Johannes Schindelin
> > <Johannes....@gmx.de> wrote:
> [...]
> >> The best thing would be to make LESS_OPTIONS be passed for MinGW as
> >> -DLESS_OPTIONS=\"FSX\" and add an #ifndef ... #define ... "FSRX" #endif.
>
> It would be the best thing... except that I just noticed that the
> pager functions in git-sh-setup.sh and git-svn.sh also set LESS (to
> -FRSX) if it's not set already.

That's right, if it is not set. But we set it.

> And presumably it would not be set when running Git from a cmd.exe
> prompt.

It would, if you set it in git.cmd (unless LESS is set at that stage
already). Would you agree that that would be a good idea?

Ciao,
Dscho

[Albert Dvornik]

On Mon, May 24, 2010 at 7:41 PM, Johannes Schindelin

<Johannes....@gmx.de> wrote:
> Hi,
>
> On Mon, 24 May 2010, Albert Dvornik wrote:
>
>> On Mon, May 24, 2010 at 8:45 AM, Erik Faye-Lund
>> <kusm...@googlemail.com> wrote:
>> > On Mon, May 24, 2010 at 2:41 PM, Johannes Schindelin
>> > <Johannes....@gmx.de> wrote:
>> [...]
>> >> The best thing would be to make LESS_OPTIONS be passed for MinGW as
>> >> -DLESS_OPTIONS=\"FSX\" and add an #ifndef ... #define ... "FSRX" #endif.
>>
>> It would be the best thing... except that I just noticed that the
>> pager functions in git-sh-setup.sh and git-svn.sh also set LESS (to
>> -FRSX) if it's not set already.
>
> That's right, if it is not set. But we set it.
>
>> And presumably it would not be set when running Git from a cmd.exe
>> prompt.
>
> It would, if you set it in git.cmd (unless LESS is set at that stage
> already). Would you agree that that would be a good idea?

It would certainly be a good idea. (I'll include that in the patch I submit.)

Would it cover all the cases, though? Does the "Run Git and included
Unix tools from the Windows command prompt" option put <git-root>\cmd
ahead of <git-root>\bin in the PATH?

If it does, we may not even need to patch the C code...

--bert

[Johannes Schindelin]

It puts cmd in the PATH instead of bin.

Ciao,
Dscho

[Johannes Sixt]

On Dienstag, 25. Mai 2010, Albert Dvornik wrote:
> On Mon, May 24, 2010 at 8:45 AM, Erik Faye-Lund
> <kusm...@googlemail.com> wrote:
> > On Mon, May 24, 2010 at 2:41 PM, Johannes Schindelin
> > <Johannes....@gmx.de> wrote:
> >> The best thing would be to make LESS_OPTIONS be passed for MinGW as
> >> -DLESS_OPTIONS=\"FSX\" and add an #ifndef ... #define ... "FSRX" #endif.
>
> It would be the best thing... except that I just noticed that the
> pager functions in git-sh-setup.sh and git-svn.sh also set LESS (to
> -FRSX) if it's not set already. And presumably it would not be set
> when running Git from a cmd.exe prompt.

Sorry, I seem to have missed some important piece: Why do you want to
remove -R from the options? I'm asking because I'm running git from cmd.exe
(though without the git.cmd wrapper), and I have LESS=R set, i.e., I removed
everything *except* -R. I also have color.ui=auto in my global config to get
nice colors everywhere. So what's the problem?

-- Hannes

[Erik Faye-Lund]

Because the -R option with colors seems to be buggy with some
buffer-sizes, resulting in some diff-lines disappearing. See issue
484:

http://code.google.com/p/msysgit/issues/detail?id=484

--
Erik "kusma" Faye-Lund

[Albert Dvornik]

On Tue, May 25, 2010 at 5:29 AM, Johannes Schindelin
<Johannes....@gmx.de> wrote:
[...]

>> Would it cover all the cases, though?  Does the "Run Git and included
>> Unix tools from the Windows command prompt" option put <git-root>\cmd
>> ahead of <git-root>\bin in the PATH?
>
> It puts cmd in the PATH instead of bin.

Actually, no-- what you're describing is "Run Git from the Windows
command prompt." If you want to run *the other included tools* from
cmd.exe, you clearly need more than git.cmd and gitk.cmd. =) But even
in this case, the installer does put the cmd directory *before* the
bin directory, so we're good. (The InnoSetup config file was
surprisingly easy to read...)

One patch, coming up.

--bert

[Erik Faye-Lund]

From my tests, just setting LESS to "-FSX" doesn't work. That is:

git --no-pager diff --color=always | less -FSX

... works, I get nice colors. While:

LESS="-FSX" git diff

... does not work, I get that nasty "ESC[32m+ESC[mESC[32m"-garbled text.

So there seems to be something strange that I haven't fully understood
going on here. The patch I sent worked for me on Windows (as in, I get
colors and I'm not losing the reported lines). The issue with colors
on other platforms MIGHT not strictly be "-R", but I don't know yet.

Unfortunately, I don't have time to look at this right now. Once I
find some time I'll dig some more into it though... unless someone
else beats me to it, that is.

--
Erik "kusma" Faye-Lund

[bert Dvornik]

The combination of -R and colors causes msysGit's "less" to misbehave.
We make both /etc/profile and git.cmd set LESS to -FSX now, which will
override the default (-FSRX) within Git itself.

This should fix msysGit issue 484 (at least, for users that don't
override LESS or customize their PATH in non-standard ways).

Signed-off-by: bert Dvornik <dvorn...@gmail.com>
---
We may want to stick something about this into the docs someplace,
too, but I'm not sure where. Suggestions?

cmd/git.cmd | 1 +
etc/profile | 2 +-
2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/cmd/git.cmd b/cmd/git.cmd
index 773a2d9..3788f1a 100644
--- a/cmd/git.cmd
+++ b/cmd/git.cmd
@@ -7,6 +7,7 @@
@set PATH=%git_install_root%\bin;%git_install_root%\mingw\bin;%PATH%

@if "%HOME%"=="" @set HOME=%USERPROFILE%
+@if "%LESS%"=="" @set LESS=-FSX
@set PLINK_PROTOCOL=ssh

@if "%1"=="gui" @goto gui
diff --git a/etc/profile b/etc/profile
index 97f8eba..7451dad 100644
--- a/etc/profile
+++ b/etc/profile
@@ -146,7 +146,7 @@ PS1='\[\033]0;$MSYSTEM:\w\007
$ '

# set default options for 'less'
-export LESS=-FRSX
+export LESS=-FSX
# set default protocol for 'plink'
export PLINK_PROTOCOL=ssh

--
1.7.1.1927.g62e28c

[Albert Dvornik]

> This should fix msysGit issue 484 (at least, for users that don't
> override LESS or customize their PATH in non-standard ways).

...but, as Erik pointed out, it just completely breaks everything. I
thought I tested this before committing it, but it's clear I did
something wrong.

Please ignore this patch, and sorry for wasting time.

--bert

[Erik Faye-Lund]

On Wed, May 26, 2010 at 1:38 PM, Erik Faye-Lund

<kusm...@googlemail.com> wrote:
> On Tue, May 25, 2010 at 1:41 AM, Johannes Schindelin
> <Johannes....@gmx.de> wrote:
>> Hi,
>>
>> On Mon, 24 May 2010, Albert Dvornik wrote:
>>
>>> On Mon, May 24, 2010 at 8:45 AM, Erik Faye-Lund
>>> <kusm...@googlemail.com> wrote:
>>> > On Mon, May 24, 2010 at 2:41 PM, Johannes Schindelin
>>> > <Johannes....@gmx.de> wrote:
>>> [...]
>>> >> The best thing would be to make LESS_OPTIONS be passed for MinGW as
>>> >> -DLESS_OPTIONS=\"FSX\" and add an #ifndef ... #define ... "FSRX" #endif.
>>>
>>> It would be the best thing... except that I just noticed that the
>>> pager functions in git-sh-setup.sh and git-svn.sh also set LESS (to
>>> -FRSX) if it's not set already.
>>
>> That's right, if it is not set. But we set it.
>>
>>> And presumably it would not be set when running Git from a cmd.exe
>>> prompt.
>>
>> It would, if you set it in git.cmd (unless LESS is set at that stage
>> already). Would you agree that that would be a good idea?
>>
>
> From my tests, just setting LESS to "-FSX" doesn't work. That is:
>
> git --no-pager diff --color=always | less -FSX
>
> ... works, I get nice colors.

Hmm, but only from bash. cmd.exe gives me the nasty
"ESC[32m+ESC[mESC[32m"-garbled text without -R.

I definitely need to dig further. Just for reference, less --version
reports the same output from both shells:
less 358
Copyright (C) 2000 Mark Nudelman

less comes with NO WARRANTY, to the extent permitted by law.
For information about the terms of redistribution,
see the file named README in the less distribution.

--
Erik "kusma" Faye-Lund

[Johannes Schindelin]

Hi,

On Wed, 26 May 2010, Albert Dvornik wrote:

> On Tue, May 25, 2010 at 5:29 AM, Johannes Schindelin
> <Johannes....@gmx.de> wrote:
> [...]
> >> Would it cover all the cases, though? �Does the "Run Git and included
> >> Unix tools from the Windows command prompt" option put <git-root>\cmd
> >> ahead of <git-root>\bin in the PATH?
> >
> > It puts cmd in the PATH instead of bin.
>
> Actually, no-- what you're describing is "Run Git from the Windows
> command prompt."

Right, my mistake. Sorry.

Ciao,
Dscho

[Albert Dvornik]

On Wed, May 26, 2010 at 7:51 AM, Erik Faye-Lund
<kusm...@googlemail.com> wrote:
[...]

>> From my tests, just setting LESS to "-FSX" doesn't work. That is:
>>
>> git --no-pager diff --color=always | less -FSX
>>
>> ... works, I get nice colors.
>
> Hmm, but only from bash. cmd.exe gives me the nasty
> "ESC[32m+ESC[mESC[32m"-garbled text without -R.

Yes, there's something very bizarre going on here. Your test works
for me, even though I don't have -R in $LESS right now. This also
works:

git --no-pager diff --color=always | LESS=-FSX less
export LESS=-FSX; git --no-pager diff --color=always | less

But all of the following spew escape sequences (!?):

git --no-pager diff --color=always | env LESS=-FSX less
git --no-pager diff --color=always | sh -c 'LESS=-FSX less'
git --no-pager diff --color=always | LESS=-FSX sh -c less
git --no-pager diff --color=always | LESS=-FSX sh -c 'exec less'
export LESS=-FSX
git --no-pager diff --color=always | env less
git --no-pager diff --color=always | sh -c less
git --no-pager diff --color=always | sh -c 'exec less'
git --no-pager diff --color=always > Q; sh -c 'less Q' # and variations

But "less Q" works.

I wonder if there is some weird nastiness going on where we "inherit"
some terminal initialization only if less is a direct child of the
login shell, or somesuch.

> I definitely need to dig further. Just for reference, less --version
> reports the same output from both shells:
> less 358
> Copyright (C) 2000 Mark Nudelman

Hmmmm, maybe we should try a newer version of less. Unfortunately,
I'm complelely swamped for the next few weeks.

--bert

[Erik Faye-Lund]

Perhaps. The GNUWin32 project has less 394[1] (including the patches
to upstream less), which is "only" 5 years old (insted of 10 years old
as 358 is). However, some quick tests indicates that it's even more
broken. When piping anything at all into it, I don't get any output at
all.

Less 418 is the most recent upstream one, it seems.

[1]: http://gnuwin32.sourceforge.net/packages/less.htm

--
Erik "kusma" Faye-Lund