fix: Improve Bash hygiene#1056
Conversation
| local __curr_tag="${tags_list_keys[$i]}" | ||
| local __prev_tag="${tags_list_keys[$i+1]:-null}" | ||
| local __curr_date="$(_valueForKeyFakeAssocArray "${__curr_tag}" "${tags_list[*]}")" | ||
| local __curr_date= |
There was a problem hiding this comment.
| local __curr_date= | |
| local __curr_date |
There was a problem hiding this comment.
I suppose this is a stylistic preference (the small behavior difference isn't relevant to every Bash project), so I went ahead and removed the extra =.
| if [ "$OUTPUT_STYLE" == "tabular" ]; then | ||
| tabular_headers="# Repo $SP Age $SP Last active $SP Active on $SP Commits $SP Uncommitted $SP Branch" | ||
| echo -e "$tabular_headers\n$project $SP $(repository_age) $SP $(last_active) $SP $(active_days $commit) days $SP $(commit_count $commit) $SP $(uncommitted_changes_count) $SP $(current_branch_name)" | column -t -s "$COLUMN_CMD_DELIMTER" | ||
| echo -e "$tabular_headers\n$project $SP $(repository_age) $SP $(last_active) $SP $(active_days "$commit") days $SP $(commit_count "$commit") $SP $(uncommitted_changes_count) $SP $(current_branch_name)" | column -t -s "$COLUMN_CMD_DELIMTER" |
There was a problem hiding this comment.
The quote is missing intently. We have shellcheck disable=SC2086 mark in active_days and other similar functions.
There was a problem hiding this comment.
To me it doesn't look like the shellcheck disable=SC2086 should even be in active_days. And actually, looking at all the ShellCheck disable directives, the ones that deal with $commit are incorrect. So to me it seems we are dealing with a larger issue that has been surpressed.
From the top, git-summary has a synposis of git-summary [--line] [--dedup-by-email] [<committish>], and accepts a single committish (even if it accepted multiple committishes, it wouldn't make sense anyways since the underlying git subcommands acccept only a single commitish). The $commit variable is either assigned HEAD or $* (which corresponds to <committish>. The $* looks fishy itself, but doesn't need to be changed.
Concerning the active_days function, the only argument passed to it is $commit, which is eventually passed to git log. In other functions, $commit is passed to git shortlog. All these git subcommands have the commonality of accepting a single [<revision range>].
[<revision range>] is defined in gitrevisions(7), and some valid revisions include things that contain spaces, like HEAD^{/fix nasty bug}, HEAD@{5 minutes ago}. So by not quoting $commit, it word splits on committishes that include spaces.
There was a problem hiding this comment.
However, there are still cases in which $commit may be empty and word splitting is intended - so those of course will have to be handled as well.
Edit: I have fixed that case by moving commit=HEAD up.
| date() { | ||
| commit_date() { | ||
| # the $1 can be empty | ||
| # shellcheck disable=SC2086 |
There was a problem hiding this comment.
We can remove the shellcheck now?
There was a problem hiding this comment.
I removed one extra one, but the rest cannot be removed because of MERGES_ARG.
There was a problem hiding this comment.
It seems we can quote MERGES_ARG as we don't expect any whitespace in it.
What do you think?
There was a problem hiding this comment.
Unfortunately not because that would mean an empty argument will be passed to git. It's funtionally similar to doing this:
$ git log '' --oneline HEAD
fatal: ambiguous argument '': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'There was a problem hiding this comment.
In other words it shouldn't be quoted so the whitespace can be skipped by the shell during word splitting.
There was a problem hiding this comment.
I see. Thanks for clarifying that.
2 participants
Closes #1055