[pacman-dev] [PATCH] contrib: Fix various error messages.

[Dave Reisner]

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 <aaron at aclindsay.com>
> > > ---
> > 
> > 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
> > > 
> > > 
> > 
> > 
>