Re: [PATCH]: notification progress improvement/fixes (issue #901)

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
Received on Wed Aug 31 03:01:09 2005