Add hooks to check .sh and .bash headers - Fixes #1491

[davidpfarrell]

Description

Adds local pre-commit hooks to check .sh and .bash files for proper headers and executable flags.

Hooks Folder

Introduces a hooks/ folder on root to act as a home for custom pre-commit hooks.

Local Repo

Adds a local repo to ".pre-commit-config.yaml"

dot-sh Hook

Adds the dot-sh hook to check .sh files against bash-it requirements:

• Proper #! header
• File is executable

file.sh header:

#!/usr/bin/env bash

dot-bash Hook

Adds the dot-bash hook to check .bash files against bash-it requirements:

• Proper #! header
• Proper shellcheck header
• File is NOT executable

file.bash header:

#!/usr/bin/env echo run from bash: .
# shellcheck shell=bash disable=SC2148,SC2096

Fix Existing Files

Once the hooks were activated, some existing .bash files from "clean_files.txt" triggered errors.
This PR also includes fixes to those few files.

Motivation and Context

Intended to help enforce consistent header (#!) usage across bash-it executable and include scripts.

Fixes #1491

How Has This Been Tested?

Extensive testing locally by manipulating .sh and .bash files to confirm expected behavior/reporting from the hooks.

Screenshots (if appropriate):

Check .sh files against bash-it requirements................................Failed
- hook id: dot-sh
- exit code: 1

Bash file `lint_clean_files.sh` is not executable

Check .bash files against bash-it requirements..............................Failed
- hook id: dot-bash
- exit code: 1

Bash include file `completion/available/cargo.completion.bash` should not be executable
Bash include file `completion/available/cargo.completion.bash` has bad/missing #! header
Bash include file `completion/available/cargo.completion.bash` has bad/missing shellcheck header

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

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.

TODO

• Let hooks try to fix issues? i.e Executable flag?
• Possibly update the documentation on the expected headers for .bash files?
• Tests?

[davidpfarrell]

Hi @cornfeedhobo ,

Your question disappeared while i was replying to it:

---

cornfeedhobo wrote:

#!/usr/bin/env echo run from bash: .
# shellcheck shell=bash disable=SC2148,SC2096

Can you explain this directive and why you chose it?

---

Answer

re: echo run from bash: . :

@ravenhall's comment on #1491 suggested adding a message to replace the #! since no #! is technically needed in .bash files:

ravenhall's suggestion

# loaded by bash-it via source

In doing a bit of research into how others were dealing with this, I came across the echo statement. Its a bit clever in that, if you do invoke the script via bash, you get an error message like the following:

$ /tmp/test.bash

run from bash: . /tmp/test.bash

However, I will admit that this might be overly clever, in that the hooks make sure the file is not executable, so the only way to see the message would be something like bash -c /tmp/test.bash - Also this echo statement introduces the need to disable SC2096 (see below).

re: shellcheck:

The main body of the shellcheck, shellcheck shell=bash disable=SC2148, came from your comment on #1491.

The need for SC2096 is introduced by the echo statement:

ShellCheck SC2096:

On most OS, shebangs can only specify a single parameter.

Soo, we could simply the header by reducing the #! to a single argument (i.e /usr/bin/env false) or just a simple comment like the original suggestion.

[cornfeedhobo]

Your question disappeared while i was replying to it:

@davidpfarrell My bad. koala_man ended up explaining it to me, so I deleted the comment. Thanks for checking in.

Edit:

Interesting quotes from irc about this:

<greycat> whether the whole message actually prints will depend on how a particular kernel handles that highly nonstandard shebang
<koala_man> it's arguably better this way because you'll see the message as part of the error, and you'll get a non-zero exit code lol

I have no knowledge of the corner/edge cases this could run into, but I'm marking it as approved since it passes CI and I really consider the error message just a best effort.

[nwinkler]

Thanks for the thorough explanation, @davidpfarrell - looks good to me!

One comment on the echo message: Would it be possible to use a more descriptive text there. The run from bash text would confuse me if I encountered it. Maybe something like You're not supposed to run this file directly - if you really want to run this, use ... - I know that's a lot longer, but also more descriptive...

[NoahGorny]

Hey @davidpfarrell , thats a great idea!
I left a couple of comments, and I also agree with @nwinkler that the message can be improved.

Thanks for the effort you are putting in btw 😄

[davidpfarrell]

Hey Team,

Would it be possible to use a more descriptive text there

The existing text was a bit of a place-holder that i copy/pasted from the interwebs.

It should probably be more bash-it specific, but I wouldn't want to get too verbose with it.

I'm thinking something like:

Cannot execute bash-it include: /path/to/script.bash

Maybe remove the #!

The more I think about it though, the more it might make since to simplify the whole header for .bash files to just:

# shellcheck shell=bash

Reasoning being:

• It looks like shell=bash is the remedy for SC2148, so we likely don't need them both. I tested locally and it appeared there was no different with/without the SC2148 directive
• The hook ensures that the .bash script is not executable, meaning that it seems very unlikely that the script could be executed directly. So the echo statement may be overkill.

Here are some tests with no #! header and chmod -x:

# Execute directly
$ /tmp/test.bash

-bash: /tmp/test.bash: Permission denied

# Source via .
$ . /tmp/test.bash
.bash script executed!

# Source via source
$ source /tmp/test.bash 

.bash script executed!

# Execute via bash -c
$  bash -c /tmp/test.bash

bash: line 1: /tmp/test.bash: Permission denied

# Execute via bash
$ bash  /tmp/test.bash

.bash script executed!

The only one that looks concerning here is bash /tmp/test.bash, BUT That method acts like a source and skips the #! so having an echo wouldn't help here anyway.

So my vote is to simplify the .bash header to just the shell=bash directive. I'll wait to hear from others before making any changes just in case the consensus is a different route.

[davidpfarrell]

Hi Team,

I just committed an update:

• Simplified the .bash check to just # shellcheck shell=bash, removing the #! and disable statements.
• Updated the 'fixed' .bash files to use the new header.
• Removed the TODO in the hooks for filename checks - The pre-commit config can take care of that.

[NoahGorny]

LGTM

[davidpfarrell]

LGTM

GREAT! I had to do a quick update for the just-added sdkman .bash file -- That makes 3 today - Let's merge this before any more get in :)