* "less -F" is broken @ 2018-08-15 20:35 Linus Torvalds 2018-08-15 21:23 ` Stefan Beller 2018-08-15 21:29 ` Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 9+ messages in thread From: Linus Torvalds @ 2018-08-15 20:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "less -F" is broken 2018-08-15 20:35 "less -F" is broken Linus Torvalds @ 2018-08-15 21:23 ` Stefan Beller 2018-08-15 21:29 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 9+ messages in thread From: Stefan Beller @ 2018-08-15 21:23 UTC (permalink / raw) To: Linus Torvalds; +Cc: Junio C Hamano, git 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." ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "less -F" is broken 2018-08-15 20:35 "less -F" is broken Linus Torvalds 2018-08-15 21:23 ` Stefan Beller @ 2018-08-15 21:29 ` Ævar Arnfjörð Bjarmason 2018-08-15 21:43 ` Linus Torvalds 2018-08-15 21:57 ` Linus Torvalds 1 sibling, 2 replies; 9+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-15 21:29 UTC (permalink / raw) To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List, bug-less 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "less -F" is broken 2018-08-15 21:29 ` Ævar Arnfjörð Bjarmason @ 2018-08-15 21:43 ` Linus Torvalds 2018-08-15 21:57 ` Linus Torvalds 1 sibling, 0 replies; 9+ messages in thread From: Linus Torvalds @ 2018-08-15 21:43 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Junio C Hamano, Git Mailing List, bug-less 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "less -F" is broken 2018-08-15 21:29 ` Ævar Arnfjörð Bjarmason 2018-08-15 21:43 ` Linus Torvalds @ 2018-08-15 21:57 ` Linus Torvalds 2018-08-16 8:22 ` Ævar Arnfjörð Bjarmason 2018-08-16 16:50 ` Mark Nudelman 1 sibling, 2 replies; 9+ messages in thread From: Linus Torvalds @ 2018-08-15 21:57 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Junio C Hamano, Git Mailing List, bug-less [-- 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 { ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: "less -F" is broken 2018-08-15 21:57 ` Linus Torvalds @ 2018-08-16 8:22 ` Ævar Arnfjörð Bjarmason 2018-08-16 16:50 ` Mark Nudelman 1 sibling, 0 replies; 9+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-16 8:22 UTC (permalink / raw) To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List, bug-less 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 > { ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "less -F" is broken 2018-08-15 21:57 ` Linus Torvalds 2018-08-16 8:22 ` Ævar Arnfjörð Bjarmason @ 2018-08-16 16:50 ` Mark Nudelman 2018-08-16 17:10 ` Linus Torvalds 1 sibling, 1 reply; 9+ messages in thread From: Mark Nudelman @ 2018-08-16 16:50 UTC (permalink / raw) To: Linus Torvalds, Ævar Arnfjörð Bjarmason Cc: Junio C Hamano, Git Mailing List, bug-less -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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "less -F" is broken 2018-08-16 16:50 ` Mark Nudelman @ 2018-08-16 17:10 ` Linus Torvalds 2018-08-20 15:54 ` Mark Nudelman 0 siblings, 1 reply; 9+ messages in thread From: Linus Torvalds @ 2018-08-16 17:10 UTC (permalink / raw) To: Mark Nudelman Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano, Git Mailing List, bug-less 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "less -F" is broken 2018-08-16 17:10 ` Linus Torvalds @ 2018-08-20 15:54 ` Mark Nudelman 0 siblings, 0 replies; 9+ messages in thread From: Mark Nudelman @ 2018-08-20 15:54 UTC (permalink / raw) To: Linus Torvalds Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano, Git Mailing List 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-08-20 15:54 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-15 20:35 "less -F" is broken Linus Torvalds 2018-08-15 21:23 ` Stefan Beller 2018-08-15 21:29 ` Ævar Arnfjörð Bjarmason 2018-08-15 21:43 ` Linus Torvalds 2018-08-15 21:57 ` Linus Torvalds 2018-08-16 8:22 ` Ævar Arnfjörð Bjarmason 2018-08-16 16:50 ` Mark Nudelman 2018-08-16 17:10 ` Linus Torvalds 2018-08-20 15:54 ` Mark Nudelman
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).