fix: Improve Bash hygiene #1056
Conversation
bin/git-changelog
Outdated
| @@ -280,7 +280,8 @@ commitList() { | |||
| for (( i=0; i<"${_tags_list_keys_length}"; i++ )); do | |||
| 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 =.
| @@ -216,9 +216,9 @@ print_summary_by_line() { | |||
| print_summary() { | |||
| 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.
| [ $# -ne 0 ] && commit=$* | ||
| fi | ||
| project=${PWD##*/} | ||
|
|
||
| # | ||
| # get date for the given <commit> | ||
| # | ||
| 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.
* fix: Improve Bash hygiene * Update quoting to account for new bugs * quotes * fix: Remove extra shellcheck comment
Closes #1055