Code review comment for lp:~e7appew/ubuntu/wily/bombono-dvd/mux-files-with-spaces-wily

Revision history for this message
Carlos Maddela (e7appew) wrote :

It's been a while since I looked at this, but if I remember correctly,
when you quote %2% as well, all the rest of the arguments would be
treated as a single string, and the command fails.

On 08/06/16 04:51, Brian Murray wrote:
> Review: Needs Fixing
>
> The patch's quoting of %2% is inconsistent, in one part it is bordered by \" and in another it isn't.
>
> Diff comments:
>
>> === added file 'debian/patches/0002-Allow-muxing-of-files-with-spaces.patch'
>> --- debian/patches/0002-Allow-muxing-of-files-with-spaces.patch 1970-01-01 00:00:00 +0000
>> +++ debian/patches/0002-Allow-muxing-of-files-with-spaces.patch 2015-12-09 09:32:38 +0000
>> @@ -0,0 +1,39 @@
>> +From: Carlos Maddela <email address hidden>
>> +Date: Thu, 13 Aug 2015 15:05:04 +1000
>> +Subject: Allow muxing of files whose name or path contains spaces
>> +
>> +Description: Allow muxing of files whose name or path contains spaces.
>> + Arguments to mplex command should be quoted in case the files to be
>> + muxed have names or paths containing spaces.
>> +Author: Carlos Maddela <email address hidden>
>> +Origin: vendor
>> +Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/bombono-dvd/+bug/1484397
>> +Last-Update: 2015-12-09
>> +---
>> +This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
>> +---
>> + src/mgui/mux.cpp | 4 ++--
>> + 1 file changed, 2 insertions(+), 2 deletions(-)
>> +
>> +diff --git a/src/mgui/mux.cpp b/src/mgui/mux.cpp
>> +index 427058e..cc4922b 100644
>> +--- a/src/mgui/mux.cpp
>> ++++ b/src/mgui/mux.cpp
>> +@@ -99,7 +99,7 @@ static bool RunMuxing(const std::string& dest_path, const std::string& args)
>> + ExitData ed;
>> + {
>> + Execution::Pulse pls(prg_bar);
>> +- std::string cmd = boost::format("mplex -f 8 -o %1% %2%") % dest_path % args % bf::stop;
>> ++ std::string cmd = boost::format("mplex -f 8 -o \"%1%\" %2%") % dest_path % args % bf::stop;
> Shouldn't %2% be treated the same way as %1% here?
>
>> + AppendCommandText(txt_view, cmd);
>> + ed = ExecuteAsync(0, cmd.c_str(), TextViewAppender(txt_view), &edat.pid);
>> + }
>> +@@ -234,7 +234,7 @@ bool MuxStreams(std::string& dest_fname, const std::string& src_fname)
>> + if( res )
>> + {
>> + dlg.hide();
>> +- res = RunMuxing(dest_fname, boost::format("%1% %2%") % v_btn.get_filename() % a_btn.get_filename() % bf::stop );
>> ++ res = RunMuxing(dest_fname, boost::format("\"%1%\" \"%2%\"") % v_btn.get_filename() % a_btn.get_filename() % bf::stop );
>> + }
>> +
>> + return res;
>

« Back to merge proposal