| Message ID | 20170901225313.3020-2-eschwartz@archlinux.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [devtools,1/2] makechrootpkg: Fix unconditionally running namcap | expand |
On Fri, 01 Sep 2017 18:53:13 -0400, Eli Schwartz wrote: > > Don't use error-prone logic e.g. > foo=true; if $foo ... Hey, I'm not to blame for this one :-) > This invokes an extra process as opposed to a simple value comparison, That's not quite true: $ type true true is a shell builtin $ type false false is a shell builtin Bash won't actually make any syscalls when executing these commands in a conditional. Apparently the code paths within Bash are slightly faster for `[[ $v = true ]]` than for `$v`, but that's negligible compared to if it actually had to invoke an extra process as you said. Time for 1 million iterations: "v=true; if $v; then ..." | 7.1 sec "v=true; if [[ $v = true ]]; then ..." | 6.5 sec "v=/bin/true; if $v; then ..." | > 5 minutes (I got bored and killed it) > in the process of completely failing to act as expected when the > variable is unset because of unrelated bugs. And that's the real reason that this change should be applied. :-)
On 09/01/2017 11:56 PM, Luke Shumaker wrote: >> This invokes an extra process as opposed to a simple value comparison, > > That's not quite true: > > $ type true > true is a shell builtin > $ type false > false is a shell builtin Ah, true enough. :o I maybe should get out of the habit of submitting brand-new patches on the spur of the moment right before going offline for the weekend. Still, implementation-defined behavior... bash includes several things that shouldn't be builtins, though this isn't one of them. e.g. the horrible `kill` implementation which should always be disabled. coreutils/builtins clashes are so annoying. :(
diff --git a/makechrootpkg.in b/makechrootpkg.in index b1c9545..90bd9cf 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -188,7 +188,7 @@ prepare_chroot() { local keepbuilddir=$3 local run_namcap=$4 - $keepbuilddir || rm -rf "$copydir/build" + [[ $keepbuilddir = true ]] || rm -rf "$copydir/build" local builduser_uid builduser_gid builduser_uid="${SUDO_UID:-$UID}" @@ -225,7 +225,7 @@ EOF declare -f _chrootbuild printf '_chrootbuild "$@" || exit\n' - if $run_namcap; then + if [[ $run_namcap = true ]]; then declare -f _chrootnamcap printf '_chrootnamcap || exit\n' fi
Don't use error-prone logic e.g. foo=true; if $foo ... This invokes an extra process as opposed to a simple value comparison, in the process of completely failing to act as expected when the variable is unset because of unrelated bugs. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- makechrootpkg.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)