Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

r499: Command-line escaping improvement

  • Loading branch information...
commit de3f1ad2d7cce98f3aa928f30d5c22619e1270bf 1 parent db77565
Alex Verkhovsky alexeyv authored
2  app/models/build.rb
View
@@ -20,7 +20,7 @@ def run
begin
raise ConfigError.new(@project.error_message) unless @project.config_valid?
in_clean_environment_on_local_copy do
- execute self.command, :stdout => build_log, :stderr => build_log, :escape_quotes => false
+ execute self.command, :stdout => build_log, :stderr => build_log
end
build_status.succeed!(seconds_since(start))
rescue => e
26 app/models/subversion.rb
View
@@ -19,13 +19,13 @@ def clean_checkout(target_directory, revision = nil, stdout = $stdout)
def checkout(target_directory, revision = nil, stdout = $stdout)
@url or raise 'URL not specified'
- options = "#{@url} #{target_directory}"
- options << " --username #{@username}" if username
- options << " --password #{@password}" if password
- options << " --revision #{revision_number(revision)}" if revision
+ options = [@url, target_directory]
+ options << "--username" << @username if @username
+ options << "--password" << @password if @password
+ options << "--revision" << revision_number(revision) if revision
# need to read from command output, because otherwise tests break
- execute(svn(:co, options)) do |io|
+ execute(svn('co', options)) do |io|
begin
while line = io.gets
stdout.puts line
@@ -50,26 +50,26 @@ def revisions_since(project, revision_number)
def update(project, revision = nil)
revision_number = revision ? revision_number(revision) : 'HEAD'
- svn_output = execute_in_local_copy(project, svn(:update, "--revision #{revision_number}"))
+ svn_output = execute_in_local_copy(project, svn('update', "--revision", revision_number))
SubversionLogParser.new.parse_update(svn_output)
end
private
def log(from, to)
- svn(:log, "--revision #{from}:#{to} --verbose --xml #{@url}")
+ svn('log', "--revision", "#{from}:#{to}", '--verbose', '--xml', @url)
end
def info(project)
- svn_output = execute_in_local_copy(project, svn(:info, "--xml"))
+ svn_output = execute_in_local_copy(project, svn('info', "--xml"))
SubversionLogParser.new.parse_info(svn_output)
end
- def svn(operation, options = nil)
- command = "svn"
- command << " --non-interactive" unless @interactive
- command << " " << operation.to_s
- command << " " << options if options
+ def svn(operation, *options)
+ command = ["svn"]
+ command << "--non-interactive" unless @interactive
+ command << operation
+ command += options.compact.flatten
command
end
4 cruisecontrol.ipr
View
@@ -6,7 +6,6 @@
<component name="BuildJarProjectSettings">
<option name="BUILD_JARS_ON_MAKE" value="false" />
</component>
- <component name="ClearCase" />
<component name="CodeStyleProjectProfileManger">
<option name="PROJECT_PROFILE" />
<option name="USE_PROJECT_LEVEL_SETTINGS" value="false" />
@@ -265,6 +264,7 @@
</item>
</group>
</component>
+ <component name="ProjectFileVersion" converted="true" />
<component name="ProjectModuleManager">
<modules>
<module fileurl="file://$PROJECT_DIR$/cruisecontrol.iml" filepath="$PROJECT_DIR$/cruisecontrol.iml" />
@@ -293,11 +293,9 @@
<option name="GENERATE_IIOP_STUBS" value="false" />
<option name="ADDITIONAL_OPTIONS_STRING" value="" />
</component>
- <component name="StarteamVcsAdapter" />
<component name="VcsDirectoryMappings">
<mapping directory="" vcs="" />
</component>
- <component name="VssVcs" />
<component name="com.intellij.jsf.UserDefinedFacesConfigs">
<option name="USER_DEFINED_CONFIGS">
<value>
48 lib/command_line.rb
View
@@ -61,11 +61,9 @@ def execute(cmd, options={}, &proc)
raise "Can't have newline in cmd" if cmd =~ /\n/
options = {
:dir => Dir.pwd,
- :escape_quotes => true,
:env => {},
:mode => 'r',
- :exitstatus => 0
- }.merge(options)
+ :exitstatus => 0 }.merge(options)
options[:stdout] = File.expand_path(options[:stdout]) if options[:stdout]
options[:stderr] = File.expand_path(options[:stderr]) if options[:stderr]
@@ -110,23 +108,25 @@ def e(cmd, options, &proc)
module_function :e
def full_cmd(cmd, options, &proc)
- commands = cmd.split("&&").collect{|c| c.strip}
stdout_opt, stderr_opt = redirects(options)
capture_info_command = (block_given? && options[:stdout]) ?
"echo [output captured and therefore not logged] >> #{options[:stdout]} && " :
- ""
-
- full_cmd = commands.collect do |c|
- escaped_command = options[:escape_quotes] ? c.gsub(/"/, QUOTE_REPLACEMENT).gsub(/</, LESS_THAN_REPLACEMENT) : c
- stdout_prompt_command = options[:stdout] ? "echo #{Platform.prompt} #{escaped_command} >> #{options[:stdout]} && " : ""
- stderr_prompt_command = options[:stderr] && options[:stderr] != options[:stdout] ?
- "echo #{Platform.prompt} #{escaped_command} >> #{options[:stderr]} && " :
- ""
- redirected_command = block_given? ? "#{c} #{stderr_opt}" : "#{c} #{stdout_opt} #{stderr_opt}"
-
- stdout_prompt_command + capture_info_command + stderr_prompt_command + redirected_command
- end.join(" && ")
+ ''
+
+ cmd = escape_and_concatenate(cmd) unless cmd.is_a? String
+
+ stdout_prompt_command = options[:stdout] ?
+ "echo #{Platform.prompt} #{cmd} >> #{options[:stdout]} && " :
+ ''
+
+ stderr_prompt_command = options[:stderr] && options[:stderr] != options[:stdout] ?
+ "echo #{Platform.prompt} #{cmd} >> #{options[:stderr]} && " :
+ ''
+
+ redirected_command = block_given? ? "#{cmd} #{stderr_opt}" : "#{cmd} #{stdout_opt} #{stderr_opt}"
+
+ stdout_prompt_command + capture_info_command + stderr_prompt_command + redirected_command
end
module_function :full_cmd
@@ -169,5 +169,21 @@ def redirects(options)
[stdout_opt, stderr_opt]
end
module_function :redirects
+
+ def escape_and_concatenate(cmd)
+ cmd.map { |item| escape(item) }.join(' ')
+ end
+ module_function :escape_and_concatenate
+ def escape(item)
+ if Platform.family == 'mswin32'
+ raise 'not implemented yet'
+ else
+ escaped_characters = /"|'|<|>| |&|\||\(|\)|\\|\$|\*|\?|;/
+ end
+
+ item.to_s.gsub(escaped_characters) { |match| "\\#{match}" }
+ end
+ module_function :escape
+
end
9 test/unit/build_test.rb
View
@@ -87,8 +87,7 @@ def test_run_successful_build
expected_build_log = File.join(expected_build_directory, 'build.log')
expected_redirect_options = {
:stdout => expected_build_log,
- :stderr => expected_build_log,
- :escape_quotes => false
+ :stderr => expected_build_log
}
Time.expects(:now).at_least(2).returns(Time.at(0), Time.at(3.2))
build.expects(:execute).with(build.rake, expected_redirect_options).returns("hi, mom!")
@@ -122,8 +121,7 @@ def test_run_unsuccessful_build
expected_build_log = File.join(expected_build_directory, 'build.log')
expected_redirect_options = {
:stdout => expected_build_log,
- :stderr => expected_build_log,
- :escape_quotes => false
+ :stderr => expected_build_log
}
error = RuntimeError.new
@@ -145,8 +143,7 @@ def test_warn_on_mistake_check_out_if_trunk_dir_exists
expected_build_log = File.join(expected_build_directory, 'build.log')
expected_redirect_options = {
:stdout => expected_build_log,
- :stderr => expected_build_log,
- :escape_quotes => false
+ :stderr => expected_build_log
}
build.expects(:execute).with(build.rake, expected_redirect_options).raises(CommandLine::ExecutionError)
27 test/unit/command_line_test.rb
View
@@ -6,9 +6,9 @@ class CommandLineTest < Test::Unit::TestCase
def test_should_write_to_both_files_when_both_files_specified_and_no_block
in_total_sandbox do
- CommandLine.execute("echo \"<hello\" && echo world", {:dir => @dir, :stdout => @stdout, :stderr => @stderr})
- assert_match(/.* echo \"<hello\"\s*\n.?\<hello.?\s*\n.* echo world\s*\nworld/n, File.read(@stdout))
- assert_match(/.* echo \"<hello\"\s*\n.* echo world\s*/n, File.read(@stderr))
+ CommandLine.execute("echo hello", {:dir => @dir, :stdout => @stdout, :stderr => @stderr})
+ assert_match(/.* echo hello\n.?hello/n, File.read(@stdout))
+ assert_match(/.* echo hello/n, File.read(@stderr))
end
end
@@ -72,6 +72,27 @@ def test_execute_should_raise_when_return_code_is_not_zero
end
end
+ def test_escape_and_concatenate
+ Platform.stubs(:family).returns("linux")
+ assert_equal 'foo', CommandLine.escape_and_concatenate(['foo'])
+ assert_equal 'foo bar', CommandLine.escape_and_concatenate(['foo', 'bar'])
+ assert_equal 'foo b\\"ar', CommandLine.escape_and_concatenate(['foo', 'b"ar'])
+ assert_equal 'foo b\\ \\ \\ ar', CommandLine.escape_and_concatenate(['foo', 'b ar'])
+ assert_equal "foo b\\'\\&\\<\\>\\\\\\|\\$\\*\\?\\;ar", CommandLine.escape_and_concatenate(['foo', "b'&<>\\|$*?;ar"])
+ end
+
+ def test_full_cmd_should_not_escape_command_if_it_is_a_string
+ assert_equal 'foo bar\ baz ', CommandLine.full_cmd('foo bar\ baz', {})
+ end
+
+ def test_full_cmd_should_escape_command_if_it_is_an_array
+ assert_equal 'foo bar baz\\ \\? ', CommandLine.full_cmd(['foo', 'bar', 'baz ?'], {})
+ end
+
+ def test_escape_and_concatenate_accepts_non_strings
+ assert_equal 'foo 10', CommandLine.escape_and_concatenate(['foo', 10])
+ end
+
def with_redirected_stdout
orgout = STDOUT.dup
STDOUT.reopen(@stdout)
25 test/unit/subversion_test.rb
View
@@ -50,7 +50,7 @@ def test_update_with_revision_number
revision_number = 10
svn = Subversion.new
- svn.expects(:execute).with("svn --non-interactive update --revision #{revision_number}", {:stderr => './svn.err'}).returns("your mom")
+ svn.expects(:execute).with(["svn", "--non-interactive", "update", "--revision", revision_number], {:stderr => './svn.err'}).returns("your mom")
svn.update(dummy_project, Revision.new(revision_number))
end
@@ -59,7 +59,8 @@ def test_latest_revision
svn = Subversion.new
svn.expects(:info).with(dummy_project).returns(Subversion::Info.new(10, 10))
- svn.expects(:execute).with("svn --non-interactive log --revision HEAD:10 --verbose --xml ", {:stderr => './svn.err'}).yields(StringIO.new(LOG_ENTRY))
+ svn.expects(:execute).with(["svn", "--non-interactive", "log", "--revision", "HEAD:10", "--verbose", "--xml"],
+ {:stderr => './svn.err'}).yields(StringIO.new(LOG_ENTRY))
revision = svn.latest_revision(dummy_project)
@@ -69,7 +70,8 @@ def test_latest_revision
def test_revisions_since_should_reverse_the_log_entries_and_skip_the_one_corresponding_to_current_revision
svn = Subversion.new
- svn.expects(:execute).with("svn --non-interactive log --revision HEAD:15 --verbose --xml ", {:stderr => './svn.err'}).yields(StringIO.new(LOG_ENTRY))
+ svn.expects(:execute).with(["svn", "--non-interactive", "log", "--revision", "HEAD:15", "--verbose", "--xml"],
+ {:stderr => './svn.err'}).yields(StringIO.new(LOG_ENTRY))
revisions = svn.revisions_since(dummy_project, 15)
@@ -79,7 +81,8 @@ def test_revisions_since_should_reverse_the_log_entries_and_skip_the_one_corresp
def test_revisions_since_should_return_all_revisions_when_curreent_revision_is_not_in_the_log_output
svn = Subversion.new
- svn.expects(:execute).with("svn --non-interactive log --revision HEAD:14 --verbose --xml ", {:stderr => './svn.err'}).yields(StringIO.new(LOG_ENTRY))
+ svn.expects(:execute).with(["svn", "--non-interactive", "log", "--revision", "HEAD:14", "--verbose", "--xml"],
+ {:stderr => './svn.err'}).yields(StringIO.new(LOG_ENTRY))
revisions = svn.revisions_since(dummy_project, 14)
@@ -89,7 +92,8 @@ def test_revisions_since_should_return_all_revisions_when_curreent_revision_is_n
def test_revisions_since_should_return_an_empty_array_for_empty_log_output
svn = Subversion.new
- svn.expects(:execute).with("svn --non-interactive log --revision HEAD:14 --verbose --xml ", {:stderr => './svn.err'}).yields(StringIO.new(EMPTY_LOG))
+ svn.expects(:execute).with(["svn", "--non-interactive", "log", "--revision", "HEAD:14", "--verbose", "--xml"],
+ {:stderr => './svn.err'}).yields(StringIO.new(EMPTY_LOG))
revisions = svn.revisions_since(dummy_project, 14)
@@ -98,7 +102,7 @@ def test_revisions_since_should_return_an_empty_array_for_empty_log_output
def test_checkout_with_no_user_password
svn = Subversion.new(:url => 'http://foo.com/svn/project')
- svn.expects(:execute).with("svn --non-interactive co http://foo.com/svn/project .")
+ svn.expects(:execute).with(["svn", "--non-interactive", "co", "http://foo.com/svn/project", "."])
svn.checkout('.')
end
@@ -122,21 +126,22 @@ def test_should_write_error_info_to_log_when_svn_server_not_available
def test_checkout_with_user_password
svn = Subversion.new(:url => 'http://foo.com/svn/project', :username => 'jer', :password => "crap")
- svn.expects(:execute).with("svn --non-interactive co http://foo.com/svn/project . --username jer --password crap")
+ svn.expects(:execute).with(["svn", "--non-interactive", "co", "http://foo.com/svn/project", ".", "--username",
+ "jer", "--password", "crap"])
svn.checkout('.')
end
def test_checkout_with_revision
svn = Subversion.new(:url => 'http://foo.com/svn/project')
- svn.expects(:execute).with("svn --non-interactive co http://foo.com/svn/project . --revision 5")
+ svn.expects(:execute).with(["svn", "--non-interactive", "co", "http://foo.com/svn/project", ".", "--revision", 5])
svn.checkout('.', Revision.new(5))
end
def test_allowing_interaction
svn = Subversion.new(:url => 'svn://foo.com/', :interactive => true)
- svn.expects(:execute).with("svn co svn://foo.com/ .")
+ svn.expects(:execute).with(["svn", "co", "svn://foo.com/", "."])
svn.checkout('.')
svn.verify
end
@@ -157,7 +162,7 @@ def test_clean_checkout
dir = @sandbox.root + "/project"
svn = Subversion.new(:url => 'http://foo.com/svn/project')
- svn.expects(:execute).with("svn --non-interactive co http://foo.com/svn/project #{dir} --revision 5")
+ svn.expects(:execute).with(["svn", "--non-interactive", "co", "http://foo.com/svn/project", dir, "--revision", 5])
svn.clean_checkout(dir, Revision.new(5))
Please sign in to comment.
Something went wrong with that request. Please try again.