You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2005/08/29 12:30:23 UTC
Re: svn commit: r15948 - in trunk/subversion: libsvn_client libsvn_ra
libsvn_ra_dav libsvn_ra_local libsvn_ra_svn
On Sun, 28 Aug 2005 brane@tigris.org wrote:
> Log:
> Implement a reduced version of issue #901 for DAV connections.
>
So, one has to look up the issue to know what this patch is doing. One
more sentence telling what the change is for would be good.
>
> +++ trunk/subversion/include/svn_ra.h Sun Aug 28 08:03:41 2005
> @@ -169,6 +169,19 @@
> svn_error_t *ra_err,
> apr_pool_t *pool);
>
> +/**
> + * Callback function type for progress notification.
> + *
> + * @a progress is the number of bytes already transferred, @a total is
> + * the total number of bytes to transfer or -1 if it's not known, @a
> + * baton is the callback baton.
> + *
> + * @since New in 1.3.
> + */
> +typedef void (*svn_ra_progress_notify_func_t) (apr_off_t progress,
> + apr_off_t total,
> + void * baton);
> +
Space after star, and this function should have a pool argument for
temporary allocations.
> +/** A collection of callbacks implemented by libsvn_client which allows
> + * an RA layer to "pull" information from the client application, or
> + * possibly store information. libsvn_client passes this vtable to
> + * @c RA->open().
> + *
> + * Each routine takes a @a callback_baton originally provided with the
> + * vtable.
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> */
> typedef struct svn_ra_callbacks_t
Should change the above docs to just say "Similar to ... except ...". And
could we get rid of the duplicate documentation for these fields?
Another thought: should we provide a constructor for the new struct so it
can be extended in the future without having to rev it?
> +++ trunk/subversion/libsvn_ra/ra_loader.c Sun Aug 28 08:03:41 2005
> +svn_error_t *svn_ra_open (svn_ra_session_t **session_p,
> + const char *repos_URL,
> + const svn_ra_callbacks_t *callbacks,
> + void *callback_baton,
> + apr_hash_t *config,
> + apr_pool_t *pool)
> +{
> + /* Ddeprecated function. Copy the contents of the svn_ra_callbacks_t
> + to a new svn_ra_callbacks2_t and call svn_ra_open2(). */
> + svn_ra_callbacks2_t callbacks2;
Ooops! Stack smasher. callbacks2 allocated on the stack; used beyond by
the session.
>
> Modified: trunk/subversion/libsvn_ra/wrapper_template.h
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra/wrapper_template.h?rev=15948&p1=trunk/subversion/libsvn_ra/wrapper_template.h&p2=trunk/subversion/libsvn_ra/wrapper_template.h&r1=15947&r2=15948
> ==============================================================================
> --- trunk/subversion/libsvn_ra/wrapper_template.h (original)
> +++ trunk/subversion/libsvn_ra/wrapper_template.h Sun Aug 28 08:03:41 2005
> @@ -45,9 +45,20 @@
> apr_pool_t *pool)
> {
> svn_ra_session_t *sess = apr_pcalloc (pool, sizeof (svn_ra_session_t));
> + svn_ra_callbacks2_t callbacks2;
Similar.
> +static void
> +ra_dav_neonprogress(void * baton, off_t progress, off_t total)
> +{
> + /* Important: don't change the svn_ra_callbacks2_t struct here! */
So, that's why it is const below? (Point is: why this comment?)
> + const svn_ra_callbacks2_t * callbacks = (svn_ra_callbacks2_t *)baton;
> + if (callbacks->progress_func)
> + {
> + callbacks->progress_func(progress, total, callbacks->progress_baton);
> + }
> +}
>
>
Best,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r15948 - in trunk/subversion: libsvn_client libsvn_ra
libsvn_ra_dav libsvn_ra_local libsvn_ra_svn
Posted by Branko Čibej <br...@xbc.nu>.
Peter N. Lundblad wrote:
>On Sun, 28 Aug 2005 brane@tigris.org wrote:
>
>
>>+++ trunk/subversion/libsvn_ra/ra_loader.c Sun Aug 28 08:03:41 2005
>>+svn_error_t *svn_ra_open (svn_ra_session_t **session_p,
>>+ const char *repos_URL,
>>+ const svn_ra_callbacks_t *callbacks,
>>+ void *callback_baton,
>>+ apr_hash_t *config,
>>+ apr_pool_t *pool)
>>+{
>>+ /* Ddeprecated function. Copy the contents of the svn_ra_callbacks_t
>>+ to a new svn_ra_callbacks2_t and call svn_ra_open2(). */
>>+ svn_ra_callbacks2_t callbacks2;
>>
>>
>
>Ooops! Stack smasher. callbacks2 allocated on the stack; used beyond by
>the session.
>
>
Oops indeed... My bad, I should've caught that.
>>+static void
>>+ra_dav_neonprogress(void * baton, off_t progress, off_t total)
>>+{
>>+ /* Important: don't change the svn_ra_callbacks2_t struct here! */
>>
>>
>
>So, that's why it is const below? (Point is: why this comment?)
>
>
See the comment a few lines later:
>+ /* Set the neon callback to make it call the svn_progress_notify_func_t
>+ * Note that ne_set_progress() takes a non-const baton as the third param.
>+ * Since we don't change the callback struct but only use the non-const
>+ * notification callback items of that struct, it's safe to cast */
The baton we give to ra_dav_neonprogress is const, but since we
obviously have no control over Neon's API, the best we can do is leave
the warning in there.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r15948 - in trunk/subversion: libsvn_client libsvn_ra
libsvn_ra_dav libsvn_ra_local libsvn_ra_svn
Posted by Stefan Küng <to...@gmail.com>.
Peter N. Lundblad wrote:
>>Something like
>>/** Similar to svn_ra_callbacks2_t, except that the progress
>> * notification function and baton is missing.
>> *
>> * @deprecated Provided for backward compatibility with the 1.2 API.
>> */
>>
>
> Yeah, or even
> "Similar to @c svn_ra_callbacks2_t, except that @c progress_func and @c
> progress_baton are not present". The important difference is that the
> members are referred directly by their names instead of inventing a new
> term.
In the patch I've sent, I have used my suggestion above. Maybe I should
have waited a little longer ;)
> And as we've seen this comment can be misunderstood, since I just did
> so:-) When I read it, I interpreted it like "don't change the content of
> that variable" which is exactly what the const keyword tells both the
> reader and the compiler. I would instead say that it is important that
> this variable remains const and why.
>
> Thanks for tracking this commit on the dev@ list. I'll look at your next
> patch.
I've already sent a patch. See my last mail in this thread. With that
patch, that comment is obsolete because to avoid the const-nonconst
stuff I've introduced a new structure private for the neon callback.
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: svn commit: r15948 - in trunk/subversion: libsvn_client libsvn_ra
libsvn_ra_dav libsvn_ra_local libsvn_ra_svn
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 29 Aug 2005, Stefan Küng wrote:
> Peter N. Lundblad wrote:
> > On Sun, 28 Aug 2005 brane@tigris.org wrote:
> >
> >>Log:
> >>Implement a reduced version of issue #901 for DAV connections.
> >>
> >
> > So, one has to look up the issue to know what this patch is doing. One
> > more sentence telling what the change is for would be good.
>
> I can't change that since I don't have commit access. Brane, can you
> please do it?
>
OK. I took that one myself. We normally fix other committers small
commit message mistakes when we see them, I wanted to point it out (mostly
directed at brane) because it is annoying when people just refer to an
issue number out of context.
> >>+/** A collection of callbacks implemented by libsvn_client which allows
> >>+ * an RA layer to "pull" information from the client application, or
> >>+ * possibly store information. libsvn_client passes this vtable to
> >>+ * @c RA->open().
> >>+ *
> >>+ * Each routine takes a @a callback_baton originally provided with the
> >>+ * vtable.
> >>+ *
> >>+ * @deprecated Provided for backward compatibility with the 1.2 API.
> >> */
> >> typedef struct svn_ra_callbacks_t
> >
> >
> > Should change the above docs to just say "Similar to ... except ...". And
> > could we get rid of the duplicate documentation for these fields?
>
> Something like
> /** Similar to svn_ra_callbacks2_t, except that the progress
> * notification function and baton is missing.
> *
> * @deprecated Provided for backward compatibility with the 1.2 API.
> */
>
Yeah, or even
"Similar to @c svn_ra_callbacks2_t, except that @c progress_func and @c
progress_baton are not present". The important difference is that the
members are referred directly by their names instead of inventing a new
term.
> >>+static void
> >>+ra_dav_neonprogress(void * baton, off_t progress, off_t total)
> >>+{
> >>+ /* Important: don't change the svn_ra_callbacks2_t struct here! */
> >
> >
> > So, that's why it is const below? (Point is: why this comment?)
>
> To make sure that if someone changes that code later for whatever
> reason, it's immediately clear what's allowed and what's not.
> Aren't comments there for such reasons too?
>
And as we've seen this comment can be misunderstood, since I just did
so:-) When I read it, I interpreted it like "don't change the content of
that variable" which is exactly what the const keyword tells both the
reader and the compiler. I would instead say that it is important that
this variable remains const and why.
Thanks for tracking this commit on the dev@ list. I'll look at your next
patch.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r15948 - in trunk/subversion: libsvn_client libsvn_ra
libsvn_ra_dav libsvn_ra_local libsvn_ra_svn
Posted by Stefan Küng <to...@gmail.com>.
Peter N. Lundblad wrote:
> On Sun, 28 Aug 2005 brane@tigris.org wrote:
>
>>Log:
>>Implement a reduced version of issue #901 for DAV connections.
>>
>
> So, one has to look up the issue to know what this patch is doing. One
> more sentence telling what the change is for would be good.
I can't change that since I don't have commit access. Brane, can you
please do it?
>>+++ trunk/subversion/include/svn_ra.h Sun Aug 28 08:03:41 2005
>>@@ -169,6 +169,19 @@
>> svn_error_t *ra_err,
>> apr_pool_t *pool);
>>
>>+/**
>>+ * Callback function type for progress notification.
>>+ *
>>+ * @a progress is the number of bytes already transferred, @a total is
>>+ * the total number of bytes to transfer or -1 if it's not known, @a
>>+ * baton is the callback baton.
>>+ *
>>+ * @since New in 1.3.
>>+ */
>>+typedef void (*svn_ra_progress_notify_func_t) (apr_off_t progress,
>>+ apr_off_t total,
>>+ void * baton);
>>+
>
>
> Space after star, and this function should have a pool argument for
> temporary allocations.
I'll take this one. Expect a patch soon.
>>+/** A collection of callbacks implemented by libsvn_client which allows
>>+ * an RA layer to "pull" information from the client application, or
>>+ * possibly store information. libsvn_client passes this vtable to
>>+ * @c RA->open().
>>+ *
>>+ * Each routine takes a @a callback_baton originally provided with the
>>+ * vtable.
>>+ *
>>+ * @deprecated Provided for backward compatibility with the 1.2 API.
>> */
>> typedef struct svn_ra_callbacks_t
>
>
> Should change the above docs to just say "Similar to ... except ...". And
> could we get rid of the duplicate documentation for these fields?
Something like
/** Similar to svn_ra_callbacks2_t, except that the progress
* notification function and baton is missing.
*
* @deprecated Provided for backward compatibility with the 1.2 API.
*/
> Another thought: should we provide a constructor for the new struct so it
> can be extended in the future without having to rev it?
Good idea. I'll take this one too.
>>+++ trunk/subversion/libsvn_ra/ra_loader.c Sun Aug 28 08:03:41 2005
>>+svn_error_t *svn_ra_open (svn_ra_session_t **session_p,
>>+ const char *repos_URL,
>>+ const svn_ra_callbacks_t *callbacks,
>>+ void *callback_baton,
>>+ apr_hash_t *config,
>>+ apr_pool_t *pool)
>>+{
>>+ /* Ddeprecated function. Copy the contents of the svn_ra_callbacks_t
>>+ to a new svn_ra_callbacks2_t and call svn_ra_open2(). */
>>+ svn_ra_callbacks2_t callbacks2;
>
>
> Ooops! Stack smasher. callbacks2 allocated on the stack; used beyond by
> the session.
Right! Ooops! ;)
I'll fix that one too.
>>+static void
>>+ra_dav_neonprogress(void * baton, off_t progress, off_t total)
>>+{
>>+ /* Important: don't change the svn_ra_callbacks2_t struct here! */
>
>
> So, that's why it is const below? (Point is: why this comment?)
To make sure that if someone changes that code later for whatever
reason, it's immediately clear what's allowed and what's not.
Aren't comments there for such reasons too?
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 progress improvement/fixes (issue #901)
Posted by Branko Čibej <br...@xbc.nu>.
David Anderson wrote:
> Peter N. Lundblad wrote:
>
>> Not all of our API users know our revving policy. I think it is a bit
>> nice
>> to be a little verbose here and give the reason. But if you change
>> it, it
>> should be changed everywhere.
>
>
> API Users need not know about the revving policies in such detail.
> They simply need to know that they have to go through the constructor.
This is exactly my point. In my experience, telling people more than is
necessary will only confuse them. :)
> Perhaps the implementation of the constructor could have a comment to
> the effect of recalling the revving policy, but I'm not sure that
> documenting it in the public API would bring anything but extra clutter.
Our compatibility rules and API revving rules are already documented.
Sowing extra comments all over the docstrings isn't only effor
duplication, it's also a maintenance nightmare. What if we change our
compatibility policy come 2.0 (we can)?
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: notification progress improvement/fixes (issue #901)
Posted by David Anderson <da...@calixo.net>.
Peter N. Lundblad wrote:
> Not all of our API users know our revving policy. I think it is a bit nice
> to be a little verbose here and give the reason. But if you change it, it
> should be changed everywhere.
API Users need not know about the revving policies in such detail. They
simply need to know that they have to go through the constructor.
Perhaps the implementation of the constructor could have a comment to
the effect of recalling the revving policy, but I'm not sure that
documenting it in the public API would bring anything but extra clutter.
> I suggest just allocating the struct by hand in wrapper_template.h (with a
> comment explaining why, of course). This is OK because you can't mix
> different libsvn_ra and libsvn_ra_whatever versions.
I see. Thanks for the explanation as to the purpose of
wrapper_template.h ! Once we've settled on the question of the
docstrings, I'll commit a fix for this.
>
> Thanks,
> //Peter
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: notification progress improvement/fixes (issue #901)
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 31 Aug 2005, David Anderson wrote:
> Branko Čibej wrote:
> > Don't even explain why. Just say, "Clients must use
> > svn_ra_create_callbacks() to allocate and initialize this structure."
>
> I was wondering about that, and decided to leave in minimal verbosity.
> But it's true that it makes sense in the current revving policy for
> structures, so no need to restate things.
>
Not all of our API users know our revving policy. I think it is a bit nice
to be a little verbose here and give the reason. But if you change it, it
should be changed everywhere.
> > Uh? Where does this dependency come from? I don't see either svnserve or
> > libsvn_ra_svn using this constructor anywhere.
>
> subversion/libsvn_ra_svn/client.c includes
> subversion/libsvn_ra/wrapper_template.h, which refers to the constructor
> in compat_open().
>
This leads to a circular dependency: libsvn_ra ->
libsvn_ra_{svn,local,dav} -> libsvn_ra. That doesn't work on all
platforms. So you can't use the constructor in wrapper_template.h. (The
whole reason for that file is to avoid circular dependencies; else
libsvn_ra could have provided wrappers for the RA implementations.)
I suggest just allocating the struct by hand in wrapper_template.h (with a
comment explaining why, of course). This is OK because you can't mix
different libsvn_ra and libsvn_ra_whatever versions.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: notification progress improvement/fixes (issue #901)
Posted by Branko Čibej <br...@xbc.nu>.
David Anderson wrote:
> Branko Čibej wrote:
>
>> Uh? Where does this dependency come from? I don't see either svnserve
>> or libsvn_ra_svn using this constructor anywhere.
>
> subversion/libsvn_ra_svn/client.c includes
> subversion/libsvn_ra/wrapper_template.h, which refers to the
> constructor in compat_open().
Ah. Hm. Having svnserve delend on libsvn_ra seems ... unclean, somehow.
Oh well.
> Any further comments before I submit a (final?) followup patch for
> this fix?
I posted a few comments about the r16010 commit. Other than that, no.
Nice work, Dave an Stefan; thanks!
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: notification progress improvement/fixes (issue #901)
Posted by David Anderson <da...@calixo.net>.
Branko Čibej wrote:
> Don't even explain why. Just say, "Clients must use
> svn_ra_create_callbacks() to allocate and initialize this structure."
I was wondering about that, and decided to leave in minimal verbosity.
But it's true that it makes sense in the current revving policy for
structures, so no need to restate things.
> And let's please remove the unnecessary cast from void*. I don't care if
> VS.NET gripes about them at warning level 4 -- which is useless for
> normal code, anyway -- no-op casts clutter up the code for no benefit.
I was wondering about that too :-).
> Uh? Where does this dependency come from? I don't see either svnserve or
> libsvn_ra_svn using this constructor anywhere.
subversion/libsvn_ra_svn/client.c includes
subversion/libsvn_ra/wrapper_template.h, which refers to the constructor
in compat_open().
Any further comments before I submit a (final?) followup patch for this fix?
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: notification progress improvement/fixes (issue #901)
Posted by Branko Čibej <br...@xbc.nu>.
David Anderson wrote:
> Stefan Küng wrote:
>
>> + * In order to avoid backward compatibility problems, clients should
>> use
>> + * svn_ra_create_callbacks() to allocate and initialize this structure
>> + * instead of doing so themselves.
>
>
> Don't give a choice: "In order to ease future compatibility, clients
> must use svn_ra_create_callbacks() to allocate and initialize this
> structure."
Don't even explain why. Just say, "Clients must use
svn_ra_create_callbacks() to allocate and initialize this structure."
>> + neonprogress_baton_t * neonprogress_baton = (neonprogress_baton_t
>> *)baton;
>
>
> You can make this pointer const, to be nicely strict. Also, no space
> between the '*' and the variable name, to remain consistent with the
> surrounding code.
And let's please remove the unnecessary cast from void*. I don't care if
VS.NET gripes about them at warning level 4 -- which is useless for
normal code, anyway -- no-op casts clutter up the code for no benefit.
> - Add a dependancy on libsvn_ra to svnserve in build.conf, otherwise
> it won't build because of an undefined reference to the constructor.
Uh? Where does this dependency come from? I don't see either svnserve or
libsvn_ra_svn using this constructor anywhere.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: notification progress improvement/fixes (issue #901)
Posted by Erik Huelsmann <eh...@gmail.com>.
On 8/31/05, Stefan Küng <to...@gmail.com> wrote:
> On 8/31/05, David Anderson <da...@calixo.net> wrote:
>
> > > + * In order to avoid backward compatibility problems, clients should use
> > > + * svn_ra_create_callbacks() to allocate and initialize this structure
> > > + * instead of doing so themselves.
> >
> > Don't give a choice: "In order to ease future compatibility, clients
> > must use svn_ra_create_callbacks() to allocate and initialize this
> > structure."
>
> Hmm, I used the comment in svn_client_create_context() and changed it
> to fit svn_ra_create_callbacks().
> Maybe we should change the comment there too? Because in
> subversion/include/svn_client.h, line 477, it also states "should use".
Yep. That's actually on my todo. (But feel free to do it for me, I'm a
bit short on time for coding...)
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: notification progress improvement/fixes (issue #901)
Posted by Stefan Küng <to...@gmail.com>.
On 8/31/05, David Anderson <da...@calixo.net> wrote:
> > + * In order to avoid backward compatibility problems, clients should use
> > + * svn_ra_create_callbacks() to allocate and initialize this structure
> > + * instead of doing so themselves.
>
> Don't give a choice: "In order to ease future compatibility, clients
> must use svn_ra_create_callbacks() to allocate and initialize this
> structure."
Hmm, I used the comment in svn_client_create_context() and changed it
to fit svn_ra_create_callbacks().
Maybe we should change the comment there too? Because in
subversion/include/svn_client.h, line 477, it also states "should use".
> > +* The current implementation never returns error, but callers should
> > +* still check for error, for compatibility with future versions.
>
> No need to speak of implementation in public API documentation. The
> function returns svn_error_t*, which means errors may be thrown and must
> be handled one way or another, period.
Again, the docstrings for svn_client_create_context() have the same comment.
(just mentioning it here because if it's not good here, we should
change it there too).
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 progress improvement/fixes (issue #901)
Posted by David Anderson <da...@calixo.net>.
Stefan Küng wrote:
> [[[
As I accidentally rediscovered part of the bug you're fixing here and
fixed it (again, partially) in r16k. Woops. I'll revert my own fix and
apply your fixes to your last commit, with the modifications I discuss
in this review.
Other commiters: meta-reviews most welcome, I'm new to this.
> Fix stack bug, improve the progress notification callback feature.
This patch should be just a fixing patch. Thus, this is no real
improvement to the progress notification feature itself, just a followup
to r15948. Thus, "Followup to r15948. Fix stack smashing bugs and
other smaller issues."
> + apr_pool_t *pool,
> + void *baton);
Pools are usually the last parameter of functions and callbacks. Any
reason for this switch? Apparently not by looking at the surrounding
code, so I've put them back in the usual order.
> + * In order to avoid backward compatibility problems, clients should use
> + * svn_ra_create_callbacks() to allocate and initialize this structure
> + * instead of doing so themselves.
Don't give a choice: "In order to ease future compatibility, clients
must use svn_ra_create_callbacks() to allocate and initialize this
structure."
> @@ -439,6 +417,23 @@
> svn_error_t *
> svn_ra_initialize (apr_pool_t *pool);
>
> +/** Initialize a callback structure.
> +* Set @a *callbacks to a ra callbacks object, allocated in @a pool.
> +*
> +* In order to avoid backwards compatibility problems, clients must
> +* use this function to initialize and allocate the
allocate, then initialize ;-).
> +* The current implementation never returns error, but callers should
> +* still check for error, for compatibility with future versions.
No need to speak of implementation in public API documentation. The
function returns svn_error_t*, which means errors may be thrown and must
be handled one way or another, period.
> + neonprogress_baton_t * neonprogress_baton = (neonprogress_baton_t *)baton;
You can make this pointer const, to be nicely strict. Also, no space
between the '*' and the variable name, to remain consistent with the
surrounding code.
> @@ -593,6 +601,8 @@
> svn_boolean_t compression;
> svn_config_t *cfg;
> const char *server_group;
> + neonprogress_baton_t * neonprogress_baton = apr_pcalloc (pool,
Same comment about the extra space.
I've commited your fix, modulo these changes, as r16010, with a followup
to cover for my own stupid mistakes in r16011:
- Pass &callbacks2 to the constructor function, not just callbacks2.
- Add a dependancy on libsvn_ra to svnserve in build.conf, otherwise
it won't build because of an undefined reference to the constructor.
While I'm at it, I'll review your feature patch (hopefully with a little
more success first time round). See my next mail.
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH]: notification progress improvement/fixes (issue #901)
Posted by Stefan Küng <to...@gmail.com>.
Branko Čibej wrote:
> Stefan Küng wrote:
>
>> [[[
>> Fix stack bug, improve the progress notification callback feature and
>> add progress notification for ra_svn. This is for issue #901.
>>
>>
> Could you please split this in two parts, one patch that addresses
> Peter's comments, and a separate patch for the ra_svn changes? It's much
> easier to review that way. Also, I expect that the ra_svn changes will
> go through several iterations, whilst we'd want to commit the fixes
> soon. It's a bit of a pain to have to extract and commit part of a patch.
Ok, separated patches attached.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
Re: svn commit: r15948 - in trunk/subversion: libsvn_client libsvn_ra
libsvn_ra_dav libsvn_ra_local libsvn_ra_svn
Posted by Branko Čibej <br...@xbc.nu>.
Stefan Küng wrote:
>[[[
>Fix stack bug, improve the progress notification callback feature and
>add progress notification for ra_svn. This is for issue #901.
>
>
Could you please split this in two parts, one patch that addresses
Peter's comments, and a separate patch for the ra_svn changes? It's much
easier to review that way. Also, I expect that the ra_svn changes will
go through several iterations, whilst we'd want to commit the fixes
soon. It's a bit of a pain to have to extract and commit part of a patch.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r15948 - in trunk/subversion: libsvn_client libsvn_ra
libsvn_ra_dav libsvn_ra_local libsvn_ra_svn
Posted by Stefan Küng <to...@gmail.com>.
Patch with fixes for what Peter commented on is attached.
The patch also contains an implementation for progress notifications for
the ra_svn protocol. It doesn't provide a 'total' for now, but it
notifies about bytes sent/received. Maybe someone more familiar with
ra_svn can give me some pointers on where/how to set the total amount of
bytes to transfer? If there's no good way to do that, then this patch
should be enough for GUI clients to at least show the user information
like "YYY bytes transferred, currently with XXX bytes/s" (ok, lame
sentence, but you get the idea).
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org