diff mbox

[pacman-dev] makepkg: Don't interpret format specifiers in msg

Message ID 20180223104229.81632-1-jadedcyborg@gmail.com
State Rejected, archived
Headers show

Commit Message

Niklas Holm Feb. 23, 2018, 10:42 a.m. UTC
Message string containing for example windows paths would have
unexpected behaviour. For example the message "Check C:\foo\bar" would
be printed as "Check C:<newline>  oar".

Signed-off-by: Niklas Holm <jadedcyborg@gmail.com>
---
 scripts/libmakepkg/util/message.sh.in | 15 +++++----------
 scripts/library/output_format.sh      | 18 ++++++------------
 2 files changed, 11 insertions(+), 22 deletions(-)

Comments

Allan McRae Feb. 23, 2018, 11:18 a.m. UTC | #1
On 23/02/18 20:42, Niklas Holm wrote:
> Message string containing for example windows paths would have
> unexpected behaviour. For example the message "Check C:\foo\bar" would
> be printed as "Check C:<newline>  oar".
> 
> Signed-off-by: Niklas Holm <jadedcyborg@gmail.com>

Did you test this patch?
Allan McRae Feb. 23, 2018, 11:30 a.m. UTC | #2
On 23/02/18 21:18, Allan McRae wrote:
> On 23/02/18 20:42, Niklas Holm wrote:
>> Message string containing for example windows paths would have
>> unexpected behaviour. For example the message "Check C:\foo\bar" would
>> be printed as "Check C:<newline>  oar".
>>
>> Signed-off-by: Niklas Holm <jadedcyborg@gmail.com>
> 
> Did you test this patch?
> .
> 

Try something like:

msg() {
	local mesg=$1; shift
	printf -v mesg2 "${mesg}" "$@"
	printf "${GREEN}==>${ALL_OFF}${BOLD} %s${ALL_OFF}\n" "${mesg2}">&2
}
Eli Schwartz Feb. 23, 2018, 11:55 a.m. UTC | #3
On 02/23/2018 05:42 AM, Niklas Holm wrote:
> Message string containing for example windows paths would have
> unexpected behaviour. For example the message "Check C:\foo\bar" would
> be printed as "Check C:<newline>  oar".

I think you're missing the purpose of these functions. They are wrappers
for printf, so that you can use them the same way you would use printf.

That means that Windows paths and other things containing escape codes
are meant to be interjected into msg the same way you would interject
them into printf...

msg "Check %s" "C:\foo\bar"

This is working *exactly* as intended. You patch breaks that, because
now we cannot provide gettext'ized $mesg arguments that contain %s
format strings.
Allan McRae Feb. 23, 2018, 1:03 p.m. UTC | #4
On 23/02/18 21:55, Eli Schwartz wrote:
> On 02/23/2018 05:42 AM, Niklas Holm wrote:
>> Message string containing for example windows paths would have
>> unexpected behaviour. For example the message "Check C:\foo\bar" would
>> be printed as "Check C:<newline>  oar".
> 
> I think you're missing the purpose of these functions. They are wrappers
> for printf, so that you can use them the same way you would use printf.
> 
> That means that Windows paths and other things containing escape codes
> are meant to be interjected into msg the same way you would interject
> them into printf...
> 
> msg "Check %s" "C:\foo\bar"
> 
> This is working *exactly* as intended. You patch breaks that, because
> now we cannot provide gettext'ized $mesg arguments that contain %s
> format strings.

My question is, is the example purely hypothetical, or do we pass a path
not using a %s variable somewhere in the code base.

A
diff mbox

Patch

diff --git a/scripts/libmakepkg/util/message.sh.in b/scripts/libmakepkg/util/message.sh.in
index 33808de7..90799640 100644
--- a/scripts/libmakepkg/util/message.sh.in
+++ b/scripts/libmakepkg/util/message.sh.in
@@ -44,26 +44,21 @@  colorize() {
 }
 
 plain() {
-	local mesg=$1; shift
-	printf "${BOLD}    ${mesg}${ALL_OFF}\n" "$@" >&2
+	printf "${BOLD}    %s${ALL_OFF}\n" "$1" >&2
 }
 
 msg() {
-	local mesg=$1; shift
-	printf "${GREEN}==>${ALL_OFF}${BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
+	printf "${GREEN}==>${ALL_OFF}${BOLD} %s${ALL_OFF}\n" "$1" >&2
 }
 
 msg2() {
-	local mesg=$1; shift
-	printf "${BLUE}  ->${ALL_OFF}${BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
+	printf "${BLUE}  ->${ALL_OFF}${BOLD} %s${ALL_OFF}\n" "$1" >&2
 }
 
 warning() {
-	local mesg=$1; shift
-	printf "${YELLOW}==> $(gettext "WARNING:")${ALL_OFF}${BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
+	printf "${YELLOW}==> $(gettext "WARNING:")${ALL_OFF}${BOLD} %s${ALL_OFF}\n" "$1" >&2
 }
 
 error() {
-	local mesg=$1; shift
-	printf "${RED}==> $(gettext "ERROR:")${ALL_OFF}${BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
+	printf "${RED}==> $(gettext "ERROR:")${ALL_OFF}${BOLD} %s${ALL_OFF}\n" "$1" >&2
 }
diff --git a/scripts/library/output_format.sh b/scripts/library/output_format.sh
index 9f02c00b..2049aab3 100644
--- a/scripts/library/output_format.sh
+++ b/scripts/library/output_format.sh
@@ -1,32 +1,26 @@ 
 plain() {
 	(( QUIET )) && return
-	local mesg=$1; shift
-	printf "${BOLD}    ${mesg}${ALL_OFF}\n" "$@" >&1
+	printf "${BOLD}    %s${ALL_OFF}\n" "$1" >&1
 }
 
 msg() {
 	(( QUIET )) && return
-	local mesg=$1; shift
-	printf "${GREEN}==>${ALL_OFF}${BOLD} ${mesg}${ALL_OFF}\n" "$@" >&1
+	printf "${GREEN}==>${ALL_OFF}${BOLD} %s${ALL_OFF}\n" "$1" >&1
 }
 
 msg2() {
 	(( QUIET )) && return
-	local mesg=$1; shift
-	printf "${BLUE}  ->${ALL_OFF}${BOLD} ${mesg}${ALL_OFF}\n" "$@" >&1
+	printf "${BLUE}  ->${ALL_OFF}${BOLD} %s${ALL_OFF}\n" "$1" >&1
 }
 
 ask() {
-	local mesg=$1; shift
-	printf "${BLUE}::${ALL_OFF}${BOLD} ${mesg}${ALL_OFF}" "$@" >&1
+	printf "${BLUE}::${ALL_OFF}${BOLD} %s${ALL_OFF}" "$1" >&1
 }
 
 warning() {
-	local mesg=$1; shift
-	printf "${YELLOW}==> $(gettext "WARNING:")${ALL_OFF}${BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
+	printf "${YELLOW}==> $(gettext "WARNING:")${ALL_OFF}${BOLD} %s${ALL_OFF}\n" "$1" >&2
 }
 
 error() {
-	local mesg=$1; shift
-	printf "${RED}==> $(gettext "ERROR:")${ALL_OFF}${BOLD} ${mesg}${ALL_OFF}\n" "$@" >&2
+	printf "${RED}==> $(gettext "ERROR:")${ALL_OFF}${BOLD} %s${ALL_OFF}\n" "$1" >&2
 }