Bug 805735

Summary:	Foption patch depends on uninitialised variable when reading from stdin and also implies -X option
Product:	[Fedora] Fedora	Reporter:	Colin Guthrie <colin>
Component:	less	Assignee:	Vojtech Vitek <vvitek>
Status:	CLOSED RAWHIDE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	unspecified	Docs Contact:
Priority:	unspecified
Version:	17	CC:	hripps, vvitek
Target Milestone:	---
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2012-05-14 16:42:59 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:

Description Colin Guthrie 2012-03-21 23:21:26 UTC

The patch in the F17 and rawhide less packages to fix up Foption is broken in a couple different ways.

I don't personally use Fedora, but I noticed this while reviewing your patches for inclusion in Mageia.

Here is a description I previously sent to Vojtech Vitek. He requested I open a bug to track this.

I was just debugging some annoying behaviour in "less" last night in
Mageia. Years ago (c. 2005) we, ahem, borrowed the Foption patch from
RedHat.

As I periodically look for improvements etc, I checked the current
Fedora version of this patch.

http://pkgs.fedoraproject.org/gitweb/?p=less.git;a=blob;f=less-443-Foption.patch;h=d657d4455a0e92495c5ff5c6347a17a03a90f204;hb=HEAD

In my use case however, I could not make it work.

I wanted to use -FR as my default options, but no matter what I tried,
the -F option always seemed to also imply the -X option too.

The -X option sadly has the side effect of breaking mouse scroll wheel
integration while using less under e.g. gnome-terminal.

So I dug into the code a little bit and found two bugs with the above patch.

The first is that the line count function always returned 0. It seems
that position(TOP) call always returned the NULL_POSITION thus never
counted any lines.

Secondly the value for line_count was only initialised when working with
files, not when working with stdin, so the behaviour in that case would
have been non-deterministic based on whatever random memory value was in
the variable.

The patch should fix it up and seems to work for me:

http://svnweb.mageia.org/packages/cauldron/less/current/SOURCES/less-445-Foption-fix.patch?revision=224291&view=markup

Any further review would be appreciated.

Comment 1 Vojtech Vitek 2012-05-14 16:42:59 UTC

Hi Colin,

thank you for your patch. I've reviewed it briefly and tested as well.

I've merged the changes into the original Foption patch and created the Foption.v2.patch, see http://pkgs.fedoraproject.org/gitweb/?p=less.git;a=blob;f=less-444-Foption.v2.patch;h=eb9140da8e4714e2dc7e423f56416992fd315c5f;hb=47da4db53ff5f00cfa893e05c9995959b8491242

Let's see how the Fedora community accepts the -F behavior now while testing Rawhide.

Comment 2 Colin Guthrie 2012-05-15 09:51:23 UTC

Annoyingly my colleague pointed out a small problem with my patch...

It relates to low-flow data...

(i=1; while [ $i -lt 50 ]; do echo "This is line $i"; i=$((i + 1)); sleep 0.5; done) | less

vs.

(i=1; while [ $i -lt 50 ]; do echo "This is line $i"; i=$((i + 1)); sleep 0.5; done) | less -F


In the latter case it waits for 0.5*21 = 11s before displaying any of the data.

Due to the kind of data I work with and the places where -F is used, I personally don't have a problem with this, but there should really be more code added to cope with this kind of situation :s

Comment 3 Colin Guthrie 2012-05-15 09:52:09 UTC

(PS I was rounding up before you point out my lack of math skills :p)

Comment 4 Vojtech Vitek 2012-05-15 18:25:12 UTC

(In reply to comment #2)
> (i=1; while [ $i -lt 50 ]; do echo "This is line $i"; i=$((i + 1)); sleep 0.5;
> done) | less -F
> 
> In the latter case it waits for 0.5*21 = 11s before displaying any of the data.

I can't reproduce this. Could you please test the update packages, eg. less-444-7.fc18? Thanks!

Comment 5 Colin Guthrie 2012-05-17 08:30:28 UTC

Hmm, I don't have an handy Fedora install sadly, but I tested the patch itself and I can still reproduce it with that applied rather than the previous two patches I had. We do however have a newer version of less than you guys (445 vs 444) so that may explain the difference in behaviour.

That said, I can quite easily see in the patched code how this happens: The get_line_count() function will always count up to as many lines as the screen is high. If the input is coming from stdin, and there has not yet been an EOF, it will wait until that many lines exist, thus delaying display of ANY output until the screen is full.

In an ideal world it would start displaying (almost) immediately and only switch to the scrolly output when the screen is full.


Anyway, I'm using this as a "less" bug tracker rather than a fedora one so that's likely not overly helpful to you. I can bow out of the discussion if you like (especially if you cannot reproduce)?