On Thu, Sep 05, 2013 at 10:27:43AM -0400, Aaron Lindsay wrote:

For instance, `rankmirrors nonexistent_file' should display: 'nonexistent_file' does not exist. rather than: $1 does not exist.

Signed-off-by: Aaron Lindsay ---

This looks more like a change in quoting than it does for error messages... I've no problem with this as m4 enjoys chomping on backticks and mangling the strings, but, see below.

contrib/bacman.sh.in | 2 +- contrib/rankmirrors.sh.in | 10 +++++----- contrib/updpkgsums.sh.in | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/contrib/bacman.sh.in b/contrib/bacman.sh.in index 5435e40..33e3920 100644 --- a/contrib/bacman.sh.in +++ b/contrib/bacman.sh.in @@ -124,7 +124,7 @@ fi

if [[ ! -d $pkg_dir ]]; then error "package %s is found in pacman database," "${pkg_name}" - plain " but \`%s' is not a directory" "${pkg_dir}" + plain " but '%s' is not a directory" "${pkg_dir}" exit 1 fi

diff --git a/contrib/rankmirrors.sh.in b/contrib/rankmirrors.sh.in index 875a143..5555edf 100644 --- a/contrib/rankmirrors.sh.in +++ b/contrib/rankmirrors.sh.in @@ -127,7 +127,7 @@ while [[ $1 ]]; do verbose) VERBOSE=1 ; shift ;; url) CHECKURL=1; [[ $2 ]] || err "Must specify url."; URL="$2"; shift 2;; repo) [[ $2 ]] || err "Must specify repo name."; TARGETREPO="$2"; shift 2;; - *) err "\`$1' is an invalid argument." + *) err "'$1' is an invalid argument." esac elif [[ ${1:0:1} = - ]]; then

@@ -146,7 +146,7 @@ while [[ $1 ]]; do u) CHECKURL=1; [[ $2 ]] || err "Must specify url."; URL="$2"; snum=2;; r) [[ $2 ]] || err "Must specify repo name."; TARGETREPO="$2"; snum=2;; n) [[ $2 ]] || err "Must specify number." ; NUM="$2" ; snum=2;; - *) err "\`-$1' is an invald argument." ;; + *) err "'$1' is an invalid argument." ;; esac done shift $snum @@ -157,7 +157,7 @@ while [[ $1 ]]; do [[ $linearray ]] || err "File is empty." shift else - err "\`$1' does not exist." + err "'$1' does not exist." fi done

@@ -169,7 +169,7 @@ done # Single url handling if [[ $CHECKURL ]]; then url="$(getfetchurl "$URL")" - [[ $url = fail ]] && err "url \`$URL' is malformed." + [[ $url = fail ]] && err "url '$URL' is malformed." [[ $VERBOSE ]] && echo "Testing $url..." time=$(gettime "$url") echo "$URL : $time" @@ -193,7 +193,7 @@ for line in "${linearray[@]}"; do server="${line#*= }" server="${server%%#*}" url="$(getfetchurl "$server")" - [[ $url = fail ]] && err "url \`$URL' is malformed." + [[ $url = fail ]] && err "url '$URL' is malformed." time=$(gettime "$url") timesarray+=("$time $server")

diff --git a/contrib/updpkgsums.sh.in b/contrib/updpkgsums.sh.in index ffea96b..f459524 100644 --- a/contrib/updpkgsums.sh.in +++ b/contrib/updpkgsums.sh.in @@ -67,7 +67,7 @@ fi

# Check $PWD/ for permission to unlink the $buildfile and write a new one if [[ ! -w . ]]; then - printf $'==> ERROR: No write permission in `%s\'\n' "$PWD" + printf $'==> ERROR: No write permission in "%s"\n' "$PWD"

And suddenly we're using double quotes here instead of single quotes.

exit 1 fi

-- 1.8.4

On Thu, Sep 05, 2013 at 11:13:33AM -0400, Aaron Lindsay wrote:

On Sep 05 10:37, Dave Reisner wrote:

...

On Thu, Sep 05, 2013 at 10:27:43AM -0400, Aaron Lindsay wrote:

...

For instance, `rankmirrors nonexistent_file' should display: 'nonexistent_file' does not exist. rather than: $1 does not exist.

Signed-off-by: Aaron Lindsay ---

This looks more like a change in quoting than it does for error messages...

Sure, I just wrote it the way I did because the combination of quoting and m4 was resulting in error messages that weren't as helpful. Would you prefer I change the commit message to something else?

I'd always prefer that commit messages accurately reflect the changes being made. Something like "contrib: unify quoting in error messages" seems reasonable.

...

I've no problem with this as m4 enjoys chomping on backticks and mangling the strings, but, see below.

Okay, let me know your thoughts on this and my comments below and I can send a v2.

-Aaron

...

...

contrib/bacman.sh.in | 2 +- contrib/rankmirrors.sh.in | 10 +++++----- contrib/updpkgsums.sh.in | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/contrib/bacman.sh.in b/contrib/bacman.sh.in index 5435e40..33e3920 100644 --- a/contrib/bacman.sh.in +++ b/contrib/bacman.sh.in @@ -124,7 +124,7 @@ fi

if [[ ! -d $pkg_dir ]]; then error "package %s is found in pacman database," "${pkg_name}" - plain " but \`%s' is not a directory" "${pkg_dir}" + plain " but '%s' is not a directory" "${pkg_dir}" exit 1 fi

diff --git a/contrib/rankmirrors.sh.in b/contrib/rankmirrors.sh.in index 875a143..5555edf 100644 --- a/contrib/rankmirrors.sh.in +++ b/contrib/rankmirrors.sh.in @@ -127,7 +127,7 @@ while [[ $1 ]]; do verbose) VERBOSE=1 ; shift ;; url) CHECKURL=1; [[ $2 ]] || err "Must specify url."; URL="$2"; shift 2;; repo) [[ $2 ]] || err "Must specify repo name."; TARGETREPO="$2"; shift 2;; - *) err "\`$1' is an invalid argument." + *) err "'$1' is an invalid argument." esac elif [[ ${1:0:1} = - ]]; then

@@ -146,7 +146,7 @@ while [[ $1 ]]; do u) CHECKURL=1; [[ $2 ]] || err "Must specify url."; URL="$2"; snum=2;; r) [[ $2 ]] || err "Must specify repo name."; TARGETREPO="$2"; snum=2;; n) [[ $2 ]] || err "Must specify number." ; NUM="$2" ; snum=2;; - *) err "\`-$1' is an invald argument." ;; + *) err "'$1' is an invalid argument." ;; esac done shift $snum @@ -157,7 +157,7 @@ while [[ $1 ]]; do [[ $linearray ]] || err "File is empty." shift else - err "\`$1' does not exist." + err "'$1' does not exist." fi done

@@ -169,7 +169,7 @@ done # Single url handling if [[ $CHECKURL ]]; then url="$(getfetchurl "$URL")" - [[ $url = fail ]] && err "url \`$URL' is malformed." + [[ $url = fail ]] && err "url '$URL' is malformed." [[ $VERBOSE ]] && echo "Testing $url..." time=$(gettime "$url") echo "$URL : $time" @@ -193,7 +193,7 @@ for line in "${linearray[@]}"; do server="${line#*= }" server="${server%%#*}" url="$(getfetchurl "$server")" - [[ $url = fail ]] && err "url \`$URL' is malformed." + [[ $url = fail ]] && err "url '$URL' is malformed." time=$(gettime "$url") timesarray+=("$time $server")

diff --git a/contrib/updpkgsums.sh.in b/contrib/updpkgsums.sh.in index ffea96b..f459524 100644 --- a/contrib/updpkgsums.sh.in +++ b/contrib/updpkgsums.sh.in @@ -67,7 +67,7 @@ fi

# Check $PWD/ for permission to unlink the $buildfile and write a new one if [[ ! -w . ]]; then - printf $'==> ERROR: No write permission in `%s\'\n' "$PWD" + printf $'==> ERROR: No write permission in "%s"\n' "$PWD"

And suddenly we're using double quotes here instead of single quotes.

Ah, I guess I got lazy and did this instead of escaped single quotes. I can change this to: printf $'==> ERROR: No write permission in \'%s\'\n' "$PWD" or printf $"==> ERROR: No write permission in '%s'\n" "$PWD" Do you have any preference?

option c: "==> ERROR: No write permission in '%s'\n" "$PWD" $"" definitely isn't what you want. It performs localization if the right gettext variables are set, but it also has security holes because the localized strings can contain command substitutions which will be happily executed...

...

...

exit 1 fi

-- 1.8.4