Re: [PATCH] Ruby bindings client for merge_peg

Hi,

> > How about following the same algorithm for defaulting the peg revision
> > as in subversion/svn/merge-cmd.c?
> >
> > If a peg revision is unspecified, it's defaulted to HEAD for URLs, and
> > WORKING for WC paths.
> >
> >
>
> That seems like a good idea. This is what it looks like, what do you think?

I think so. Please commit after applying my suggestions.

> Index: subversion/bindings/swig/ruby/test/test_client.rb

> @@ -911,12 +911,10 @@

> ctx.propdel("svn:mergeinfo", trunk)
> - merge_info = ctx.merge_info(trunk)
> - assert_equal(["/branch"], merge_info.keys)
> - assert_equal([[2, 2]], merge_info["/branch"].collect {|range| range.to_a})
> + assert_nil ctx.merge_info(trunk)

Don't omit parenthesis:
  assert_nil(ctx.merge_info(trunk))

> Index: subversion/bindings/swig/ruby/svn/client.rb
> ===================================================================
> --- subversion/bindings/swig/ruby/svn/client.rb (revision 26199)
> +++ subversion/bindings/swig/ruby/svn/client.rb (working copy)
> @@ -315,10 +315,15 @@
> dry_run, options, self)
> end
>
> + def uri? path
> + path=~%r{^[^:]+\://.+}
> + end
> +

Add spaces around operator:
  path =~ %r{^[^:]+\://.+}

And use Regex#=~ not String#=~:
  %r{^[^:]+\://.+} =~ path

> def merge_peg(src, rev1, rev2, target_wcpath,
> peg_rev=nil, depth=nil,
> ignore_ancestry=false, force=false,
> dry_run=false, options=nil, record_only=false)
> + peg_rev ||= uri?(src) ? 'HEAD' : 'WORKING'

By the way, does we need to define #uri?:
  peg_rev ||= /\A[^:]+:\/\//.match(src) ? 'HEAD' : 'WORKING'

At least, should we make #uri? private?

Or should we define #default_peg_revision not #uri??

Thanks,

--
kou
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org