Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(alias): ls alias lx is an illegal option on darwin bsd style ls #1752

Conversation

casaper
Copy link
Contributor

@casaper casaper commented Oct 19, 2019

Fixes illegal option -X on MacOS

The option -X gets MacOS' ls (and probably also other BSD-ish OS) to complain about an illegal option:

user@host ~ $ ls -lhXB
ls: illegal option -- X
usage: ls [-ABCFGHLOPRSTUWabcdefghiklmnopqrstuwx1] [file ...]

Proposed Changes

  • only define the alias lx with option -X on machines that are not Darwin

I would also exclude other BSD like OS but since I do not have such a host at hand I cannot verify if that is even really needed for them.

@wadkar
Copy link
Contributor

wadkar commented Feb 12, 2020

👍
Makes sense. Also the recent PR #1786 will make it work with people who've done a brew install coreutils.

Edit: correct PR number.

@casaper
Copy link
Contributor Author

casaper commented Feb 13, 2020

@wadkar

Makes sense. Also the recent PR #1786 will make it work with people who've done a brew install coreutils.

Who could I request a review from then? I mean in order to get it merged?

Edit:

Well I just chose to bother two of the three reviewers of #1786 now....

@@ -118,7 +118,9 @@ alias ll='ls -lh' # Lists human readable sizes.
alias lr='ll -R' # Lists human readable sizes, recursively.
alias la='ll -A' # Lists human readable sizes, hidden files.
alias lm='la | "$PAGER"' # Lists human readable sizes, hidden files through pager.
alias lx='ll -XB' # Lists sorted by extension (GNU only).
if [[ "$OSTYPE" != darwin* ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to the top? Makes the code more maintainable in the long run. (this can be considered as a nitpick :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can do. But I just considered to keep changes at a minimum.

But regarding readability I also had something in mind, and that is why I did #1793 with helpers. Presuming that helper would exist, would that calm your nit'ish feeling a little? 😉

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a view on #1793, so I was waiting for the other contributors decide. Probably should add @belak to the reviewers list 😄 for #1793

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1793 is highly optional. I was just mentioning it, because I interpreted your comment as a readability one.

Will add belak then... thx for the hint 😄

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment is totality a readability one 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srijanshetty

I changed this code now. Now I check for the capability of ls instead of the OS running.

Just the fact that you're running on darwin doesn't prove your having a BSD style ls.
Since I'm aware of its existence I myself have the gnu coreutils installed here, so for myself this fix is not needed any more. 😉

@srijanshetty
Copy link
Collaborator

Added a comment

@casaper casaper closed this Feb 28, 2020
@belak belak reopened this Mar 19, 2020
@belak
Copy link
Collaborator

belak commented Mar 19, 2020

I'm not sure the best way to do this, but this alias should be enabled only if ls is coreutils based. I believe this will also be broken on BSD.

It's good to know though. I'd be happy to merge this with a slightly better check.

@casaper
Copy link
Contributor Author

casaper commented Mar 21, 2020

@belak

It's good to know though. I'd be happy to merge this with a slightly better check.

What would you think if I checked for ls running with code 0 with -XB instead, and only then add the alias?

This way it would really only depend on ls's capabilities, and not on the OS at all.

if ls -XB &> /dev/null; then
  alias  lx='ll -XB'        # Lists sorted by extension (GNU only).
fi

Haven't tried if that would work well everywhere. The idea just came up when you asked for a better check...

@casaper casaper force-pushed the fix/do_not_define_maos_incompatible_alias_lx branch 2 times, most recently from 018a6df to cb76c48 Compare March 21, 2020 16:57
@casaper casaper changed the title fix(alias): ls alias lx is an illegal option on MacOS/Darwin fix(alias): ls alias lx is an illegal option on darwin bsd style ls Mar 21, 2020
The option `-X` lets MacOS (and probably also other BSD-ish OS) ls
complain about an illegal option:

```
ls -lhXB
ls: illegal option -- X
usage: ls [-ABCFGHLOPRSTUWabcdefghiklmnopqrstuwx1] [file ...]
```
@casaper casaper force-pushed the fix/do_not_define_maos_incompatible_alias_lx branch from cb76c48 to cd12f76 Compare March 21, 2020 17:00
Copy link
Collaborator

@indrajitr indrajitr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, other than the way to test for gnu coreutils.

alias lk='ll -Sr' # Lists sorted by size, largest last.
alias lt='ll -tr' # Lists sorted by date, most recent last.
alias lc='lt -c' # Lists sorted by date, most recent last, shows change time.
alias lu='lt -u' # Lists sorted by date, most recent last, shows access time.
alias sl='ls' # I often screw this up.

# Only make alias if ls supports -XB (darwins BSD style ls doesn't)
if ls -XB &> /dev/null; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if you would consider if is-callable 'dircolors' instead.
ls -XB might be expensive depending on the directory on which it is executed.

These things keep on adding to the startup time (unless we memoize it on first call).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could use ls /dev/null since that should only be a single file and should be fast.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ls -XB /dev/null would be a great idea! That said, would be nice to be consistently using one way to detect BSD/GNU versions. Turns out we use is-callable 'dircolors' to detect this already.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just detect the options via --help like so:

if grep -q '\-X' <(ls --help 2>&1); then
  # ...
fi

We do similar thing in rsync module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indrajitr I wasn't aware of the performance penalty my idea could introduce, and your concern makes quite a lot of sense.

On the part of finding a better solution:

At first glance the most straight forward solution @belak has suggested ls -XB /dev/null seems feasible, but in the end the callable 'dircolors' appears to be the most efficient and elegant alternative.

But regarding the test with dircolors, I have one question: Is there an unlikeley case I might not see, where dircolors is not callable and gnu-ls is present or the other way around?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants