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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

martinvonz (Martin von Zweigbergk)
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6789

AFFECTED FILES
  contrib/check-code.py

CHANGE DETAILS

diff --git a/contrib/check-code.py b/contrib/check-code.py
--- a/contrib/check-code.py
+++ b/contrib/check-code.py
@@ -116,7 +116,6 @@
     (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.*[^\\]\\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'\[[^\]]+==', '[ foo == bar ] is a bashism, use [ foo = bar ] instead'),
     (r'(^|\|\s*)grep (-\w\s+)*[^|]*[(|]\w',



To: martinvonz, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

martinvonz (Martin von Zweigbergk)
Closed by commit rHG4257c33e24b7: check-code: allow command substitution with $(command) (authored by martinvonz).
This revision was automatically updated to reflect the committed changes.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D6789?vs=16394&id=16433

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6789/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6789

AFFECTED FILES
  contrib/check-code.py

CHANGE DETAILS

diff --git a/contrib/check-code.py b/contrib/check-code.py
--- a/contrib/check-code.py
+++ b/contrib/check-code.py
@@ -116,7 +116,6 @@
     (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.*[^\\]\\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'\[[^\]]+==', '[ foo == bar ] is a bashism, use [ foo = bar ] instead'),
     (r'(^|\|\s*)grep (-\w\s+)*[^|]*[(|]\w',



To: martinvonz, #hg-reviewers, indygreg
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: D6789: check-code: allow command substitution with $(command)

Augie Fackler-2
In reply to this post by martinvonz (Martin von Zweigbergk)
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.

> On Sep 7, 2019, at 02:32, martinvonz (Martin von Zweigbergk) <[hidden email]> wrote:
>
> martinvonz created this revision.
> Herald added a subscriber: mercurial-devel.
> Herald added a reviewer: hg-reviewers.
>
> REVISION 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).
>
> REPOSITORY
>  rHG Mercurial
>
> REVISION DETAIL
>  https://phab.mercurial-scm.org/D6789
>
> AFFECTED FILES
>  contrib/check-code.py
>
> CHANGE DETAILS
>
> diff --git a/contrib/check-code.py b/contrib/check-code.py
> --- a/contrib/check-code.py
> +++ b/contrib/check-code.py
> @@ -116,7 +116,6 @@
>     (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.*[^\\]\\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'\[[^\]]+==', '[ foo == bar ] is a bashism, use [ foo = bar ] instead'),
>     (r'(^|\|\s*)grep (-\w\s+)*[^|]*[(|]\w',
>
>
>
> To: martinvonz, #hg-reviewers
> Cc: mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

martinvonz (Martin von Zweigbergk)
In reply to this post by martinvonz (Martin von Zweigbergk)
martinvonz added a comment.


  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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6789/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6789

To: martinvonz, #hg-reviewers, indygreg
Cc: durin42, indygreg, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

martinvonz (Martin von Zweigbergk)
In reply to this post by martinvonz (Martin von Zweigbergk)
martinvonz added a comment.


  In D6789#100218 <https://phab.mercurial-scm.org/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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6789/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6789

To: martinvonz, #hg-reviewers, indygreg
Cc: durin42, indygreg, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

martinvonz (Martin von Zweigbergk)
In reply to this post by martinvonz (Martin von Zweigbergk)
lothiraldan added a comment.


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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6789/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6789

To: martinvonz, #hg-reviewers, indygreg
Cc: lothiraldan, durin42, indygreg, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

martinvonz (Martin von Zweigbergk)
In reply to this post by martinvonz (Martin von Zweigbergk)
spectral added a comment.


  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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6789/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6789

To: martinvonz, #hg-reviewers, indygreg
Cc: spectral, lothiraldan, durin42, indygreg, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel