cowsay-org / cowsay

apjanke's fork of the classic cowsay project
http://cowsay.diamonds
GNU General Public License v3.0
83 stars 15 forks source link

cowsay -n: "Undefined subroutine &main::display_usage" #20

Open ndim opened 2 years ago

ndim commented 2 years ago

This was originally reported as https://bugzilla.redhat.com/show_bug.cgi?id=2103455.

Basically, calling cowsay -n moo produces

$ cowsay --version
/usr/bin/cowsay version 3.7.0 calling Getopt::Std::getopts (version 1.13),
running under Perl version 5.34.1.
$ cowsay -n moo
Undefined subroutine &main::display_usage called at ./cowsay line 238.
$ echo s=$?
s=2
$ _

while the commands cowsay moo and echo moo | cowsay -n both produce cows as expected.

The HELP_MESSAGE output describes the cowsay usage as

Usage:

    $progname [-bdgpstwy] [-e <eyes>] [-f <cowfile> | -r [-C] ] 
        [-n] [-T <tongue>] [-W <wrapcolumn>]
        <message>

which means cowsay -n <message> should be valid cowsay usage.

I am not sure how much sense cowsay -n <message> actually makes.

Confusingly, the information about the -n option is not found in the man page's OPTIONS section, but is a paragraph in DESCRIPTION.

The cowsay.1 man page says

If -n is specified, there must not be any command line arguments left after all the switches have been processed.

so it appears that cowsay should run the HELP_MESSAGE function and then abort and quit the program instead of trying to call the non-existent display_usage function.

Also, the message HELP_MESSAGE prints could mention that cowsay -n ANYTHING is considered an error.

This problem was probably introduced in the https://github.com/cowsay-org/cowsay/commit/f863e586480d984380bfec6bbd7148895df85fdc commit.

apjanke commented 2 years ago

That "undefined subroutine" error is a bug all right.

I am not sure how much sense cowsay -n actually makes.

I think it does make sense. As I understand it, the -n option just affects the output behavior so it doesn't do any word wrapping. So I'd think that most of the other calling forms of cowsay, including the one with the message on the command line, should work with -n too, in some forms.

I'm don't understand the reason that the original author had for forbidding additional command line arguments after switch/option processing in the case where -n is present. But since I don't understand it, I probably shouldn't alter that behavior until I do understand it. :)

The HELP_MESSAGE output ... which means cowsay -n should be valid cowsay usage.

Yeah, I think the HELP_MESSAGE output does not correctly describe the valid usage in the presence of the -n option.

The right fix might be this:

    $progname [-bdgpstwy] [-e <eyes>] [-f <cowfile> | -r [-C] ] 
        [-n] [-T <tongue>] [-W <wrapcolumn>]
        [<message>]

... with the brackets around <message> to indicate that it is actually optional. That reflects both the "no message with -n" behavior, and the fact that even without -n, you can just omit the <message> in the command, in which case cowsay will read from stdin instead.

This might be something that I introduced myself in this cowsay fork. Running cowsay -h against an older upstream version does have the square brackets around message.

cowsay -h
cow{say,think} version 3.03, (c) 1999 Tony Monroe
Usage: cowsay [-bdgpstwy] [-h] [-e eyes] [-f cowfile]
          [-l] [-n] [-T tongue] [-W wrapcolumn] [message]

I think what I did is changed those square brackets to angle brackets to indicate that "message" was a placeholder and not the literal text "message" (like I did with the words inside the options, like "eyes" and "cowfile"), which is conventionally indicated with angle brackets, but I didn't understand that the message really was optional, so those square brackets around "message" should have also been retained.

With some other programs, it is conventional to use the argument - (just a dash) or some other option to request reading from stdin instead of a file or string given on the command line. But not for all programs.

Confusingly, the information about the -n option is not found in the man page's OPTIONS section, but is a paragraph in DESCRIPTION.

Yeah, I think there should be an entry for -n in the OPTIONS section.

apjanke commented 2 months ago

Okay, over in https://github.com/cowsay-org/cowsay/commit/0ab13e900dfb7ca96d8fd7c1819a5cb4909673c4, I fixed the busted "undefined subroutine" behavior, so now when you use -n incorrectly, it gives you a more reasonable help screen output, instead of a "no such perl function" "undefined subroutine" internal error.

[cowsay] $ ./cowsay -n moo
cowsay version 3.9.0-SNAPSHOT

Usage:

    cowsay [-bdgpstwy] [-e <eyes>] [-f <cowfile> | -r [-C] ]
        [-n] [-T <tongue>] [-W <wrapcolumn>]
        <message>
    cowsay -l    # List defined cows
    cowsay -h    # Displays this help screen

[cowsay] $ echo $?
1
[cowsay] $ echo "blah blah" | ./cowsay -n
 ___________
< blah blah >
 -----------
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||
[cowsay] $

I still don't actually understand what -n is supposed to do, so I don't know if its behavior is correct in the case when you're calling it with a valid combination of command arguments.

apjanke commented 2 months ago

Released the basic fix in cowsay 3.8.1, so it gives a decent error message now.

I'm not sure what the right overall fix is here, but after reading the code some, I think that the -n option is just incompatible with messages passed on the command line (instead of via stdin), and combining the two should just produce an error message and exit nonzero. Objections?

limburgher commented 2 months ago

None. Thank you!