plugin/percol shellcheck

[gaelicWizard]

Description

Adopt _command_exists, use _log_warning, some cleanup, shellcheck, and finally shfmt.

Motivation and Context

This was part of my _command_exists branch (#1938) but was out of scope.

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code follows the code style of this project.
• If my change requires a change to the documentation, I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
• I have added tests to cover my changes, and all the new and existing tests pass.

[davidpfarrell]

[gaelicWizard]

Rebased on master, no other changes to PR

[davidpfarrell]

I'm pretty happy with this so I'm approving it BUT ...

There could have been some acknowledgement that this PR deprecates using tac when present in favor of always using tail -r ...

[gaelicWizard]

I'm pretty happy with this so I'm approving it BUT ...

There could have been some acknowledgement that this PR deprecates using tac when present in favor of always using tail -r ...

I did. It's in the commit notes: according to shellcheck, the _tac alias simply never worked. Please don't claim that I did not specifically call that out.

[davidpfarrell]

I did. It's in the commit notes

Hey @gaelicWizard, apologies for not noticing your related commit message.

TIL that clicking the ... on the commit note in the PR comments will expand them (I will be doing this going forward)

The alias approach was definitely borked !

BUT

There are other ways to accomplish the tac || tail -r invocation.

I still feel that choosing to deprecate tac was worth calling out in the PR description.

SIDE NOTE:

I also wish the original PR for this had a comment in the code explaining the use.

I was not able to confirm either of :

• tac is noticeably more performant than tail -r
• tail -r is not fully portable

Without one of those being true, I'm not sure why one would add that logic present in the first place ...

[NoahGorny]

I think this is mergable right now, the tac thing can be opened as a separate issue

[gaelicWizard]

@davidpfarrell, I had actually never heard of tac before and had to Google it. (cat, backwards...😑) Doesn't exist on Mac anyway.

Since literally zero people use this plugin, as it's been broken since original commit (except for someone who has _tac previously defined in their environment, or has some kind of double-invocation bug where the plugin gets loaded, the function gets run (thereby defining the alias, but failing to run successfully), then the plugin gets loaded again (this time with the alias already defined)), I didn't think any further about the impact of using cat or tail -r.

That said, I'm trying out percol now so maybe I'll have some more thoughts in the future or at least be able to offer some intuition about how this runs.

EDIT: found a very-related discussion about tac vs tail -r for history sorting in Fish.

[gaelicWizard]

Final thought for now:

tac and tail -r seem indistinguishable performance wise on my 16k history:

$ TAC='tail -r'
$ time { history | ${TAC} | sed -e 's/^\ *[0-9]*\ *//' ; } | wc -l
   16179

real    0m4.382s
user    0m1.839s
sys     0m2.616s
$ TAC='tac'
$ time { history | ${TAC} | sed -e 's/^\ *[0-9]*\ *//' ; } | wc -l
   16181

real    0m4.306s
user    0m1.756s
sys     0m2.621s