You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by John Beranek <jo...@redux.org.uk> on 2009/06/16 12:21:09 UTC
SWIG (Perl) prospective patch
Hello all,
So I've just started using the Subversion API, from Perl. Currently have
Subversion 1.6.1 rebuilt from a source RPM on my Fedora 10 development
system.
Was looking into examining transactions in a pre-commit hook, using the
Perl Subversion API.
Got some way down the road and got to _p_svn_fs_root_t->paths_changed()
(aka svn_fs_paths_changed() in the C API).
Started reading the C API and discovered svn_fs_paths_changed2() which
returns the seemingly valuable extra info that svn_fs_path_change2_t was
created to add in Subversion 1.6.
So, wanted to call this from Perl, only to find the Perl bindings didn't
implement that call.
After much ado the attached patch is my attempt at adding said call to
the SWIG bindings for Perl. There are a few provisos/apologies:
* This is a patch against the Subversion 1.6.1 code from the SRPM I
started from, not trunk or any Subversion branch.
* I'm not convinced of the change to svn_fs.h and svn_fs.i, whether this
is the way to achieve what I've done.
* I've not yet written complete Perldoc for my additions to SVN::Fs.
* I've tested the additions and ->copyfrom_path() and ->copyfrom_rev()
and they return what's expected if the change is indeed a copy. If the
change isn't a copy ->copyfrom_known() still appears to be true, and
->copyfrom_path()/->copyfrom_rev() just return empty. Is this as
expected, the C API docs are far from clear here?
Right, I hope this is helpful to someone...
Cheers,
John.
--
John Beranek To generalise is to be an idiot.
http://redux.org.uk/ -- William Blake
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2362435
Re: parameter name of svn_fs_paths_changed2() -- was: Re: SWIG
(Perl) prospective patch
Posted by Neels Janosch Hofmeyr <ne...@elego.de>.
C. Michael Pilato wrote:
> Stefan Sperling wrote:
>> On Tue, Jun 16, 2009 at 04:07:26PM +0200, Neels Janosch Hofmeyr wrote:
>>> That said, I see how the bindings sort of *need* it to be a different name.
>>>
>>> [[[
>>> %hash_argout_typemap(changed_paths_p, svn_fs_path_change_t *)
>>> +%hash_argout_typemap(changed_paths2_p, svn_fs_path_change2_t *)
>>> ]]]
>>>
>>> The bindings code shouldn't be like that. after all, variable names can be
>>> similar and hash tables can contain anything.
>> Somehow this doesn't make sense to me...
>> I must be missing something.
>>
>> Why do bindings depend on the name of variables, instead of the type?
>> E.g. it's legal in C to omit variable names from function prototypes
>> altogether. What would the bindings do then?
>>
>> If the type of values in a hash table changes, can't the bindings
>> deal with this by making changes internally to the function,
>> just like we do in C?
>>
>> I admit that I don't know much about how SWIG works, but relying
>> on the names of variables seems really wrong. And from the short
>> time I played with SWIG a long time ago I can't recall it having
>> such a requirement.
>
> The bindings *can* rely on the type, when the type is distinct enough. But
> it's really hard to write a function that converts an apr_hash_t * of
> arbitrary C data type keys and arbitrary C data type values into a Python
> dictionary of Python object keys and Python object values. You rather need
> to know the specifics of the data types inside the hash/dictionary. There
> are two ways to do this: make a new typedef in C for every possible flavor
> of apr_hash_t so the bindings can distinguish between them, or use
> variable-name-based hints to pull that off.
>
So you're saying, since we don't create a separate typedef for each hash
table, the bindings will never work unless we rename that parameter? In that
case, +1 for
[[[
--- subversion/include/svn_fs.h
+++ subversion/include/svn_fs.h
@@ -1135,11 +1135,11 @@
* Use @c pool for all allocations, including the hash and its values.
*
* @since New in 1.6.
*/
svn_error_t *
-svn_fs_paths_changed2(apr_hash_t **changed_paths_p,
+svn_fs_paths_changed2(apr_hash_t **changed_paths2_p,
svn_fs_root_t *root,
apr_pool_t *pool);
/** Same as svn_fs_paths_changed2(), only with @c svn_fs_path_change_t * values
]]]
~Neels
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2362676
Re: parameter name of svn_fs_paths_changed2() -- was: Re: SWIG
(Perl) prospective patch
Posted by "C. Michael Pilato" <cm...@collab.net>.
Stefan Sperling wrote:
> On Tue, Jun 16, 2009 at 04:07:26PM +0200, Neels Janosch Hofmeyr wrote:
>> That said, I see how the bindings sort of *need* it to be a different name.
>>
>> [[[
>> %hash_argout_typemap(changed_paths_p, svn_fs_path_change_t *)
>> +%hash_argout_typemap(changed_paths2_p, svn_fs_path_change2_t *)
>> ]]]
>>
>> The bindings code shouldn't be like that. after all, variable names can be
>> similar and hash tables can contain anything.
>
> Somehow this doesn't make sense to me...
> I must be missing something.
>
> Why do bindings depend on the name of variables, instead of the type?
> E.g. it's legal in C to omit variable names from function prototypes
> altogether. What would the bindings do then?
>
> If the type of values in a hash table changes, can't the bindings
> deal with this by making changes internally to the function,
> just like we do in C?
>
> I admit that I don't know much about how SWIG works, but relying
> on the names of variables seems really wrong. And from the short
> time I played with SWIG a long time ago I can't recall it having
> such a requirement.
The bindings *can* rely on the type, when the type is distinct enough. But
it's really hard to write a function that converts an apr_hash_t * of
arbitrary C data type keys and arbitrary C data type values into a Python
dictionary of Python object keys and Python object values. You rather need
to know the specifics of the data types inside the hash/dictionary. There
are two ways to do this: make a new typedef in C for every possible flavor
of apr_hash_t so the bindings can distinguish between them, or use
variable-name-based hints to pull that off.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2362486
Re: parameter name of svn_fs_paths_changed2() -- was: Re: SWIG
(Perl) prospective patch
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Jun 16, 2009 at 04:07:26PM +0200, Neels Janosch Hofmeyr wrote:
> That said, I see how the bindings sort of *need* it to be a different name.
>
> [[[
> %hash_argout_typemap(changed_paths_p, svn_fs_path_change_t *)
> +%hash_argout_typemap(changed_paths2_p, svn_fs_path_change2_t *)
> ]]]
>
> The bindings code shouldn't be like that. after all, variable names can be
> similar and hash tables can contain anything.
Somehow this doesn't make sense to me...
I must be missing something.
Why do bindings depend on the name of variables, instead of the type?
E.g. it's legal in C to omit variable names from function prototypes
altogether. What would the bindings do then?
If the type of values in a hash table changes, can't the bindings
deal with this by making changes internally to the function,
just like we do in C?
I admit that I don't know much about how SWIG works, but relying
on the names of variables seems really wrong. And from the short
time I played with SWIG a long time ago I can't recall it having
such a requirement.
Stefan
Re: parameter name of svn_fs_paths_changed2() -- was: Re: SWIG
(Perl) prospective patch
Posted by John Beranek <jo...@redux.org.uk>.
Neels J Hofmeyr wrote:
>
> BTW, John, you can create a patch comfortably by making your changes in an
> actual working copy and then running "svn diff" in the working copy root
> (usually from trunk, but using 1.6.1 like you did:
> "
> svn co https://svn.collab.net/repos/svn/tags/1.6.1
> <edit> ./1.6.1/**
> svn diff ./1.6.1 > patch.file
> "). That's the standard form normally circulating around here.
I admit that would be better, but I hadn't yet gone the whole way to
getting a copy of Subversion out of Subversion, making changes there and
building - even if that method is arguably much tidier than an SRPM
build environment.
Patch against tags/1.6.1 attached, my previous patch applied cleanly.
John.
--
John Beranek To generalise is to be an idiot.
http://redux.org.uk/ -- William Blake
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2362484
parameter name of svn_fs_paths_changed2() -- was: Re: SWIG (Perl)
prospective patch
Posted by Neels Janosch Hofmeyr <ne...@elego.de>.
John Beranek wrote:
[[[
--- subversion/include/svn_fs.h
+++ subversion/include/svn_fs.h
@@ -1135,11 +1135,11 @@
* Use @c pool for all allocations, including the hash and its values.
*
* @since New in 1.6.
*/
svn_error_t *
-svn_fs_paths_changed2(apr_hash_t **changed_paths_p,
+svn_fs_paths_changed2(apr_hash_t **changed_paths2_p,
svn_fs_root_t *root,
apr_pool_t *pool);
/** Same as svn_fs_paths_changed2(), only with @c svn_fs_path_change_t * values
]]]
I'm not sure that this is ok. I see what you mean -- the data stored in the
hash is now different. Actually, if we were to decide on a rule like this to
be put in place, I'd probably be in favor.
But AFAIK we haven't ever been doing *this* kind of API versioning.
Functionally speaking, it's a bikeshed. And since svn_fs_paths_changed2() is
already in trunk the way that it is, it seems a little silly to do this now.
That said, I see how the bindings sort of *need* it to be a different name.
[[[
%hash_argout_typemap(changed_paths_p, svn_fs_path_change_t *)
+%hash_argout_typemap(changed_paths2_p, svn_fs_path_change2_t *)
]]]
The bindings code shouldn't be like that. after all, variable names can be
similar and hash tables can contain anything.
Does anyone know how to fix the bindings without changing the actual
parameter name in svn_fs_paths_changed2()?
Alternatively, I see how the variable name change makes sense. Any vetos
from you committers out there?
~Neels
BTW, John, you can create a patch comfortably by making your changes in an
actual working copy and then running "svn diff" in the working copy root
(usually from trunk, but using 1.6.1 like you did:
"
svn co https://svn.collab.net/repos/svn/tags/1.6.1
<edit> ./1.6.1/**
svn diff ./1.6.1 > patch.file
"). That's the standard form normally circulating around here.
> Hello all,
>
> So I've just started using the Subversion API, from Perl. Currently have
> Subversion 1.6.1 rebuilt from a source RPM on my Fedora 10 development
> system.
>
> Was looking into examining transactions in a pre-commit hook, using the
> Perl Subversion API.
>
> Got some way down the road and got to _p_svn_fs_root_t->paths_changed()
> (aka svn_fs_paths_changed() in the C API).
>
> Started reading the C API and discovered svn_fs_paths_changed2() which
> returns the seemingly valuable extra info that svn_fs_path_change2_t was
> created to add in Subversion 1.6.
>
> So, wanted to call this from Perl, only to find the Perl bindings didn't
> implement that call.
>
> After much ado the attached patch is my attempt at adding said call to
> the SWIG bindings for Perl. There are a few provisos/apologies:
>
> * This is a patch against the Subversion 1.6.1 code from the SRPM I
> started from, not trunk or any Subversion branch.
> * I'm not convinced of the change to svn_fs.h and svn_fs.i, whether this
> is the way to achieve what I've done.
> * I've not yet written complete Perldoc for my additions to SVN::Fs.
> * I've tested the additions and ->copyfrom_path() and ->copyfrom_rev()
> and they return what's expected if the change is indeed a copy. If the
> change isn't a copy ->copyfrom_known() still appears to be true, and
> ->copyfrom_path()/->copyfrom_rev() just return empty. Is this as
> expected, the C API docs are far from clear here?
>
> Right, I hope this is helpful to someone...
>
> Cheers,
>
> John.
>
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2362468