Bug 579251

Created attachment 432385 [details]
less-436-less-fails-to-decompress-empty-files.patch

proposing patch

    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
Previously, viewing a compressed zero-byte file lead to unwanted messages because it failed to decompress empty files. With this update, the decompressed contents of zero-byte files are shown in the same way as larger text files.

problem is pretty tricky

<snip man>
It  is  also  possible  to set up an input preprocessor to pipe the file
data directly to less, rather than putting the data into a replacement
file.  This avoids the need to decompress the entire file before starting
to view it.
</snip man>

        $ cat lesspipe.sh
        #!/bin/bash
        case "$1" in
                *.gz) gzip -dc -- "$1" ;;
        esac

1) If empty gzip compressed file is passed to lesspipe.sh and file is
caught by case and decompressed into pipe(stdout) that means pipe is *empty*

2) If file is passed to lesspipe.sh and file is *not* caught by case
pipe is *empty* too.

My solution is to add exit code into lesspipe.sh like that

        $ cat lesspipe.sh
        #!/bin/bash
        case "$1" in
                *.gz) gzip -dc -- "$1" ;;
                *) exit 127
        esac

If exit code is not equal to 127 and pipe is empty, it means that preprocessor
decompressed empty file and pipe must be opened.

Solution works till someone set printexitvalue=1 in tcsh.
<snip tcsh man>
The *printexitvalue* shell variable can be set to print the exit status of commands which exit with a status other than zero.
</snip tcsh man>

God knows why tcsh prints message *Exit `status`* to stdout and in my solution into pipe. 

There is one solution. Don't use pipe, but prepare file elsewhere e.g. /tmp and when file is ready than open it. This solution brings bad performance when file is compressed and big. It needs to be decompressed first which could take awhile.

Possible solution:

lessopen.sh:
            #! /bin/sh
            case "$1" in
            *.gz) uncompress -; echo /tmp/less.$$
                 ;;
            esac

lessclose.sh:
            #! /bin/sh
            rm $2

Reopening the bug, as we have the working fix now (attached in Bug #615303).

Setting Depends on Bug #531353 (tcsh obeys printexitvalue for back-ticks) and rhel-5.7? flag.

Setting Cond NACK UPSTREAM, as we still don't have any response from the upstream.

Even though the patch is backward-compatible also with 3rd party scripts (and
Fedora proves it's functionality), we can't be sure the patch won't break
LESSOPEN string compatibility with future upstream versions. For example, this
could eventually cause some difficulties while upgrading to RHEL-6 and using
customized or 3rd party scripts.

Upstream response:

> > Hi Vojtech,
> > Sorry I haven't responded earlier.  I did receive your patch but haven't had much
> > time to work on less issues lately.  My major concerns would be the two points that
> > you yourself raised -- its use of /dev/null is Unix-specific, and the potential
> > incompatibility with existing LESSOPEN scripts that may return random exit codes.
> > The second issue is the most serious, and I think we'd need to have some way to 
> > distinguish "new" LESSOPEN scripts that use the exit code for this purpose from
> > old ones.
> Actually, in Fedora, we are using *two vertical bars* in row as a workaround:
>     eg. LESSOPEN="||/usr/bin/lesspipe.sh %s"
I see.  I'm a little leery of the proliferation of cryptic characters appearing at the start of the LESSOPEN string, first "|", then "-|", now "||" and I suppose "-||" is also possible.  But it's not bad.  If we can find a way to solve the /dev/null issue, it would be fine.

--Mark
> 
> See the following link for the whole Fedora changeset:
> http://pkgs.fedoraproject.org/gitweb/?p=less.git;a=blob;f=less-436-empty-lessopen-pipe.patch;h=69f84ca1287c55e5d948d0be3763797941fc3523;hb=HEAD
> 
> I think it would be great if we could unify this "new" exit code behaviour in the similar manner,
> as it is backward-compatible with old scripts, and also it provides the "correct" behaviour.
> 
> So if you agreed with similar solution, we would have to deal just with /dev/null issue.
>  
> - Vojtech
> >
> > --Mark

Upstream won't merge the changes until we have the "/dev/null" solution fixed for non-unix systems. Still Cond NACK UPSTREAM..

Anyone able to find a /dev/null solution to non-unix systems?

The component has changed its owner to me, so I'm assigning the bug to myself.

I wrote a portable patch and sent it to upstream to check it. Mark Nudelmann (the author) replied on Friday:

>Hi Martin,
>Thank you for the patch.  I have actually implemented this feature in a
>different way, using a special filename similar to the way FAKE_HELPFILE
>is used.  It's kludgy, but at least it has precedent.
>
>I'll release this in a beta soon.
>
>--Mark
>
>
>
>On 6/13/2012 6:56 AM, Martin Briza wrote:
>> Hello,
>>
>> I'm the new maintainer of less in RHEL and Fedora and Vojtech told me
>> about your communication regarding empty files as output from input
>> preprocessor.
>> I think I solved the portability problem with opening /dev/null to get
>> empty output to print with a small hack - opening the input file again
>> (as it should still be there to be read) and seeking to its end to get
>> EOF immediately.
>> There is the patch (based upon the one Vojtech has sent you) attached to
>> this mail.
>> I know it's not a clean solution but I'm afraid there isn't much more
>> options. Anyway, I'll understand perfectly if you refuse it.
>>
>> Thanks in advance,
>>
>> Martin Briza

I'm absolutely sure his implementation is better so I will wait until he releases his version and merge it into our tree.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Created attachment 667150 [details]
Patch extracted from the upstream package

I'm proposing fixing this bug with this patch. It is included in the upstream packages since version 451. However, using it would introduce Bug 845527 to RHEL5, too, as exit statuses of the programs in the $LESSOPEN script are now handled. This bug applies only to tcsh. Also, using the printexitvalue variable outside of debugging environments is not a standard situation.

Created attachment 667151 [details]
Fedora's lesspipe script for $LESSOPEN

It would also be necessary to change the script in the $LESSOPEN variable to reflect the changes and provide necessary exit statuses. I think using the one from Fedora is reasonable.

This Bugzilla has been reviewed by Red Hat and is not planned on being
addressed in Red Hat Enterprise Linux 5, and therefore will be closed.
If this bug is critical to production systems, please contact your Red
Hat support representative and provide sufficient business
justification. 

In Red Hat Enterprise Linux 6 please follow bug #615303.
In Red Hat Enterprise Linux 7 is this issue fixed.