You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Joe Swatosh <jo...@gmail.com> on 2007/08/11 04:19:07 UTC
Re: svn commit: r26006 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn
Hi kou,
On 8/8/07, joeswatosh@tigris.org <jo...@tigris.org> wrote:
> Author: joeswatosh
> Date: Wed Aug 8 19:51:31 2007
> New Revision: 26006
>
> Log:
> Added support for the svn_wc_conflict_resolver_func_t conflict_func, and
> void *conflict_baton arguments added to svn_wc_get_update_editor3.
>
> * subversion\bindings\swig\include\svn_types.swg new in and argout typemaps
> for svn_wc_conflict_resolver_func_t.
>
> * subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.h
> (svn_swig_rb_conflict_resolver_func) new function
>
> * subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c
> (svn_swig_rb_conflict_resolver_func) new function
>
> * subversion\bindings\swig\ruby\svn\wc.rb (Svn::Wc::AdmAccess#update_editor)
> added new conflict_func argument, defaulted to nil.
>
>
> Modified:
> trunk/subversion/bindings/swig/include/svn_types.swg
> trunk/subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c
> trunk/subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.h
> trunk/subversion/bindings/swig/ruby/svn/wc.rb
>
> Modified: trunk/subversion/bindings/swig/include/svn_types.swg
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/bindings/swig/include/svn_types.swg?pathrev=26006&r1=26005&r2=26006
> ==============================================================================
> --- trunk/subversion/bindings/swig/include/svn_types.swg (original)
> +++ trunk/subversion/bindings/swig/include/svn_types.swg Wed Aug 8 19:51:31 2007
> @@ -801,6 +801,18 @@
> };
> #endif
>
> +#ifdef SWIGRUBY
> +%typemap(in) (svn_wc_conflict_resolver_func_t conflict_func, void *conflict_baton)
> +{
> + $1 = svn_swig_rb_conflict_resolver_func;
> + $2 = (void *)svn_swig_rb_make_baton($input, _global_svn_swig_rb_pool);
> +}
> +
> +%typemap(argout) (svn_wc_conflict_resolver_func_t conflict_func, void *conflict_baton)
> +{
> + svn_swig_rb_set_baton($result, (VALUE)$2);
> +};
> +#endif
>
> /* -----------------------------------------------------------------------
> Callback: svn_info_receiver_t
>
> Modified: trunk/subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c?pathrev=26006&r1=26005&r2=26006
> ==============================================================================
> --- trunk/subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c (original)
> +++ trunk/subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c Wed Aug 8 19:51:31 2007
> @@ -2150,6 +2150,34 @@
> }
>
> svn_error_t *
> +svn_swig_rb_conflict_resolver_func(svn_wc_conflict_result_t *result,
> + const svn_wc_conflict_description_t *description,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + svn_error_t *err = SVN_NO_ERROR;
> + VALUE proc, rb_pool, rb_result;
> +
> + *result = svn_wc_conflict_result_conflicted;
> +
> + svn_swig_rb_from_baton((VALUE)baton, &proc, &rb_pool);
> +
> + if (!NIL_P(proc)) {
> + callback_baton_t cbb;
> +
> + cbb.receiver = proc;
> + cbb.message = id_call;
> + cbb.args = rb_ary_new3(1,
> + c2r_swig_type((void *)description,
> + (void *)"const svn_wc_conflict_description_t *"));
> + rb_result = invoke_callback_handle_error((VALUE)(&cbb), rb_pool, &err);
> + r2c_swig_type2(rb_result, "svn_wc_conflict_result_t *", &result);
> + }
> +
> + return err;
> +}
> +
> +svn_error_t *
> svn_swig_rb_commit_callback(svn_revnum_t new_revision,
> const char *date,
> const char *author,
>
> Modified: trunk/subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.h?pathrev=26006&r1=26005&r2=26006
> ==============================================================================
> --- trunk/subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.h (original)
> +++ trunk/subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.h Wed Aug 8 19:51:31 2007
> @@ -214,6 +214,13 @@
> apr_pool_t *pool);
>
> SVN_RB_SWIG_SWIGUTIL_EXPORT
> +svn_error_t * svn_swig_rb_conflict_resolver_func(
> + svn_wc_conflict_result_t *result,
> + const svn_wc_conflict_description_t *description,
> + void *baton,
> + apr_pool_t *pool);
> +
> +SVN_RB_SWIG_SWIGUTIL_EXPORT
> svn_error_t *svn_swig_rb_commit_callback(svn_revnum_t new_revision,
> const char *date,
> const char *author,
>
> Modified: trunk/subversion/bindings/swig/ruby/svn/wc.rb
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/bindings/swig/ruby/svn/wc.rb?pathrev=26006&r1=26005&r2=26006
> ==============================================================================
> --- trunk/subversion/bindings/swig/ruby/svn/wc.rb (original)
> +++ trunk/subversion/bindings/swig/ruby/svn/wc.rb Wed Aug 8 19:51:31 2007
> @@ -298,13 +298,13 @@
> def update_editor(target_revision, target, use_commit_times=true,
> depth=nil, allow_unver_obstruction=false, diff3_cmd=nil,
> notify_func=nil, cancel_func=nil, traversal_info=nil,
> - preserved_exts=nil)
> + preserved_exts=nil, conflict_func=nil)
> preserved_exts ||= []
> traversal_info ||= _traversal_info
> results = Wc.get_update_editor3(target_revision, self, target,
> use_commit_times, depth,
> allow_unver_obstruction,
> - notify_func, cancel_func, diff3_cmd,
> + notify_func, cancel_func, conflict_func, diff3_cmd,
> preserved_exts, traversal_info)
> target_revision_address, editor, editor_baton = results
> editor.__send__(:target_revision_address=, target_revision_address)
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
>
>
David has already fixed my poorly formatted log message, so that part
is better.
However, I'm still unhappy with the svn_swig_rb_conflict_resolver_func
function itself. It feels like it should return the
svn_wc_conflict_result_t, but I wasn't sure how to make that happen.
Not to mention that there is no test for any of this (I've no idea how
I could tie this together).
Now that you've let me get started complaining, I'm not nuts about all
the default arguments to Svn::Wc::AdmAccess#update_editor and
switch_editor. Is it time to start considering a hash? How about (DO
NOT COMMIT!):
Index: subversion/bindings/swig/ruby/svn/wc.rb
===================================================================
--- subversion/bindings/swig/ruby/svn/wc.rb (revision 26006)
+++ subversion/bindings/swig/ruby/svn/wc.rb (working copy)
@@ -295,17 +295,28 @@
Wc.is_wc_root(path, self)
end
- def update_editor(target_revision, target, use_commit_times=true,
- depth=nil, allow_unver_obstruction=false,
diff3_cmd=nil,
- notify_func=nil, cancel_func=nil, traversal_info=nil,
- preserved_exts=nil, conflict_func=nil)
- preserved_exts ||= []
- traversal_info ||= _traversal_info
+ def update_editor(target_revision, target, options={})
+ options = {
+ :use_commit_times => true,
+ :depth => nil,
+ :allow_unver_obstruction => false,
+ :notify_func => nil,
+ :cancel_func => nil,
+ :conflict_func => nil,
+ :diff3_cmd => nil,
+ :preserved_exts => [],
+ :traversal_info => _traversal_info,
+ }.merge options
results = Wc.get_update_editor3(target_revision, self, target,
- use_commit_times, depth,
- allow_unver_obstruction,
- notify_func, cancel_func,
conflict_func, diff3_cmd,
- preserved_exts, traversal_info)
+ options[:use_commit_times],
+ options[:depth],
+ options[:allow_unver_obstruction],
+ options[:notify_func],
+ options[:cancel_func],
+ options[:conflict_func],
+ options[:diff3_cmd],
+ options[:preserved_exts],
+ options[:traversal_info])
target_revision_address, editor, editor_baton = results
editor.__send__(:target_revision_address=, target_revision_address)
editor.baton = editor_baton
???
It looks like we've broken compatibility with 1.4.x for that function
already. That may have to be a different discussion.
Sorry to unload all at once, thanks for your consideration.
--
Joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26006 - in trunk/subversion/bindings/swig:
include ruby/libsvn_swig_ruby ruby/svn
Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi Joe,
I agree with your plan. :)
In <ae...@mail.gmail.com>
"Re: svn commit: r26006 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn" on Sat, 11 Aug 2007 16:34:13 -0700,
"Joe Swatosh" <jo...@gmail.com> wrote:
> > What I am planning to do:
> >
> > 1) Finish getting the tests to pass. The client test_merge and
> > test_merge_peg are still broken (I think there have been at least
> > three revisions that have contributed to their brokenness). I have
> > them passing locally, but I'm going to spend some time trolling the
> > history to see if I can figure out exactly what revisions did the
> > breaking.
> >
> > 2) Change all the merge_info to mergeinfo and get rid of the aliases.
> >
>
> 2.5 New test for svn_swig_rb_conflict_resolver_func along with tidying it up.
>
> > 3) Revert update_editor and switch_editor changes and add
> > update_editor2 and switch_editor2 with the hash options as we discuss
> > above.
> >
> > 4) More diffs with 1.4.x to see if I can spot any other api regressions.
> >
> > 5) Start adding more options hashes (should we come up with a list of
> > methods with many optional arguments to prioritize?).
It isn't needed. I think all of functions that has
svn_XXX{2,3,...}() functions are target. Their function
signature will be changed. We will hide the changes by hash
options. (Yes, 5) needs much works...)
Thanks,
--
kou
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26006 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn
Posted by Joe Swatosh <jo...@gmail.com>.
Hi again
On 8/11/07, Joe Swatosh <jo...@gmail.com> wrote:
>
Oops
> What I am planning to do:
>
> 1) Finish getting the tests to pass. The client test_merge and
> test_merge_peg are still broken (I think there have been at least
> three revisions that have contributed to their brokenness). I have
> them passing locally, but I'm going to spend some time trolling the
> history to see if I can figure out exactly what revisions did the
> breaking.
>
> 2) Change all the merge_info to mergeinfo and get rid of the aliases.
>
2.5 New test for svn_swig_rb_conflict_resolver_func along with tidying it up.
> 3) Revert update_editor and switch_editor changes and add
> update_editor2 and switch_editor2 with the hash options as we discuss
> above.
>
> 4) More diffs with 1.4.x to see if I can spot any other api regressions.
>
> 5) Start adding more options hashes (should we come up with a list of
> methods with many optional arguments to prioritize?).
>
> --
> Joe
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26006 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn
Posted by Joe Swatosh <jo...@gmail.com>.
Hi kou
On 8/10/07, Kouhei Sutou <ko...@cozmixng.org> wrote:
> Hi,
>
> > changed the apis. I just did a diff between the 1.4.x branch and
> > trunk and I think update_editor has a new required argument (perhaps
> > just a rename, though?).
>
> Ah, it's not good... Sorry.
> We should define update_editor2 for Wc.get_update_editor3
> and keep update_editor API. Could you fix my faults?
I'll look at it later. See below.
>
> > > Of cause we should validate arguments of hash argument
> > > version methods (xxx_yyy2). We'll need to use the following
> > > method:
> > >
> > > class Hash
> > > def assert_valid_keys(*valid_keys)
> > > unknown_keys = keys - [valid_keys].flatten
> > > unless unknown_keys.empty?
> > > raise(ArgumentError, "Unknown key(s): #{unknown_keys.join(", ")}")
> > > end
> > > end
> > > end
> > >
> > > # The above method is quoted from ActiveSupport.
> > >
> >
> > I'm not familiar with this. Is it just to ensure that there were no
> > typos in the passed in options?
>
> Yes.
>
> > I was thinking it might be nice just
> > to ignore unrecognized options. I haven't given it a lot of thought,
> > but it seemed like clients might be able to create their own options
> > hashes, and their might be room for reuse if we ignored unrecognized
> > keys.
>
> In my experience, there aren't many cases to reuse option
> hash but there are many typos in scripts. :<
> I think a case that we trust our script's logic doesn't have
> any problems but the script doesn't work is too costly (we
> will use much time and be damaged our mind). And most of
> causes of the case are caused by a typo!
>
> So I want to validate hash options. We can remove validation
> after many people request.
>
That is reasonable. Obviously, I hadn't tried making such a hash yet
so I will accept your judgment that it won't help. Anyway, being so
explicit will probably make it easier to document and understand which
options are valid for each function.
What I am planning to do:
1) Finish getting the tests to pass. The client test_merge and
test_merge_peg are still broken (I think there have been at least
three revisions that have contributed to their brokenness). I have
them passing locally, but I'm going to spend some time trolling the
history to see if I can figure out exactly what revisions did the
breaking.
2) Change all the merge_info to mergeinfo and get rid of the aliases.
3) Revert update_editor and switch_editor changes and add
update_editor2 and switch_editor2 with the hash options as we discuss
above.
4) More diffs with 1.4.x to see if I can spot any other api regressions.
5) Start adding more options hashes (should we come up with a list of
methods with many optional arguments to prioritize?).
--
Joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26006 - in trunk/subversion/bindings/swig:
include ruby/libsvn_swig_ruby ruby/svn
Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi,
> changed the apis. I just did a diff between the 1.4.x branch and
> trunk and I think update_editor has a new required argument (perhaps
> just a rename, though?).
Ah, it's not good... Sorry.
We should define update_editor2 for Wc.get_update_editor3
and keep update_editor API. Could you fix my faults?
> > Of cause we should validate arguments of hash argument
> > version methods (xxx_yyy2). We'll need to use the following
> > method:
> >
> > class Hash
> > def assert_valid_keys(*valid_keys)
> > unknown_keys = keys - [valid_keys].flatten
> > unless unknown_keys.empty?
> > raise(ArgumentError, "Unknown key(s): #{unknown_keys.join(", ")}")
> > end
> > end
> > end
> >
> > # The above method is quoted from ActiveSupport.
> >
>
> I'm not familiar with this. Is it just to ensure that there were no
> typos in the passed in options?
Yes.
> I was thinking it might be nice just
> to ignore unrecognized options. I haven't given it a lot of thought,
> but it seemed like clients might be able to create their own options
> hashes, and their might be room for reuse if we ignored unrecognized
> keys.
In my experience, there aren't many cases to reuse option
hash but there are many typos in scripts. :<
I think a case that we trust our script's logic doesn't have
any problems but the script doesn't work is too costly (we
will use much time and be damaged our mind). And most of
causes of the case are caused by a typo!
So I want to validate hash options. We can remove validation
after many people request.
Thanks,
--
kou
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26006 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn
Posted by Joe Swatosh <jo...@gmail.com>.
Hi kou
On 8/10/07, Kouhei Sutou <ko...@cozmixng.org> wrote:
> Hi,
>
> In <ae...@mail.gmail.com>
> "Re: svn commit: r26006 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn" on Fri, 10 Aug 2007 21:19:07 -0700,
> "Joe Swatosh" <jo...@gmail.com> wrote:
>
> > However, I'm still unhappy with the svn_swig_rb_conflict_resolver_func
> > function itself. It feels like it should return the
> > svn_wc_conflict_result_t, but I wasn't sure how to make that happen.
> > Not to mention that there is no test for any of this (I've no idea how
> > I could tie this together).
>
> I don't know about the recent API but test_resolved in
> test_client.rb will help you. Because the test causes
> conflict on purpose.
I'll peek at that to see if I can come up with anything.
>
> > Now that you've let me get started complaining, I'm not nuts about all
> > the default arguments to Svn::Wc::AdmAccess#update_editor and
> > switch_editor. Is it time to start considering a hash? How about (DO
> > NOT COMMIT!):
>
> > It looks like we've broken compatibility with 1.4.x for that function
> > already. That may have to be a different discussion.
>
> I think it's not good to break API compatibility for now. I
> think the time is 2.0.0. In my thought, we define xxx_yyy2
> methods for hash argument version. And xxx_yyy methods still
> use the current style. How about this idea?
That seems reasonable. If I can scare up the time, I'll have a try at
that. At some point we'll have to check to ensure that we haven't
changed the apis. I just did a diff between the 1.4.x branch and
trunk and I think update_editor has a new required argument (perhaps
just a rename, though?).
>
> Of cause we should validate arguments of hash argument
> version methods (xxx_yyy2). We'll need to use the following
> method:
>
> class Hash
> def assert_valid_keys(*valid_keys)
> unknown_keys = keys - [valid_keys].flatten
> unless unknown_keys.empty?
> raise(ArgumentError, "Unknown key(s): #{unknown_keys.join(", ")}")
> end
> end
> end
>
> # The above method is quoted from ActiveSupport.
>
I'm not familiar with this. Is it just to ensure that there were no
typos in the passed in options? I was thinking it might be nice just
to ignore unrecognized options. I haven't given it a lot of thought,
but it seemed like clients might be able to create their own options
hashes, and their might be room for reuse if we ignored unrecognized
keys.
Thanks for listening,
--
Joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26006 - in trunk/subversion/bindings/swig:
include ruby/libsvn_swig_ruby ruby/svn
Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi,
In <ae...@mail.gmail.com>
"Re: svn commit: r26006 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn" on Fri, 10 Aug 2007 21:19:07 -0700,
"Joe Swatosh" <jo...@gmail.com> wrote:
> However, I'm still unhappy with the svn_swig_rb_conflict_resolver_func
> function itself. It feels like it should return the
> svn_wc_conflict_result_t, but I wasn't sure how to make that happen.
> Not to mention that there is no test for any of this (I've no idea how
> I could tie this together).
I don't know about the recent API but test_resolved in
test_client.rb will help you. Because the test causes
conflict on purpose.
> Now that you've let me get started complaining, I'm not nuts about all
> the default arguments to Svn::Wc::AdmAccess#update_editor and
> switch_editor. Is it time to start considering a hash? How about (DO
> NOT COMMIT!):
> It looks like we've broken compatibility with 1.4.x for that function
> already. That may have to be a different discussion.
I think it's not good to break API compatibility for now. I
think the time is 2.0.0. In my thought, we define xxx_yyy2
methods for hash argument version. And xxx_yyy methods still
use the current style. How about this idea?
Of cause we should validate arguments of hash argument
version methods (xxx_yyy2). We'll need to use the following
method:
class Hash
def assert_valid_keys(*valid_keys)
unknown_keys = keys - [valid_keys].flatten
unless unknown_keys.empty?
raise(ArgumentError, "Unknown key(s): #{unknown_keys.join(", ")}")
end
end
end
# The above method is quoted from ActiveSupport.
Thanks,
--
kou
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org