This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6789

check-code: allow command substitution with $(command)
ClosedPublic

Authored by martinvonz on Sep 7 2019, 2:32 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG4257c33e24b7: check-code: allow command substitution with $(command)
Summary

Both command and $(command) are specified by POSIX. The latter nests
better. I don't see why we shouldn't allow both (or only the latter).

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Sep 7 2019, 2:32 AM
indygreg accepted this revision.Sep 7 2019, 12:51 PM
This revision is now accepted and ready to land.Sep 7 2019, 12:51 PM
durin42 added subscribers: indygreg, durin42.Sep 9 2019, 10:52 AM

I went through the history of this line and sadly it came into existence with the initial check-code.py commit. However, I got to use hg grep for the purpose nobody uses (!) and found this:

augie% hg grep -r '0::e7d3b509af8b7cb31ec1b419dd7cc05ea5a6c7c1' '\$\(' tests
tests/test-http-proxy:2337:kill $(cat proxy.pid a/hg.pid)
tests/test-bisect:2924: count=$(( $count + 1 ))
tests/README:2968:- don't use math expressions like let, (( ... )), or $(( ... )); use "expr"
tests/test-diff-change:7628:#rev=$(hg log -r 1 --template '{node|short}')
tests/test-gendoc:9444:RST2HTML=$(which rst2html 2> /dev/null || which rst2html.py)
tests/test-diffstat:9640:i=0; while (( $i < 213 )); do echo a >> a; i=$(($i + 1)); done
tests/test-subrepo-svn:10180:escapedwd=$(pwd | \
tests/test-subrepo-svn:10180:WCROOT="$(pwd)/svn-wc"

looking at the history for test-http-proxy, I found this:

https://www.mercurial-scm.org/repo/hg/rev/a20877c8a3e2:

Use more compatible cmd instead of $(cmd) in test-http-proxy

IOW, my foggy memory that some sh implementations don't understand $() seems justified. That doesn't mean we shouldn't make this change, but we should at a minimum call out in the relnotes that $() is now a required feature of your shell if you want to run the tests.

There's also https://www.mercurial-scm.org/repo/hg/rev/d77022db1bca but I suspect $(()) is a bashism. The only other explicit cleanup of $() I can find is https://www.mercurial-scm.org/repo/hg/rev/7e3a685be2f3, which calls it a bashism but that's seemingly not quite true.

I've left the change not-accepted for now, but I don't object if someone wants to accept it. If you do, please send a follow-up release note for packagers.

martinvonz added a comment.Sep 9 2019, 2:02 PM

The only information about it that I could find is that Bourne Shell doesn't support $() syntax. I'll still send a patch to mention it in the release notes.

martinvonz added a comment.Sep 9 2019, 2:07 PM
In D6789#100218, @martinvonz wrote:

The only information about it that I could find is that Bourne Shell doesn't support $() syntax. I'll still send a patch to mention it in the release notes.

Oh, the most likely reason one would be using Bourne Shell is probably if one is running Solaris before version 11 (2011): https://docs.oracle.com/cd/E23824_01/html/E24456/userenv-1.html

lothiraldan added a subscriber: lothiraldan.Sep 9 2019, 2:15 PM

FYI, Fish also doesn't support the $(expr) syntax but it's not a POSIX compatible shell

spectral added a subscriber: spectral.Sep 9 2019, 3:19 PM

This shouldn't be about the user's chosen shell, though, right? This is about the /bin/sh implementation, which you wouldn't/can't replace with fish, so I think it won't bother users that use fish, just distros with really old implementations of /bin/sh.

Revision Contents
Changeset List

PathPackages
Mcontrib/check-code.py (1 line)

Diff 16433

contrib/check-code.py

(r'echo -n', "don't use 'echo -n', use printf"), (r'echo -n', "don't use 'echo -n', use printf"),
(r'(^|\|\s*)\bwc\b[^|]*$\n(?!.*\(re\))', "filter wc output"), (r'(^|\|\s*)\bwc\b[^|]*$\n(?!.*\(re\))', "filter wc output"),
(r'head -c', "don't use 'head -c', use 'dd'"), (r'head -c', "don't use 'head -c', use 'dd'"),
(r'tail -n', "don't use the '-n' option to tail, just use '-<num>'"), (r'tail -n', "don't use the '-n' option to tail, just use '-<num>'"),
(r'sha1sum', "don't use sha1sum, use $TESTDIR/md5sum.py"), (r'sha1sum', "don't use sha1sum, use $TESTDIR/md5sum.py"),
(r'\bls\b.*-\w*R', "don't use 'ls -R', use 'find'"), (r'\bls\b.*-\w*R', "don't use 'ls -R', use 'find'"),
(r'printf.*[^\\]\\([1-9]|0\d)', r"don't use 'printf \NNN', use Python"), (r'printf.*[^\\]\\([1-9]|0\d)', r"don't use 'printf \NNN', use Python"),
(r'printf.*[^\\]\\x', "don't use printf \\x, use Python"), (r'printf.*[^\\]\\x', "don't use printf \\x, use Python"),
(r'\$\(.*\)', "don't use $(expr), use `expr`"),
(r'rm -rf \*', "don't use naked rm -rf, target a directory"), (r'rm -rf \*', "don't use naked rm -rf, target a directory"),
(r'\[[^\]]+==', '[ foo == bar ] is a bashism, use [ foo = bar ] instead'), (r'\[[^\]]+==', '[ foo == bar ] is a bashism, use [ foo = bar ] instead'),
(r'(^|\|\s*)grep (-\w\s+)*[^|]*[(|]\w', (r'(^|\|\s*)grep (-\w\s+)*[^|]*[(|]\w',
"use egrep for extended grep syntax"), "use egrep for extended grep syntax"),
(r'(^|\|\s*)e?grep .*\\S', "don't use \\S in regular expression"), (r'(^|\|\s*)e?grep .*\\S', "don't use \\S in regular expression"),
(r'(?<!!)/bin/', "don't use explicit paths for tools"), (r'(?<!!)/bin/', "don't use explicit paths for tools"),
(r'#!.*/bash', "don't use bash in shebang, use sh"), (r'#!.*/bash', "don't use bash in shebang, use sh"),
(r'[^\n]\Z', "no trailing newline"), (r'[^\n]\Z', "no trailing newline"),