rmyorston / busybox-w32

WIN32 native port of BusyBox.
https://frippery.org/busybox
Other
670 stars 124 forks source link

Support NO_COLOR environment variable (it seems now only needed in ls.c) #382

Closed ghost closed 6 months ago

ghost commented 6 months ago

Rationale:

https://no-color.org/

To support it, I think one new line has to be added (between 2 and 3) to the ls.c and one line and one comment have to be changed (3 and 4):

Before:

if (ENABLE_FEATURE_LS_COLOR_IS_DEFAULT && !is_TERM_dumb()) {
    char *p = getenv("LS_COLORS");
    /* LS_COLORS is unset, or (not empty && not "none") ? */
    if (!p || (p[0] && strcmp(p, "none") != 0)) {

After:

    if (ENABLE_FEATURE_LS_COLOR_IS_DEFAULT && !is_TERM_dumb()) {
        char *p = getenv("LS_COLORS");
        char *no_c = getenv("NO_COLOR"); /* if set: no color. See: no-color.org */
        /* if NO_COLOR and LS_COLORS are unset, or later (not empty && not "none") ? */
        if ((!no_c && !p) || (p[0] && strcmp(p, "none") != 0)) {

I haven't tested it, just believe that it could be enough.

The idea is that the user can still set LS_COLORS to something not none to override user's "more global" NO_COLOR, as per in https://no-color.org/

ghost commented 6 months ago

(Note: I've made a typo initially which I've edited afterwards: the correct name for the variable is NO_COLOR (8 chars and singular not plural), per https://no-color.org/

avih commented 6 months ago

I don't see how this is relevant.

Which busybox-w32 applets output color by default, where this NO_COLOR env would make a difference?

EDIT: oh, in ls.

rmyorston commented 6 months ago

This isn't a Windows-specific issue. Really it should be addressed to upstream BusyBox.

Still, it's not a big deal so I've gone ahead and added NO_COLOR support. Try one of the most recent prerelease binaries.

ghost commented 6 months ago

Really it should be addressed to upstream BusyBox.

I'm sorry :( I use your BusyBox, I haven't used the upstream for years, and I wasn't aware that it comes from there.

Sometimes an implemented solution gets pushed upstream, maybe this can help. I see I've missed " when present and not an empty string" I'm sorry about that.

The existing LS_COLORS=none already worked for my use case of using your busybox, I just hoped to motivate that more global approach of no-color.org where I saw it. Maybe it's better that you revert that change, as it shouldn't be mingw specific anyway? If you do want to solve it completely, it seems there would be more work to do to achieve full no-color.org logic, specifically from Faq answer 2, where, if the program insists to have color by default, like it was in busybox, then NO_COLOR allows turning that default off for all such applications, but also, per Faq 2, the user can still turn on the colors "per app". Your current solution doesn't cover that scenario. That gets messy if it has to be solved with # if directives, so maybe better reverting?

rmyorston commented 6 months ago

I often submit my changes to upstream BusyBox. I haven't done that for this one just yet. It isn't that intrusive so I'm happy to carry it in busybox-w32 for now. Upstream haven't been very responsive lately and there are more important issues for them to deal with than this.

I'm not sure I understand your concern about FAQ 2. The ls --color option allows the user to turn on colour:

export NO_COLOR=1
ls --color

The output is coloured in this case.

ghost commented 6 months ago

Why I think "it gets messy": the initial report was based on false expectation that a few unobtrusive changes were possible. But then you recognized that "if not empty" condition. (Windows doesn't allow creating empty env vars AFAIK, but busybox does). (I've also missed the necessity for an additional check of p before p[0] in the second part of the big condition)

Finally, as I saw your code including these "#if ... mingw", I don't know if these are there to make upsteaming better, but it all makes harder reducing the number of changes to a minimal line count for a complete and clean solution, where complete would be that LS_COLORS still overrides "more global" NO_COLOR, to still allow the user to configure his per-app preferences, even for the colors and even if no color is default for all (I hope I haven't made mistakes in the truth tables, same-spaced font needed):

 present:
 - "color by default"
 - LS_COLORS can disable
 - command line switch overrides
   (GREEN represents colored output)

            LS_COLORS  --color    output
            -------------------------------
               ''                 GREEN
               ''      --color    GREEN
               'none'             .
               'none'  --color    GREEN
               'T'                GREEN
               'T'     --color    GREEN

For apps with "color by default" the "NO_COLOR" should mean
- "user's preference is no color for all the apps 
    which like color by default"
- but the per-app settings still allow configuration of 
    color on per app basis (that would include respecting LS_COLORS)

   NO_COLOR   LS_COLORS  --color  output
   -------------------------------------

    ''           ''               GREEN
    ''           ''      --color  GREEN
    ''           'none'           .
    ''           'none'  --color  GREEN
    ''           'T'              GREEN
    ''           'T'     --color  GREEN

    'T'          ''               .
    'T'          ''      --color  GREEN
    'T'          'none'           .
    'T'          'none'  --color  GREEN
    'T'          'T'              GREEN
    'T'          'T'     --color  GREEN

Your pre release:

   NO_COLOR   LS_COLORS  --color  output
   -------------------------------------

    ''           ''               GREEN
    ''           ''      --color  GREEN
    ''           'none'           .
    ''           'none'  --color  GREEN
    ''           'T'              GREEN
    ''           'T'     --color  GREEN

    'T'          ''               .
    'T'          ''      --color  GREEN
    'T'          'none'           .
    'T'          'none'  --color  GREEN
    'T'          'T'              .
    'T'          'T'     --color  GREEN

bb1

And, as far as I understand, all this shouldn't be mingw specific.

ghost commented 6 months ago

(deleted previous, I'm not sure I have thought about all the "unset" aspects, the truth tables I've made don't include null pointers -- "unset" cases)

ghost commented 6 months ago

Changing the basic assumption of "colors by default" for unset ls_colors would be much cleaner, I think, and would make NO_COLOR not needed at all, but somebody introduced that and now the complexity falls on somebody else.

rmyorston commented 6 months ago

The situation with colour in ls is complicated.

Neither coreutils ls nor FreeBSD ls enable colour by default. In each case it's necessary to use one of the --color options to enable colour. Therefore NO_COLOR is unnecessary.

BusyBox ls has a build-time configuration option to enable colour by default. This option is enabled in default builds of upstream and busybox-w32, despite the description of the option saying its use is not recommended. Changing this now is unlikely to be welcome.

When colour is enabled by default it's possible to turn it off either with the --color=none option or by a particular setting of the LS_COLORS environment variable. The latter is unique to BusyBox. Setting LS_COLORS to an empty string or none disables colour.

The use of none was a bad idea. It isn't a valid string for coreutils ls: it does disable colour there but it also issues a warning that the string can't be parsed. An empty value, on the other hand, disables colour in coreutils ls without a warning.

Although particular settings of LS_COLORS can be used to disable colour in BusyBox ls, no setting of it will enable colour. This is also true of coreutils ls. That's not what LS_COLORS is for.

I think my recent change is the correct way to extend BusyBox to support NO_COLOR while retaining support for the existing (somewhat flawed) mechanism.

ghost commented 6 months ago

The use of none was a bad idea

As far as I understand, empty environment variable can neither be entered in windows dialog nor in bat files, so I understand, at least, why it was invented. I understood that it's not compatible outside of BusyBox. I didn't know that outside of BusyBox the command line switch is actually required to turn that on, thanks. Then what's there can really be enough.

Also many thanks for all your work on busybox-w32 . I really like to use it, it's marvelous that it's a single binary with all the functionality and careful adjustments needed for convenient work under Windows.