You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Küng <to...@gmail.com> on 2005/08/27 11:54:25 UTC
[PATCH] notification callbacks for network data transfers (V3)
Branko Čibej wrote:
> Stefan Küng wrote:
>
>> New patch which uses Branko's comments to fix what was broken.
>
>
> Sorry this took so long... The patch looks good, I've a few minor comments
No problem. You said that it would take you some time, so I was prepared
to wait ;)
> I think we could commit this with some minor changes, even if we only
> get progress info fo ra_dav for now. Could you update the patch to
> trunk? Thaks.
Ok, done.
I'm planning on implementing this for ra_svn too, and maybe even for
file:/// access. But it will take a little more time.
Also, I wasn't sure if such a patch would be accepted, so I only wrote
one for ra_dav to start with - if this get's in, then I can start
working on the other protocols.
[snip]
>> * subversion/libsvn_client/copy.c
>> (repos_to_repos_copy): correct comment
>> ]]]
>>
>>
> Just a comment for the future: please use whole sentences in the log
> message; for example,
>
> * subversion/libsvn_client/copy.c
> (repos_to_repos_copy): Correct comment.
Ok. Done.
> You also put lots of tabs in the code where there weren't any before.
I think I got it right this time. Changed my editor to show spaces and
tabs differently so I can see where I messed up.
> The progress_baton field needs a docstring, too. Same in
> svn_ra_callbacks2_t.
Done.
>> +static void
>> +svn_ra_dav__neonprogress(void * baton, off_t progress, off_t total)
>> +{
>> + const svn_ra_callbacks_t * callbacks = (svn_ra_callbacks_t *)baton;
>> + if (callbacks->progress_func)
>> + {
>> + callbacks->progress_func(progress, total,
>> callbacks->progress_baton);
>> + }
>> +}
>>
>>
> This function's name shouldn't actually have a svn_ra_dav__ prefix --
> it's a static function, not library-private.
Changed it to ra_dav_neonprogress().
Patch attached.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
Re: [PATCH] notification callbacks for network data transfers (V3)
Posted by Branko Čibej <br...@xbc.nu>.
Stefan Küng wrote:
> Branko Čibej wrote:
>
>> Stefan Küng wrote:
>
>>> * subversion/libsvn_client/copy.c
>>> (repos_to_repos_copy): correct comment
>>> ]]]
>>>
>>>
>> Just a comment for the future: please use whole sentences in the log
>> message; for example,
>>
>> * subversion/libsvn_client/copy.c
>> (repos_to_repos_copy): Correct comment.
>
>
> Ok. Done.
Hah. No it wasn't. Sentences start with a capital letter, and end with a
period.
No matter, I'm tweaking this patch now and will commit after running the
tests.
Thanks for the patch!
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] notification callbacks for network data transfers (V3)
Posted by Branko Čibej <br...@xbc.nu>.
Branko Čibej wrote:
> Hey Stefan, did you actually compile this patch?
>
> subversion\libsvn_ra\ra_loader.c(278) : warning C4133: 'function' :
> incompatible types - from 'const struct svn_ra_callbacks2_t *' to
> 'const struct svn_ra_callbacks_t *'
> subversion\libsvn_ra\ra_loader.c(304) : warning C4716: 'svn_ra_open' :
> must return a value
>
> I can certainly fix this myself, but next time, please make sure you
> don't leave thinks ilke this in the code.
subversion\libsvn_ra_dav\session.c(806) : warning C4090: 'function' : different 'const' qualifiers
subversion\libsvn_ra_dav\session.c(806) : warning C4022: 'ne_set_progress' : pointer mismatch for actual parameter 3
subversion\libsvn_ra_dav\session.c(807) : warning C4090: 'function' : different 'const' qualifiers
subversion\libsvn_ra_dav\session.c(807) : warning C4022: 'ne_set_progress' : pointer mismatch for actual parameter 3
Together with the fact that fixing the type of ra_vtable->open to accept
svn_ra_callbacks2_t instead of the old, deprecated type, this has become
more than I'm willing to "tweak". Here's the updated patch and log
message, the result of my tweakings. You'll have to finish this yourself.
One of the things I did is to remove the svn_progress.h header. It's not
needed, the progress callback typedef belongs in svn_ra.h.
-- Brane
Re: [PATCH] notification callbacks for network data transfers (V6)
Posted by Stefan Küng <to...@gmail.com>.
Branko Čibej wrote:
> O.K. I changed the log message a bit (not all changes were mentioned).
Sorry about that. I thought I had it all covered, but I saw in your
commit that I missed some.
Thanks for committing btw!
> Other tweaks noted below.
>
> Committed in r15948. Thanks!
Thanks for committing!
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] notification callbacks for network data transfers (V6)
Posted by Branko Čibej <br...@xbc.nu>.
Stefan Küng wrote:
> Branko Čibej wrote:
>
>> You don't have to rev the svn_ra__vtable_t::open callback. The
>> svn_ra__vtable_t type is private to libsvn_ra*, so you can change the
>> signature of the open() callback instead of revving it. It's not a
>> public API. :)
>>
>> Otherwise, it looks fine. Please change the svn_ra__vtable_t thing
>> and update the log message, and I'll test and commit.
>
>
> Patch attached.
O.K. I changed the log message a bit (not all changes were mentioned).
Other tweaks noted below.
Committed in r15948. Thanks!
>Index: subversion/include/svn_client.h
>===================================================================
>--- subversion/include/svn_client.h (Revision 15946)
>+++ subversion/include/svn_client.h (Arbeitskopie)
>@@ -40,6 +40,7 @@
> #include "svn_error.h"
> #include "svn_opt.h"
> #include "svn_version.h"
>+#include "svn_ra.h"
>
>
> #ifdef __cplusplus
>@@ -533,6 +534,14 @@
> /** callback baton for log_msg_func2
> * @since New in 1.3. */
> void *log_msg_baton2;
>+
>+ /** Notification callback for network progress information.
>+ * @since New in 1.3. */
>+ svn_ra_progress_notify_func_t progress_func;
>
>
In the docstring, i added "May be NULL if not used.", since that's an
important part of the API semantics. Same for svn_ra_callbacks2_t.
>Index: subversion/include/svn_ra.h
>===================================================================
>--- subversion/include/svn_ra.h (Revision 15946)
>+++ subversion/include/svn_ra.h (Arbeitskopie)
>@@ -169,6 +169,22 @@
> svn_error_t *ra_err,
> apr_pool_t *pool);
>
>+/**
>+ * Callback function type for progress notification.
>+ *
>+ * @a progress is the amount of bytes already transferred.
>+ *
>+ * @a total is the total amount of bytes to transfer or -1 if it's not
>+ * known.
>+ *
>+ * @a baton is the callback baton. If not used, must be NULL.
>+ *
>+ * @since New in 1.3.
>+ */
>+typedef void (*svn_ra_progress_notify_func_t) (apr_off_t progress,
>+ apr_off_t total,
>+ void * baton);
>
>
I tweaked this docstring a bit, but most imporantly, I removed the part
about BATON having to be NULL if it's not used. You can't impose
constraints on how a particular client will implement its callback
functions. Since BATON isn't interpreted by Subversion's code but only
passed back to the client, its value is irrelevant as far as our API is
concerned.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] notification callbacks for network data transfers (V6)
Posted by Stefan Küng <to...@gmail.com>.
Branko Čibej wrote:
> You don't have to rev the svn_ra__vtable_t::open callback. The
> svn_ra__vtable_t type is private to libsvn_ra*, so you can change the
> signature of the open() callback instead of revving it. It's not a
> public API. :)
>
> Otherwise, it looks fine. Please change the svn_ra__vtable_t thing and
> update the log message, and I'll test and commit.
Patch attached.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
Re: [PATCH] notification callbacks for network data transfers (V3)
Posted by Branko Čibej <br...@xbc.nu>.
Stefan Küng wrote:
> So I've got rid of those warnings. Some more files needed changes for
> this. Now I'm not sure if the log comment should be done as it is now,
> or maybe it could be shortened so that those files where the only
> changes are to use the new functions instead of the deprecated ones
> can be grouped together?
Yes, you can do that. Just make one log entry for those files that were
changed only for the open->open2 change.
> Well, just have a look at the patch and tell me what you think.
You don't have to rev the svn_ra__vtable_t::open callback. The
svn_ra__vtable_t type is private to libsvn_ra*, so you can change the
signature of the open() callback instead of revving it. It's not a
public API. :)
Otherwise, it looks fine. Please change the svn_ra__vtable_t thing and
update the log message, and I'll test and commit.
Thanks!
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] notification callbacks for network data transfers (V3)
Posted by Stefan Küng <to...@gmail.com>.
Branko Čibej wrote:
> Yah, I know about those warnings, but the easy fix (using a cast) is one
> I'd like to avoid at all costs. I'll sit down one of these days and fix
> the dam' thing.
Sometimes a cast isn't that bad. Of course, if you can avoid the warning
without a cast, that's better.
>> I'm working on it right now. Will take some time though, so stay tuned
>> for the fixed patch. Maybe this afternoon.
>
> O.K., no rush. Sorry if I sounded a bit pissed off in my last mail --
> but I really don't have time to fix these issues myself.
I completely understand. I wouldn't have sent the patch if I would have
noticed those warnings myself.
So I've got rid of those warnings. Some more files needed changes for
this. Now I'm not sure if the log comment should be done as it is now,
or maybe it could be shortened so that those files where the only
changes are to use the new functions instead of the deprecated ones can
be grouped together? Well, just have a look at the patch and tell me
what you think.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
Re: [PATCH] notification callbacks for network data transfers (V3)
Posted by Branko Čibej <br...@xbc.nu>.
Stefan Küng wrote:
> Branko Čibej wrote:
>
>> Hey Stefan, did you actually compile this patch?
>
>
> Yes, I did. I even have a small change to main.c where a simple
> printf() outputs the values of the callback.
>
>> subversion\libsvn_ra\ra_loader.c(278) : warning C4133: 'function' :
>> incompatible types - from 'const struct svn_ra_callbacks2_t *' to
>> 'const struct svn_ra_callbacks_t *'
>> subversion\libsvn_ra\ra_loader.c(304) : warning C4716: 'svn_ra_open'
>> : must return a value
>>
>> I can certainly fix this myself, but next time, please make sure you
>> don't leave thinks ilke this in the code.
>
>
> Now I feel stupid. Very sorry about this. Seems I just missed those
> warnings in all the warnings I get when compiling Subversion (there
> are really many warnings - I think I'll write a patch later to get rid
> of those).
Yah, I know about those warnings, but the easy fix (using a cast) is one
I'd like to avoid at all costs. I'll sit down one of these days and fix
the dam' thing.
> > subversion\libsvn_ra_dav\session.c(806) : warning C4090: 'function'
> : > different 'const' qualifiers
> > subversion\libsvn_ra_dav\session.c(806) : warning C4022:
> > 'ne_set_progress' : pointer mismatch for actual parameter 3
> > subversion\libsvn_ra_dav\session.c(807) : warning C4090: 'function'
> : > different 'const' qualifiers
> > subversion\libsvn_ra_dav\session.c(807) : warning C4022:
> > 'ne_set_progress' : pointer mismatch for actual parameter 3
> >
> >
> > Together with the fact that fixing the type of ra_vtable->open to
> > accept svn_ra_callbacks2_t instead of the old, deprecated type, this
> > has become more than I'm willing to "tweak". Here's the updated patch
> > and log message, the result of my tweakings. You'll have to finish
> > this yourself.
> >
> > One of the things I did is to remove the svn_progress.h header. It's
> > not needed, the progress callback typedef belongs in svn_ra.h.
>
> I'm working on it right now. Will take some time though, so stay tuned
> for the fixed patch. Maybe this afternoon.
O.K., no rush. Sorry if I sounded a bit pissed off in my last mail --
but I really don't have time to fix these issues myself.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] notification callbacks for network data transfers (V3)
Posted by Stefan Küng <to...@gmail.com>.
Branko Čibej wrote:
> Hey Stefan, did you actually compile this patch?
Yes, I did. I even have a small change to main.c where a simple printf()
outputs the values of the callback.
> subversion\libsvn_ra\ra_loader.c(278) : warning C4133: 'function' :
> incompatible types - from 'const struct svn_ra_callbacks2_t *' to 'const
> struct svn_ra_callbacks_t *'
> subversion\libsvn_ra\ra_loader.c(304) : warning C4716: 'svn_ra_open' :
> must return a value
>
> I can certainly fix this myself, but next time, please make sure you
> don't leave thinks ilke this in the code.
Now I feel stupid. Very sorry about this. Seems I just missed those
warnings in all the warnings I get when compiling Subversion (there are
really many warnings - I think I'll write a patch later to get rid of
those).
> subversion\libsvn_ra_dav\session.c(806) : warning C4090: 'function' :
> different 'const' qualifiers
> subversion\libsvn_ra_dav\session.c(806) : warning C4022:
> 'ne_set_progress' : pointer mismatch for actual parameter 3
> subversion\libsvn_ra_dav\session.c(807) : warning C4090: 'function' :
> different 'const' qualifiers
> subversion\libsvn_ra_dav\session.c(807) : warning C4022:
> 'ne_set_progress' : pointer mismatch for actual parameter 3
>
>
> Together with the fact that fixing the type of ra_vtable->open to
> accept svn_ra_callbacks2_t instead of the old, deprecated type, this
> has become more than I'm willing to "tweak". Here's the updated patch
> and log message, the result of my tweakings. You'll have to finish
> this yourself.
>
> One of the things I did is to remove the svn_progress.h header. It's
> not needed, the progress callback typedef belongs in svn_ra.h.
I'm working on it right now. Will take some time though, so stay tuned
for the fixed patch. Maybe this afternoon.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] notification callbacks for network data transfers (V3)
Posted by Branko Čibej <br...@xbc.nu>.
Hey Stefan, did you actually compile this patch?
subversion\libsvn_ra\ra_loader.c(278) : warning C4133: 'function' : incompatible types - from 'const struct svn_ra_callbacks2_t *' to 'const struct svn_ra_callbacks_t *'
subversion\libsvn_ra\ra_loader.c(304) : warning C4716: 'svn_ra_open' : must return a value
I can certainly fix this myself, but next time, please make sure you
don't leave thinks ilke this in the code.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org