Support optional arg in -L (--color) short option

[chu11]

Problem: Several flux commands support a --color option. However, a short option (-L) is inconsistently supported. In addition to this inconsistency, the options to --color are optional and may not always work with the short option.

For consistency across flux tools, only support the long --color option.

Update tests that tested the -L short option.

Update manpages and remove refernce to -L short option.

Fixes #5828

[grondo]

Hm, I don't approve this. I use -L all the time to force color when passing output to less, etc, and I refuse to type --color=always 🙂 #5828 suggests updating docs and --usage, not removal of the option.

[chu11]

Understood, I went for this approach bc

A) we are already inconsistent, so some tools don't support -L. Do you think we should leave that be or add -L to those tools?

B) I noticed ls doesn't support -L.

Personally, I dislike the inconsistency more than the documentation issue.

[grondo]

Do you think we should leave that be or add -L to those tools?

We should leave it off IMO if it hasn't had a use case. The short option is useful for commands that might be piped to ls to preserve color in that case. For tools like flux top that doesn't seem necessary?

I noticed ls doesn't support -L.

Yes, but if we're looking for prior examples, dmesg(1) and hexdump(1) cfdisk(8) and fdisk(8) do 😜

My preference would be to not remove existing options people may be using. I'm not so sure the short option must be added to commands where it is currently missing, but if it does bother you then that would be fine with me.

Mainly I was thinking we just update docs to say -L is equivalent to --color=always. (Though I do notice that dmesg -Lalways works and dmesg -L always does not: I wonder if there is some way to fix our implementation (possibly dmesg is parsing options by hand though...)

[garlick]

Looks like dmesg uses getopt (if I have the right dmesg):
https://github.com/util-linux/util-linux/blob/master/sys-utils/dmesg.c#L1715

from getopt(3):

Two colons mean an option takes an optional arg; if there is text in
the current argv-element (i.e., in the same word as the option name it‐
self, for example, "-oarg"), then it is returned in optarg, otherwise
optarg is set to zero. This is a GNU extension.

[grondo]

Hm, in optparse we only do the two colons when a flag exists. Need to look at this again:

    if (o->has_arg == 1)
        colons = ":";
    else if (o->has_arg == 2 && o->flags & OPTPARSE_OPT_SHORTOPT_OPTIONAL_ARG)
        colons = "::";

[chu11]

Hm, in optparse we only do the two colons when a flag exists. Need to look at this again:

Yeah, I noticed last night but didn't look into it too much bc A) the approach I was going to take and B) dunno if we should rely on a GNU extension for optional short options?

Can look at later as well.

[grondo]

B) dunno if we should rely on a GNU extension for optional short options?

We have vendored gnu getopt_long so I'm not sure that's an issue.

I wonder if "fixing" this will break usage like flux dmesg -HLf though...

[chu11]

@grondo so someting like -Lnever works for you in flux-dmesg? I noticed this last night but wanted to double check this morning before mentioning it

>flux dmesg -L never
flux-dmesg: flux-dmesg accepts no free arguments

>flux dmesg -Lnever
flux dmesg: unrecognized option '-e'
Try `flux dmesg --help' for more information.

similar with flux kvs eventlog get -L never | -Lnever ....

[grondo]

No, I meant it works in the Linux dmesg(1) command, so it is possible to support it.

It works with flux dmesg if I add OPTPARSE_OPT_SHORTOPT_OPTIONAL_ARG to flags for that option. However, that breaks flux dmesg -HLf:

$ flux dmesg -HLf
flux-dmesg: Invalid argument to --color: 'f'

So I'm not 100% sure it is an improvement, though flux dmesg -HfL works, so maybe it is?

[grondo]

I had to remind myself why that flag exists:

commit 55e57d4c238d504a05d912f7da86ee188fdf3351
Author: Mark A. Grondona <mark.grondona@gmail.com>
Date:   Wed Jun 9 11:46:15 2021 -0700

    optparse: make optional arg apply to longopt only by default
    
    Problem: It is common practice to make a `-v, --verbose` option
    increase verbosity with multiple use, e.g. `-vvv` sets `verbose = 3`
    internally in the command or process to which the option is applied.
    If using the long option, however, it would be more convenient if an
    optional argument was used, so that `--verbose=3` is equivalent to
    `-vvv` (Nobody wants to write `--verbose --verbose --verbose). Sadly,
    this is not currently supported in an acceptable way by liboptparse. If
    you choose an optional argument, then `-v` cannot be combined with
    any other short option, since `-vv` passes `v` as the argument to
    the first `-v`.
    
    Change liboptparse so that optional argument (.has_arg = 2) applies
    only to the long option by default. The old behavior can be
    restored with the new flag: OPTPARSE_OPT_SHORTOPT_OPTIONAL_ARG.

[chu11]

So I'm not 100% sure it is an improvement, though flux dmesg -HfL works, so maybe it is?

This feels better IMO. There's a clear error message that makes sense with the flux dmesg -HLf case.

[grondo]

Yeah, this would be fine with me. Some existing use cases would fail, but as long as -L is the last short option in a group it can continue to be used as --color=always.

Note that users may still get confused that flux dmesg -L always doesn't work, but this is the case for all the tools previously mentioned so it clearly isn't a big deal. Mention can be made of this limitation in the man page(s).

[chu11]

re-pushed basically adding OPTPARSE_OPT_SHORTOPT_OPTIONAL_ARG in appropriate locations to get --color's -L working

Updated a few tests to add -Lalways.

I didn't add new color tests for all the tools that didn't already have color tests, as I felt that was overkill? But obviously could be added. i added some more tests, but I didn't go crazy with every possible permutation out there. Just added to the tests that were already there.

I did not make --color consistent between tools, figuring that could be a different PR if we decide to do it.

[codecov]

Codecov Report

Merging #5890 (33ad653) into master (21ce5c0) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5890      +/-   ##
==========================================
- Coverage   83.31%   83.30%   -0.01%     
==========================================
  Files         514      514              
  Lines       82809    82809              
==========================================
- Hits        68990    68987       -3     
- Misses      13819    13822       +3     

Files	Coverage Δ
src/cmd/builtin/dmesg.c	93.75% <ø> (ø)
src/cmd/builtin/overlay.c	91.32% <ø> (ø)
src/cmd/flux-kvs.c	84.01% <ø> (ø)
src/cmd/job/eventlog.c	88.63% <ø> (ø)

... and 15 files with indirect coverage changes

[grondo]

LGTM. Probably should reorder commits as suggested below, though.

[grondo]

This commit should probably go before 90f0d30 so the test isn't broken in between (and note in the commit message that -L will be fixed to take an optional arg so this usage will break)

[chu11]

oh yeah! i fixed that last as it was uncovered.

[chu11]

thanks re-ordered commits and tweaked that commit message. setting MWP