Re: [PATCH] [RESEND] add support for svn_client_info to perl bindings

On 3/6/06, Garrett Rooney <rooneg@electricjellyfish.net> wrote:
> On 3/5/06, Al Tobey <tobert@gmail.com> wrote:
> > The attached patch adds support for svn_client_info to the perl
> > bindings. I added a test for working on a WC since that's all I'm
> > using currently, but it should work OK for URL's, too.
> >
> > #!/usr/bin/perl
> > my $svn = SVN::Client->new();
> > my $receiver = sub {
> > # $_[1] is an svn_info_t object
> > print "Current revision of $_[0] is", $_[1]->rev, "\n";
> > };
> > $svn->info( "foo/bar.c", undef, 'WORKING', $receiver, 0 );
>
>
> This looks good, other than some sketchy code in
> svn_swig_pl_info_receiver and the lack of any log message at all.
>
> Your current implementation of svn_swig_pl_info_receiver looks like this:
>
> /* Thunked version of svn_client_info_t callback type. */
> svn_error_t *svn_swig_pl_info_receiver(void *baton,
> const char *path,
> const svn_info_t *info,
> apr_pool_t *pool)
> {
> svn_error_t *err = SVN_NO_ERROR;
> swig_type_info *infoinfo = _SWIG_TYPE("svn_info_t *");
>
> if (!SvOK((SV *)baton)) {
> return ;
> }
>
> svn_swig_pl_callback_thunk(CALL_SV, baton, NULL, "sSS", path, info,
> infoinfo, pool, POOLINFO);
> }
>
> So you declare an error variable, but never use it. Worse, the
> function returns an svn_error_t *, but in the error case you return
> nothing at all and at the end of the function you don't return
> success. I'm not all that clear on the right way to write these
> functions in the bindings, but that seems wrong to me.
>
> As far as a log message, http://subversion.tigris.org/hacking.html
> should have all the details you need on how to write one of those.
>
> -garrett
>

You're absolutely right. Once I got it working and did the svn diff,
I had to move on to other stuff (which is a lame excuse) and didn't
fill in the error handling. I've attached a cleaned up version that
matches the rest of the binding code. The complete path of errors
through swig to perl land appears to me to be inconsisent, so I just
stuck with doing it just like similar functions such as
svn_client_blame()/svn_swig_pl_blame_func().

Thanks,
-Al Tobey

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