Hi kou,
On 2/3/07, Kouhei Sutou <kou@cozmixng.org> wrote:
> Hi,
>
> In <20070203.194319.222490146.kou@cozmixng.org>
> "Re: Status report for Ruby bindings on Windows" on Sat, 03 Feb 2007 19:43:19 +0900 (JST),
> Kouhei Sutou <kou@cozmixng.org> wrote:
>
> > > > > > > > > * Normalizing the line breaks. Upon reflection, I've decided that I
> > > > > > > > > started us down the wrong path when I modified the tests for the line
> > > > > > > > > ending issues. As I've thought about it more, I now believe that the
> > > > > > > > > bindings should do the conversions. My thinking is since Ruby treats
> > > > > > > > > line endings as "'\n" instead of "\r\n" the bindings should provide
> > > > > > > > > any multiline values Ruby expects. (This is where the 'b' comes from
> > > > > > > > > when calling File::open. Without the 'b' on windows there is a \r\n
> > > > > > > > > to \n conversion. With the 'b', no conversion. Maybe we should check
> > > > > > > > > out what the Perl and Python bindings do about this issue on
> > > > > > > > > windows?). (I've made zero effort to figure out what this will take).
> > > > > > > >
> > > > > > > > OK. We'll try the problem after we solve test environment
> > > > > > > > problem on Windows.
> > > > > > > >
> > > > > > >
> > > > > > > Removing all invocations and the definition of normalize_line_break
> > > > > > > from the test\*.rb files and the attached patch get us:
> > > > > >
> > > > > > It seems that the changes make cat ignore svn:eof-style. I
> > > > > > think tests have a problem. Could you try the attached
> > > > > > patch?
> > > > >
> > > > > The client tests pass with the patch. I (now) still think normalizing
> > > > > the line breaks in the test is the wrong solution. As you pointed
> > > > > out, my patch wasn't right for cat, but I hope you'll consider parts
> > > > > of it, or something like it for the the Core methods. Ruby on windows
> > > > > uses \n for line breaks, even though windows uses \r\n. This gives us
> > > > > a real problem to know when to pass the \r through and when not. I'll
> > > > > see if I can figure out how (and if) the perl bindings deal with the
> > > > > issue.
> > > >
> > > > OK. Could you give me a time to think this issue?
> > > >
> > >
> > > Of course.
> >
> > The attached patch is my answer.
> >
> > 1. Remove normalize_line_break.
Great.
> > 2. Use here document and binary mode open.
Here docs don't really make a difference. I sort of prefered the
other way, but
it doesn't really matter, opening in binary causes ruby to not fiddle with line
endings and that's the key
> > 3. Don't use both gsub(/\r\n/, "\n") and gsub(/\n/, "\r\n").
>
>
> I'm sorry. There are some typos. I'll attache a revised
> patch.
>
> Thanks,
> --
> kou
After patching (and see comments inline) this is what I got:
D:\SVN\src-trunk\subversion\bindings\swig\ruby>ruby test\run-test.rb
Loaded suite test
Started
..................................................................F.....................F.......F..........................................
Finished in 646.233 seconds.
1) Failure:
test_diff_unified(SvnCoreTest)
[D:/SVN/src-trunk/subversion/bindings/swig/ruby/test/test_core.rb:366]:
<"--- (orig)\n+++ (mod)\n@@ -1,3 +1,3 @@\n- a\n-b\n- c\n+a\n+\n+
c\n"> expected but was
<"--- (orig)\r\n+++ (mod)\r\n@@ -1,3 +1,3 @@\r\n- a\n-b\n- c\n+a\n+\n+ c\n">.
2) Failure:
test_delta(SvnFsTest)
[D:/SVN/src-trunk/subversion/bindings/swig/ruby/test/test_fs.rb:320]:
teardown errors (and fouls up subsequent tests) if we allow the next
line to execute.
3) Failure:
test_diff(SvnInfoTest)
[D:/SVN/src-trunk/subversion/bindings/swig/ruby/test/test_info.rb:202]:
<2> expected but was
<1>.
139 tests, 897 assertions, 3 failures, 0 errors
>
> Index: subversion/bindings/swig/ruby/test/util.rb
> ===================================================================
> --- subversion/bindings/swig/ruby/test/util.rb (revision 23318)
> +++ subversion/bindings/swig/ruby/test/util.rb (working copy)
> @@ -176,14 +176,6 @@
> auth_baton[Svn::Core::AUTH_PARAM_DEFAULT_USERNAME] = @author
> end
>
> - def normalize_line_break(str)
> - if windows?
> - str.gsub(/\n/, "\r\n")
> - else
> - str
> - end
> - end
> -
> module_function
> def windows?
> /cygwin|mingw|mswin32|bccwin32/.match(RUBY_PLATFORM)
> Index: subversion/bindings/swig/ruby/test/test_client.rb
> ===================================================================
> --- subversion/bindings/swig/ruby/test/test_client.rb (revision 23317)
> +++ subversion/bindings/swig/ruby/test/test_client.rb (working copy)
> @@ -820,7 +820,9 @@
> def test_merge
> log = "sample log"
> file = "sample.txt"
> - src = normalize_line_break("sample\n")
> + src = <<-EOS
> +sample
> +EOS
> trunk = File.join(@wc_path, "trunk")
> branch = File.join(@wc_path, "branch")
> trunk_path = File.join(trunk, file)
> @@ -834,7 +836,7 @@
> ctx.add(branch_path)
> rev1 = ctx.commit(@wc_path).revision
>
> - File.open(branch_path, "w") {|f| f.print(src)}
> + File.open(branch_path, "wb") {|f| f.print(src)}
> rev2 = ctx.commit(@wc_path).revision
>
> ctx.merge(branch, rev1, branch, rev2, trunk)
> @@ -867,7 +869,9 @@
> def test_merge_peg
> log = "sample log"
> file = "sample.txt"
> - src = normalize_line_break("sample\n")
> + src = <<-EOS
> +sample
> +EOS
> trunk = File.join(@wc_path, "trunk")
> branch = File.join(@wc_path, "branch")
> trunk_path = File.join(trunk, file)
> @@ -881,7 +885,7 @@
> ctx.add(branch_path)
> rev1 = ctx.commit(@wc_path).revision
>
> - File.open(branch_path, "w") {|f| f.print(src)}
> + File.open(branch_path, "wb") {|f| f.print(src)}
> rev2 = ctx.commit(@wc_path).revision
>
> ctx.merge_peg(branch, rev1, rev2, trunk)
> @@ -1273,12 +1277,16 @@
>
> def test_cat
> log = "sample log"
> - src1 = normalize_line_break("source1\n")
> - src2 = normalize_line_break("source2\n")
> + src1 = <<-EOS
> +source1
> +EOS
> + src2 = <<-EOS
> +source2
> +EOS
> file = "sample.txt"
> path = File.join(@wc_path, file)
>
> - File.open(path, "w") {|f| f.print(src1)}
> + File.open(path, "wb") {|f| f.print(src1)}
>
> ctx = make_context(log)
> ctx.add(path)
> @@ -1288,7 +1296,7 @@
> assert_equal(src1, ctx.cat(path, rev1))
> assert_equal(src1, ctx.cat(path))
>
> - File.open(path, "w") {|f| f.print(src2)}
> + File.open(path, "wb") {|f| f.print(src2)}
>
> commit_info = ctx.commit(@wc_path)
> rev2 = commit_info.revision
> @@ -1552,8 +1560,12 @@
>
> def test_switch
> log = "sample log"
> - trunk_src = normalize_line_break("trunk source\n")
> - tag_src = normalize_line_break("tag source\n")
> + trunk_src = <<-EOS
> +trunk source
> +EOS
> + tag_src = <<-EOS
> +tag source
> +EOS
> file = "sample.txt"
> file = "sample.txt"
> trunk_dir = "trunk"
> @@ -1571,12 +1583,12 @@
> ctx = make_context(log)
>
> ctx.mkdir(trunk_dir_path)
> - File.open(trunk_path, "w") {|f| f.print(trunk_src)}
> + File.open(trunk_path, "wb") {|f| f.print(trunk_src)}
> ctx.add(trunk_path)
> trunk_rev = ctx.commit(@wc_path).revision
>
> ctx.mkdir(tag_dir_path, tag_name_dir_path)
> - File.open(tag_path, "w") {|f| f.print(tag_src)}
> + File.open(tag_path, "wb") {|f| f.print(tag_src)}
> ctx.add(tag_path)
> tag_rev = ctx.commit(@wc_path).revision
>
> @@ -1614,12 +1626,14 @@
>
> def test_authentication
> log = "sample log"
> - src = normalize_line_break("source\n")
> + src = <<-EOS
> +source
> +EOS
> file = "sample.txt"
> path = File.join(@wc_path, file)
> svnserve_uri = "#{@repos_svnserve_uri}/#{file}"
>
> - File.open(path, "w") {|f| f.print(src)}
> + File.open(path, "wb") {|f| f.print(src)}
>
> ctx = make_context(log)
> ctx.add(path)
> @@ -1659,12 +1673,14 @@
>
> def test_simple_provider
> log = "sample log"
> - src = normalize_line_break("source\n")
> + src = <<-EOS
> +source
> +EOS
> file = "sample.txt"
> path = File.join(@wc_path, file)
> svnserve_uri = "#{@repos_svnserve_uri}/#{file}"
>
> - File.open(path, "w") {|f| f.print(src)}
> + File.open(path, "wb") {|f| f.print(src)}
>
> ctx = make_context(log)
> setup_auth_baton(ctx.auth_baton)
> @@ -1675,7 +1691,7 @@
> setup_auth_baton(ctx.auth_baton)
> ctx.add_simple_provider
> assert_raises(Svn::Error::RaNotAuthorized) do
> - assert_equal(src, ctx.cat(svnserve_uri))
> + ctx.cat(svnserve_uri)
> end
>
> ctx = Svn::Client::Context.new
> @@ -1698,12 +1714,14 @@
> return unless Svn::Core.respond_to?(:add_windows_simple_provider)
>
> log = "sample log"
> - src = normalize_line_break("source\n")
> + src = <<-EOS
> +source
> +EOS
> file = "sample.txt"
> path = File.join(@wc_path, file)
> svnserve_uri = "#{@repos_svnserve_uri}/#{file}"
>
> - File.open(path, "w") {|f| f.print(src)}
> + File.open(path, "wb") {|f| f.print(src)}
>
> ctx = make_context(log)
> setup_auth_baton(ctx.auth_baton)
> @@ -1714,7 +1732,7 @@
> setup_auth_baton(ctx.auth_baton)
> ctx.add_windows_simple_provider
> assert_raises(Svn::Error::RaNotAuthorized) do
> - assert_equal(src, ctx.cat(svnserve_uri))
> + ctx.cat(svnserve_uri)
> end
>
> ctx = Svn::Client::Context.new
> Index: subversion/bindings/swig/ruby/test/test_core.rb
> ===================================================================
> --- subversion/bindings/swig/ruby/test/test_core.rb (revision 23317)
> +++ subversion/bindings/swig/ruby/test/test_core.rb (working copy)
> @@ -343,9 +343,11 @@
> modified_header = "(mod)"
>
> original.open
> + original.binmode
> original.print(original_src)
> original.close
> modified.open
> + modified.binmode
> modified.print(modified_src)
> modified.close
>
> @@ -361,8 +363,7 @@
> + c
> EOD
The issue causing this failure is not the line endings of the files being
compared, but the line endings of the multiline return from file_diff. That is
what my previously submitted patch to core.rb
(http://article.gmane.org/gmane.comp.version-control.subversion.devel/84910)
was trying to address.
You know, perhaps it would be better to change the interface to return
an array of strings instead of a single string with multiple lines?
> diff = Svn::Core::Diff.file_diff(original.path, modified.path)
> - assert_equal(normalize_line_break(expected),
> - diff.unified(original_header, modified_header))
> + assert_equal(expected, diff.unified(original_header, modified_header))
>
> options = Svn::Core::DiffFileOptions.parse("--ignore-space-change")
> expected = <<-EOD
> @@ -376,8 +377,7 @@
> c
> EOD
> diff = Svn::Core::Diff.file_diff(original.path, modified.path, options)
> - assert_equal(normalize_line_break(expected),
> - diff.unified(original_header, modified_header))
> + assert_equal(expected, diff.unified(original_header, modified_header))
>
> options = Svn::Core::DiffFileOptions.parse("--ignore-all-space")
> expected = <<-EOD
> @@ -390,8 +390,7 @@
> c
> EOD
> diff = Svn::Core::Diff.file_diff(original.path, modified.path, options)
> - assert_equal(normalize_line_break(expected),
> - diff.unified(original_header, modified_header))
> + assert_equal(expected, diff.unified(original_header, modified_header))
> end
>
> def test_diff_merge
> @@ -421,12 +420,15 @@
> EOS
>
> original.open
> + original.binmode
> original.print(original_src)
> original.close
> modified.open
> + modified.binmode
> modified.print(modified_src)
> modified.close
> latest.open
> + latest.binmode
> latest.print(latest_src)
> latest.close
>
> @@ -440,7 +442,7 @@
> diff = Svn::Core::Diff.file_diff3(original.path,
> modified.path,
> latest.path)
> - assert_equal(normalize_line_break(expected), diff.merge)
> + assert_equal(expected, diff.merge)
>
> options = Svn::Core::DiffFileOptions.parse("--ignore-space-change")
> expected = <<-EOD
> @@ -454,7 +456,7 @@
> modified.path,
> latest.path,
> options)
> - assert_equal(normalize_line_break(expected), diff.merge)
> + assert_equal(expected, diff.merge)
>
> options = Svn::Core::DiffFileOptions.parse("--ignore-all-space")
> expected = <<-EOD
> @@ -468,7 +470,7 @@
> modified.path,
> latest.path,
> options)
> - assert_equal(normalize_line_break(expected), diff.merge)
> + assert_equal(expected, diff.merge)
> end
>
> def test_diff_file_options
> Index: subversion/bindings/swig/ruby/test/test_fs.rb
> ===================================================================
> --- subversion/bindings/swig/ruby/test/test_fs.rb (revision 23317)
> +++ subversion/bindings/swig/ruby/test/test_fs.rb (working copy)
> @@ -293,20 +293,44 @@
> def test_delta
> log = "sample log"
> file = "source.txt"
> - src = "a\nb\nc\nd\ne\n"
> - modified = "A\nb\nc\nd\nE\n"
> - result = "a\n\n\n\ne\n"
> - expected = "A\n\n\n\nE\n"
> + src = <<-EOS
> +a
> +b
> +c
> +d
> +e
> +EOS
> + modified = <<-EOS
> +A
> +b
> +c
> +d
> +E
> +EOS
> + result = <<-EOS
> +a
> +
> +
> +
> +e
> +EOS
> + expected = <<-EOS
> +A
> +
> +
> +
> +E
> +EOS
> path_in_repos = "/#{file}"
> path = File.join(@wc_path, file)
>
> ctx = make_context(log)
>
> - File.open(path, "w") {|f| f.print(src)}
> + File.open(path, "wb") {|f| f.print(src)}
> ctx.add(path)
> rev1 = ctx.ci(@wc_path).revision
>
> - File.open(path, "w") {|f| f.print(modified)}
> + File.open(path, "wb") {|f| f.print(modified)}
> @fs.transaction do |txn|
> checksum = MD5.new(result).hexdigest
> stream = txn.root.apply_text(path_in_repos, checksum)
> @@ -314,7 +338,7 @@
> stream.close
> end
> ctx.up(@wc_path)
> - assert_equal(expected, File.open(path){|f| f.read})
> + assert_equal(expected, File.open(path) {|f| f.read})
>
> rev2 = ctx.ci(@wc_path).revision
I added this line so the tests following this test would run. Perhaps there is
a leaking pool? (The errors in teardown where similar to what we were seeing
before).
flunk "teardown errors (and fouls up subsequent tests) if we allow
the next line to execute"
> stream = @fs.root(rev2).file_delta_stream(@fs.root(rev1),
> Index: subversion/bindings/swig/ruby/test/test_info.rb
> ===================================================================
> --- subversion/bindings/swig/ruby/test/test_info.rb (revision 23317)
> +++ subversion/bindings/swig/ruby/test/test_info.rb (working copy)
> @@ -176,7 +176,7 @@
> file4_prop_key = "XXX"
> file4_prop_value = "YYY"
> File.open(file1_path, "w") {|f| f.puts "changed"}
> - File.open(file2_path, "w") {|f| f.puts "removed\nadded"}
> + File.open(file2_path, "w") {|f| f.puts "removed"; f.puts "added"}
The above didn't change the result of the test, but opening the file 'wb' does.
> FileUtils.touch(file4_path)
> ctx.add(file4_path)
> ctx.propdel(file1_prop_key, file1_path)
> @@ -205,10 +205,14 @@
> assert_equal(0, info.diffs[file4][:added].deleted_line)
> assert_equal(0, info.diffs[file5_key][:copied].added_line)
> assert_equal(0, info.diffs[file5_key][:copied].deleted_line)
> - assert_equal("Name: #{file1_prop_key}\n - #{file1_prop_value}\n",
> - info.diffs[file1][:property_changed].body)
> - assert_equal("Name: #{file4_prop_key}\n + #{file4_prop_value}\n",
> - info.diffs[file4][:property_changed].body)
> + assert_equal(<<-EOD, info.diffs[file1][:property_changed].body)
> +Name: #{file1_prop_key}
> + - #{file1_prop_value}
> +EOD
> + assert_equal(<<-EOD, info.diffs[file4][:property_changed].body)
> +Name: #{file4_prop_key}
> + + #{file4_prop_value}
> +EOD
> assert_equal(commit_info.revision, info.revision)
> assert_equal(log, info.log)
> end
> Index: subversion/bindings/swig/ruby/svn/util.rb
> ===================================================================
> --- subversion/bindings/swig/ruby/svn/util.rb (revision 23317)
> +++ subversion/bindings/swig/ruby/svn/util.rb (working copy)
> @@ -76,7 +76,7 @@
> def filename_to_temp_file(filename)
> file = Tempfile.new("svn-ruby")
> file.binmode
> - file.print(File.read(filename))
> + file.print(File.open(filename, "rb") {|f| f.read})
> file.close
> file.open
> file.binmode
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Feb 4 20:02:30 2007