Hi,
In <ae6cb1100701160751n33d23cb8p4f61ea2e0b94a7bc@mail.gmail.com>
"Re: Ruby binding test of fs failing on windows" on Tue, 16 Jan 2007 07:51:00 -0800,
"Joe Swatosh" <joe.swatosh@gmail.com> wrote:
> > > Interface changes:
> > > - Add a close method to Svn::Fs (that would release the new pool
> > > member added to Svn::Fs which would close the filesystem).
> > > - Add a block form to Svn::Fs.open, Svn::Fs.new, and Svn::Fs.create
> > > (with the obvious implementations).
> > >
> > > Implementation changes:
> > > Here I'm kind of fuzzy. I think that we'd have to add a pool member
> > > to Svn::Fs (initialized as a "subpool" of the class variable pool?)
> > > and pass it into Svn::Ext::Fs.create or Svn::Ext::Fs.open, which would
> > > mean modifying their signatures and implementations to use it instead
> > > of the global.
> >
> > It seems that a block form solution is good. But I can't
> > implement this because I don't want to expose pool related
> > API to Ruby side. I just removed
> > Svn::Fs::FileSystem.delete. Could you give me a time to
> > think about the implementation?
>
> Of course. I was thinking that the pool stuff would have leak into
> the implementation of Svn::Fs since pools would have to be passed to
> Svn::Ext::Fs, but that the interface of Svn::Fs would remain unchanged
> (except for adding close, and possibly the block versions).
> Essentially, my thought is we could remove the @@fs_pool and let each
> instance have its own @fs_pool. Then couldn't we also remove the
> global that is initialized with the @@fs_pool. (It doesn't *sound*
> that hard, but the SWIG stuff is daunting).
>
> I think not exposing the pool related stuff is good. I'm wondering
> where you're drawing the line Svn::Fs (where I'm suggesting) or
> Svn::Ext::Fs (where it seems to be now).
>
> Getting rid of Svn::Fs::FileSystem.delete might work, but since the
> files in the directory are still being held open, even a
> FileUtils.rm_rf might fail in this scenario on windows. IOW I'm not
> sure that we don't have a deeper issue.
I attach a patch that shows my thought. The patch isn't
complete. We should add NULL check to detect invalid
Svn::Ext::Fs object to each _wrap_svn_fs_* function by SWIG.
I implemented your idea with my way using SWIG. What about
my idea?
Thanks,
--
kou
Index: subversion/bindings/swig/core.i
===================================================================
--- subversion/bindings/swig/core.i (revision 23066)
+++ subversion/bindings/swig/core.i (working copy)
@@ -697,6 +697,10 @@
apr_pool_wrapper_destroy(self);
xfree(self);
}
+
+ void destroy(void) {
+ apr_pool_wrapper_destroy(self);
+ }
};
%ignore apr_pool_wrapper_destroy;
Index: subversion/bindings/swig/ruby/test/test_fs.rb
===================================================================
--- subversion/bindings/swig/ruby/test/test_fs.rb (revision 23066)
+++ subversion/bindings/swig/ruby/test/test_fs.rb (working copy)
@@ -25,18 +25,22 @@
def test_create
path = File.join(@tmp_path, "fs")
- fs_type = Svn::Fs::TYPE_BDB
+ fs_type = Svn::Fs::TYPE_FSFS
config = {Svn::Fs::CONFIG_FS_TYPE => fs_type}
assert(!File.exist?(path))
- fs = Svn::Fs::FileSystem.create(path, config)
- assert(File.exist?(path))
- assert_equal(fs_type, Svn::Fs.type(path))
- fs.set_warning_func do |err|
- p err
- abort
+ Svn::Fs::FileSystem.create(path, config) do |fs|
+ assert(File.exist?(path))
+ assert_equal(fs_type, Svn::Fs.type(path))
+ fs.set_warning_func do |err|
+ p err
+ abort
+ end
+ assert_equal(path, fs.path)
end
- assert_equal(path, fs.path)
+
+ Svn::Fs::FileSystem.delete(path)
+ assert(!File.exist?(path))
end
def test_hotcopy
Index: subversion/bindings/swig/ruby/test/test_repos.rb
===================================================================
--- subversion/bindings/swig/ruby/test/test_repos.rb (revision 23066)
+++ subversion/bindings/swig/ruby/test/test_repos.rb (working copy)
@@ -76,13 +76,15 @@
def test_create
tmp_repos_path = File.join(@tmp_path, "repos")
- fs_config = {Svn::Fs::CONFIG_FS_TYPE => Svn::Fs::TYPE_BDB}
- repos = Svn::Repos.create(tmp_repos_path, {}, fs_config)
- assert(File.exist?(tmp_repos_path))
- fs_type_path = File.join(repos.fs.path, Svn::Fs::CONFIG_FS_TYPE)
- assert_equal(Svn::Fs::TYPE_BDB,
- File.open(fs_type_path) {|f| f.read.chop})
- repos.fs.set_warning_func(&warning_func)
+ fs_type = Svn::Fs::TYPE_FSFS
+ fs_config = {Svn::Fs::CONFIG_FS_TYPE => fs_type}
+ Svn::Repos.create(tmp_repos_path, {}, fs_config) do |repos|
+ assert(File.exist?(tmp_repos_path))
+ fs_type_path = File.join(repos.fs.path, Svn::Fs::CONFIG_FS_TYPE)
+ assert_equal(fs_type, File.open(fs_type_path) {|f| f.read.chop})
+ repos.fs.set_warning_func(&warning_func)
+ end
+
Svn::Repos.delete(tmp_repos_path)
assert(!File.exist?(tmp_repos_path))
end
Index: subversion/bindings/swig/ruby/svn/util.rb
===================================================================
--- subversion/bindings/swig/ruby/svn/util.rb (revision 23066)
+++ subversion/bindings/swig/ruby/svn/util.rb (working copy)
@@ -7,8 +7,6 @@
module Svn
module Util
- @@wrapper_procs = []
-
module_function
def to_ruby_class_name(name)
name.split("_").collect do |x|
@@ -63,12 +61,12 @@
target_name = meth
end
unless target_name.nil?
- target_id = target_name.intern
- target_method = ext_mod.method(meth)
- target_proc = Proc.new{|*args| target_method.call(*args)}
- target_mod.funcall(:define_method, target_id, target_proc)
- target_mod.funcall(:module_function, target_id)
- @@wrapper_procs << target_proc
+ target_mod.module_eval(<<-EOC, __FILE__, __LINE__ + 1)
+ def #{target_name}(*args, &block)
+ #{ext_mod.name}.#{meth}(*args, &block)
+ end
+ module_function :#{target_name}
+EOC
end
end
end
Index: subversion/bindings/swig/ruby/svn/fs.rb
===================================================================
--- subversion/bindings/swig/ruby/svn/fs.rb (revision 23066)
+++ subversion/bindings/swig/ruby/svn/fs.rb (working copy)
@@ -23,16 +23,16 @@
class FileSystem
class << self
- def create(path, config={})
- Fs.create(path, config)
+ def create(path, config={}, &block)
+ Fs.create(path, config, &block)
end
def delete(path)
Fs.delete_fs(path)
end
- def open(path, config={})
- Fs.open(path, config)
+ def open(path, config={}, &block)
+ Fs.open(path, config, &block)
end
alias new open
Index: subversion/bindings/swig/ruby/svn/repos.rb
===================================================================
--- subversion/bindings/swig/ruby/svn/repos.rb (revision 23066)
+++ subversion/bindings/swig/ruby/svn/repos.rb (working copy)
@@ -12,8 +12,7 @@
Util.set_methods(Ext::Repos, self)
- @@alias_targets = %w(create open hotcopy recover
- db_logfiles)
+ @@alias_targets = %w(create hotcopy recover db_logfiles)
class << self
@@alias_targets.each do |target|
alias_method "_#{target}", target
@@ -25,17 +24,8 @@
@@alias_targets = nil
module_function
- def open(path)
- repos = _open(path)
- if block_given?
- yield repos
- else
- repos
- end
- end
-
- def create(path, config={}, fs_config={})
- _create(path, nil, nil, config, fs_config)
+ def create(path, config={}, fs_config={}, &block)
+ _create(path, nil, nil, config, fs_config, &block)
end
def hotcopy(src, dest, clean_logs=true)
Index: subversion/bindings/swig/include/svn_types.swg
===================================================================
--- subversion/bindings/swig/include/svn_types.swg (revision 23066)
+++ subversion/bindings/swig/include/svn_types.swg (working copy)
@@ -75,6 +75,20 @@
}
#endif
+#ifdef SWIGRUBY
+%typemap(argout) SWIGTYPE **OUTPARAM_WITH_BLOCK {
+ VALUE tmp;
+ tmp = SWIG_NewPointerObj(*$1, $*1_descriptor, 0);
+ if (rb_block_given_p()) {
+ rb_yield(tmp);
+ rb_funcall(_global_svn_swig_rb_pool, rb_intern("destroy"), 0);
+ DATA_PTR(tmp) = NULL;
+ } else {
+ %append_output(tmp);
+ }
+}
+#endif
+
%apply SWIGTYPE **OUTPARAM {
/* apr */
apr_file_t **,
@@ -151,6 +165,14 @@
void **set_locks_baton
};
+%apply SWIGTYPE **OUTPARAM_WITH_BLOCK {
+ /* svn_fs */
+ svn_fs_t **,
+ /* svn_repos */
+ svn_repos_t **
+};
+
+
/* -----------------------------------------------------------------------
%apply-ing of typemaps
*/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jan 17 16:25:54 2007