"less -F" is broken

"less -F" is broken

[Linus Torvalds]

Sadly, as of less-530, the behavior of "less -F" is broken enough that
I think git needs to potentially think about changing the defaults for
the pager, or people should at least be aware of it.

Older versions of less (at least up to less-487 - March 2017) do not
have this bug.  There were apparently 520, 527 and 529 releases in
2017 too, but I couldn't find their sources to verify if they were
already broken - but 530 (February 2018) has the problem.

The breakage is easy to see without git:

        (echo "hello"; sleep 5; echo "bye bye") | less -F

which will result in no output at all for five seconds, and then you
get both lines at once as "less" exits.

It's not always obvious when using git, because when the terminal
fills up, less also starts outputting, but the default options with -F
are really horrible if you are looking for something uncommon, and
"git log" doesn't respond at all.

On the kernel tree, this is easy to see with something like

   git log --oneline --grep="The most important one is the mpt3sas fix"

which takes a bit over 7 seconds before it shows the commit I was looking for.

In contrast, if you do

   LESS=-RX git log --oneline --grep="The most important one is the mpt3sas fix"

that (recent) commit is found and shown immediately. It still takes 7s
for git to go through all history and decide "that was it", but at
least you don't need to wait for the intermediate results.

I've reported it as a bug in less, but I'm not sure what the reaction
will be, the less releases seem to be very random.

             Linus

Re: "less -F" is broken

[Stefan Beller]

On Wed, Aug 15, 2018 at 1:35 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Sadly, as of less-530, the behavior of "less -F" is broken enough that
> I think git needs to potentially think about changing the defaults for
> the pager, or people should at least be aware of it.
>
> Older versions of less (at least up to less-487 - March 2017) do not
> have this bug.  There were apparently 520, 527 and 529 releases in
> 2017 too, but I couldn't find their sources to verify if they were
> already broken - but 530 (February 2018) has the problem.

http://www.greenwoodsoftware.com/less/news.527.html
http://www.greenwoodsoftware.com/less/news.520.html
http://www.greenwoodsoftware.com/less/
Release notes for 520 and 527 contains:
 "Don't output terminal init sequence if using -F and file fits on one screen."

Re: "less -F" is broken

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 15 2018, Linus Torvalds wrote:

> Sadly, as of less-530, the behavior of "less -F" is broken enough that
> I think git needs to potentially think about changing the defaults for
> the pager, or people should at least be aware of it.

Downloading & trying versions of it locally reveals that it's as of
version 520, not 530. The last version before 520 is 487. Presumably
it's covered by this item in the changelog:

    Don't output terminal init sequence if using -F and file fits on one
    screen[1]

> Older versions of less (at least up to less-487 - March 2017) do not
> have this bug.  There were apparently 520, 527 and 529 releases in
> 2017 too, but I couldn't find their sources to verify if they were
> already broken - but 530 (February 2018) has the problem.

FWIW they're not linked from
http://www.greenwoodsoftware.com/less/download.html but you can just URL
hack and see releases http://www.greenwoodsoftware.com/less/ and change
links like http://www.greenwoodsoftware.com/less/less-530.tar.gz to
http://www.greenwoodsoftware.com/less/less-520.tar.gz

> The breakage is easy to see without git:
>
>         (echo "hello"; sleep 5; echo "bye bye") | less -F
>
> which will result in no output at all for five seconds, and then you
> get both lines at once as "less" exits.

The relevant change in less is this, cutting out the non-relevant parts:

    diff --git a/less-487/forwback.c b/less-520/forwback.c
    index 83ae78e..680fa25 100644
    --- a/less-487/forwback.c
    +++ b/less-520/forwback.c
    [...]
    @@ -444,3 +444,21 @@ get_back_scroll()

     		return (sc_height - 2);
     	return (10000); /* infinity */
     }
    +
    +/*
    + * Return number of displayable lines in the file.
    + * Stop counting at screen height + 1.
    + */
    +	public int
    +get_line_count()
    +{
    +	int nlines;
    +	POSITION pos = ch_zero();
    +
    +	for (nlines = 0;  nlines <= sc_height;  nlines++)
    +	{
    +		pos = forw_line(pos);
    +		if (pos == NULL_POSITION) break;
    +	}
    +	return nlines;
    +}
    [...]
    diff --git a/less-487/main.c b/less-520/main.c
    index 960d120..6d54851 100644
    --- a/less-487/main.c
    +++ b/less-520/main.c
    [...]
    @@ -273,10 +275,19 @@ main(argc, argv)
     	{
     		if (edit_stdin())  /* Edit standard input */
     			quit(QUIT_ERROR);
    +		if (quit_if_one_screen)
    +			line_count = get_line_count();
     	} else
     	{
     		if (edit_first())  /* Edit first valid file in cmd line */
     			quit(QUIT_ERROR);
    +		if (quit_if_one_screen)
    +		{
    +			if (nifile() == 1)
    +				line_count = get_line_count();
    +			else /* If more than one file, -F can not be used */
    +				quit_if_one_screen = FALSE;
    +		}
     	}

     	init();
    diff --git a/less-487/screen.c b/less-520/screen.c
    index ad3fca1..2d51bbc 100644
    --- a/less-487/screen.c
    +++ b/less-520/screen.c
    [...]
    @@ -1538,7 +1555,9 @@ win32_deinit_term()
     init()
     {
     #if !MSDOS_COMPILER
    -	if (!no_init)
    +	if (quit_if_one_screen && line_count >= sc_height)
    +		quit_if_one_screen = FALSE;
    +	if (!no_init && !quit_if_one_screen)
     		tputs(sc_init, sc_height, putchr);
     	if (!no_keypad)
     		tputs(sc_s_keypad, sc_height, putchr);

If you undo that first changed part in main.c your test case prints
"hello" to the terminal immediately.

> It's not always obvious when using git, because when the terminal
> fills up, less also starts outputting, but the default options with -F
> are really horrible if you are looking for something uncommon, and
> "git log" doesn't respond at all.
>
> On the kernel tree, this is easy to see with something like
>
>    git log --oneline --grep="The most important one is the mpt3sas fix"
>
> which takes a bit over 7 seconds before it shows the commit I was looking for.
>
> In contrast, if you do
>
>    LESS=-RX git log --oneline --grep="The most important one is the mpt3sas fix"
>
> that (recent) commit is found and shown immediately. It still takes 7s
> for git to go through all history and decide "that was it", but at
> least you don't need to wait for the intermediate results.
>
> I've reported it as a bug in less, but I'm not sure what the reaction
> will be, the less releases seem to be very random.

Via bug-less@gnu.org? Is this report available online somewhere? Anyway,
CC-ing that address since my digging into this will be useful to them.

1. http://www.greenwoodsoftware.com/less/news.520.html

Re: "less -F" is broken

[Linus Torvalds]

On Wed, Aug 15, 2018 at 2:29 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> FWIW they're not linked from
> http://www.greenwoodsoftware.com/less/download.html but you can just URL
> hack and see releases http://www.greenwoodsoftware.com/less/ and change
> links like http://www.greenwoodsoftware.com/less/less-530.tar.gz to
> http://www.greenwoodsoftware.com/less/less-520.tar.gz

I should have just tried that. I just downloaded the ones linked to,
made a git archive of the history, and started bisecting. Which was
all pointless extra work, since it was in the last release, but
whatever.

> > I've reported it as a bug in less, but I'm not sure what the reaction
> > will be, the less releases seem to be very random.
>
> Via bug-less@gnu.org?

Heh. Another thing I didn't actually find. No, I just emailed Mark
Nudelman directly, because that's what the FAQ says to do:

 "There is a list of known bugs here. If you find a bug that is not in
the list, please send email to the author. Describe the bug in as much
detail as possible, and I'll do what I can to help resolve the
problem."

and it doesn't mention any mailing list.

> Is this report available online somewhere?

It was not all that different from the email to the git list - just
giving the trivial test-case and my (limited) bisection result.

The data you dug up is much more useful.

            Linus

Re: "less -F" is broken

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On Wed, Aug 15, 2018 at 2:29 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> Downloading & trying versions of it locally reveals that it's as of
> version 520, not 530. The last version before 520 is 487. Presumably
> it's covered by this item in the changelog:
>
>     Don't output terminal init sequence if using -F and file fits on one
>     screen[1]

Side note: that's sad, because we already use X in the default exactly
for that reason.

So apparently "less" was broken for us to fix something that we
already had long solved. The code basically tried to do "automatic X
when F is set".

And all that line_count stuff (which is what breaks) is pointless when
-X is already given.

That does give a possible fix: just stop doing the line_count thing if
no_init is set.

So "-F" would continue to be broken, but "-FX" would work.

Something like the attached patch, perhaps?

            Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 523 bytes --]

 main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/main.c b/main.c
index 179bd78..961a9db 100644
--- a/main.c
+++ b/main.c
@@ -59,6 +59,7 @@ extern int	missing_cap;
 extern int	know_dumb;
 extern int	pr_type;
 extern int	quit_if_one_screen;
+extern int	no_init;
 
 
 /*
@@ -274,7 +275,7 @@ main(argc, argv)
 	{
 		if (edit_stdin())  /* Edit standard input */
 			quit(QUIT_ERROR);
-		if (quit_if_one_screen)
+		if (quit_if_one_screen && !no_init)
 			line_count = get_line_count();
 	} else 
 	{

Re: "less -F" is broken

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 15 2018, Linus Torvalds wrote:

> On Wed, Aug 15, 2018 at 2:29 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> Downloading & trying versions of it locally reveals that it's as of
>> version 520, not 530. The last version before 520 is 487. Presumably
>> it's covered by this item in the changelog:
>>
>>     Don't output terminal init sequence if using -F and file fits on one
>>     screen[1]
>
> Side note: that's sad, because we already use X in the default exactly
> for that reason.

And as another note for those following along (and myself until a short
while ago, I didn't remember how this worked).

We set those default options at compile-time here:
https://github.com/git/git/blob/63749b2dea5d1501ff85bab7b8a7f64911d21dea/Makefile#L1761-L1763

I.e. set LESS=FRX and then when we setup the pager we use that unless we
can find LESS (and LV) set in the env already:
https://github.com/git/git/blob/63749b2dea5d1501ff85bab7b8a7f64911d21dea/pager.c#L71-L96

> So apparently "less" was broken for us to fix something that we
> already had long solved. The code basically tried to do "automatic X
> when F is set".
>
> And all that line_count stuff (which is what breaks) is pointless when
> -X is already given.
>
> That does give a possible fix: just stop doing the line_count thing if
> no_init is set.
>
> So "-F" would continue to be broken, but "-FX" would work.
>
> Something like the attached patch, perhaps?

This works for me under -FX.

>             Linus
>  main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/main.c b/main.c
> index 179bd78..961a9db 100644
> --- a/main.c
> +++ b/main.c
> @@ -59,6 +59,7 @@ extern int	missing_cap;
>  extern int	know_dumb;
>  extern int	pr_type;
>  extern int	quit_if_one_screen;
> +extern int	no_init;
>
>
>  /*
> @@ -274,7 +275,7 @@ main(argc, argv)
>  	{
>  		if (edit_stdin())  /* Edit standard input */
>  			quit(QUIT_ERROR);
> -		if (quit_if_one_screen)
> +		if (quit_if_one_screen && !no_init)
>  			line_count = get_line_count();
>  	} else
>  	{

Re: "less -F" is broken

[Mark Nudelman]

-X is a workaround for the previous behavior of -F, but it's not a great 
solution.  By not sending the terminal init sequence, some things can be 
broken, depending on the terminal.  For example, some terminals send the 
"wrong" sequences for the arrow keys when the terminal doesn't receive 
the init sequence.  For that reason and similar ones, I've never liked 
-X.  The change in behavior for -F was to deal with some other types of 
(arguably broken) terminals that switch to an alternate screen when they 
receive the init sequence.  This makes -F fairly useless on such 
terminals.  However this does change the behavior to the one Linus 
objected to, where the first page is not output until we know whether it 
fits on the screen, so any delays in the first screen will delay all 
output.  (Note that this doesn't happen for delays that occur after the 
first screen has been displayed.)

So I'm not sure what the best solution is.  Linus's proposal to disable 
the line counting stuff if -X is set seems reasonable.  I will look into 
that and see if there are any issues with it.

--Mark

On 8/15/2018 2:57 PM, Linus Torvalds wrote:
> On Wed, Aug 15, 2018 at 2:29 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> Downloading & trying versions of it locally reveals that it's as of
>> version 520, not 530. The last version before 520 is 487. Presumably
>> it's covered by this item in the changelog:
>>
>>      Don't output terminal init sequence if using -F and file fits on one
>>      screen[1]
> 
> Side note: that's sad, because we already use X in the default exactly
> for that reason.
> 
> So apparently "less" was broken for us to fix something that we
> already had long solved. The code basically tried to do "automatic X
> when F is set".
> 
> And all that line_count stuff (which is what breaks) is pointless when
> -X is already given.
> 
> That does give a possible fix: just stop doing the line_count thing if
> no_init is set.
> 
> So "-F" would continue to be broken, but "-FX" would work.
> 
> Something like the attached patch, perhaps?
> 
>              Linus
> 

Re: "less -F" is broken

[Linus Torvalds]

On Thu, Aug 16, 2018 at 9:50 AM Mark Nudelman
<markn@greenwoodsoftware.com> wrote:
>
> So I'm not sure what the best solution is.  Linus's proposal to disable
> the line counting stuff if -X is set seems reasonable.  I will look into
> that and see if there are any issues with it.

One option that I didn't try to go for - because I just don't know the
less code base well enough - is to basically make the behavior of '-F'
be something like this:

 - as long as all the lines are short and well-behaved, and we haven't
seen enough lines to fill the screen, act like 'cat' and just feed
them through

 - when you fill the screen (or when you hit some other condition that
makes you go "now I won't exit" - that could be a long line, but maybe
it could also be the user giving keyboard input for a less command?)
you send the init sequence and just redraw the whole screen.

That sounds like the best of both worlds.

In fact, right now "less -F" is in my opinion a bit broken in other
ways that my patch doesn't fix.

Do this:

    (echo 1; sleep 10; echo 2) | LESS=FX less

and with my patch it will show "a" immediately. So far so good.

But let's say that that was all the user was interested in, and the
user presses 'q' to quit less. That doesn't work at all - it will wait
for that full ten seconds.

That actually happens even without -F too.

Wouldn't it be good to react to things like searches to highlight
something (and to 'quit' for the 'never mind, alteady got it' case)
even if there isn't enough data to fill the whole screen yet?

that said, ^C works, and this is not new behavior, so I'm just
throwing this out as a "maybe a different approach would fix _both_
the -F behavior _and_ the above traditional issue"?

                        Linus

Re: "less -F" is broken

[Mark Nudelman]

On 8/16/2018 10:10 AM, Linus Torvalds wrote:
> One option that I didn't try to go for - because I just don't know the
> less code base well enough - is to basically make the behavior of '-F'
> be something like this:
> 
>   - as long as all the lines are short and well-behaved, and we haven't
> seen enough lines to fill the screen, act like 'cat' and just feed
> them through
> 
>   - when you fill the screen (or when you hit some other condition that
> makes you go "now I won't exit" - that could be a long line, but maybe
> it could also be the user giving keyboard input for a less command?)
> you send the init sequence and just redraw the whole screen.

I'm not sure that this would be a very nice user experience.  On a 
terminal where the init sequence opens an alternate screen, some lines 
from the start of the file would be printed in the main screen, and then 
the whole file would be viewed in the alternate screen.  After exiting 
less, the user would see his main screen with some lines from the first 
page of the file displayed and then cut off at a seemingly arbitrary 
point.  Seems like that could be confusing and annoying.


> But let's say that that was all the user was interested in, and the
> user presses 'q' to quit less. That doesn't work at all - it will wait
> for that full ten seconds.
> 
> That actually happens even without -F too.
> 
> Wouldn't it be good to react to things like searches to highlight
> something (and to 'quit' for the 'never mind, alteady got it' case)
> even if there isn't enough data to fill the whole screen yet?
> 
> that said, ^C works, and this is not new behavior, so I'm just
> throwing this out as a "maybe a different approach would fix _both_
> the -F behavior _and_ the above traditional issue"?

This issue is, as you say, not related to the -F issue, but arises 
because less doesn't have a way to be reading a file and simultaneously 
react to terminal key presses.  When I first wrote less, there was no 
easy way to do this in Unix.  Less also runs on other OSes which don't 
provide this functionality.  The best I was able to do was to allow 
ctrl-C to interrupt the read.  Of course in a modern OS that has 
select() or similar functionality this could be implemented, but I think 
it would require some largish changes to the architecture.  (Or maybe 
not; I haven't really investigated this in detail.)

BTW, your first message seems to indicate that you didn't find the less 
project on github.  It's at https://github.com/gwsw/less (mentioned in 
the README).  The latest version (v535) has the -F change implemented, 
but I haven't yet released this for beta testing.

--Mark