Stefan Küng wrote:
> [[[
> Add progress notification for ra_svn. This is for issue #901.
>
> Patch by: Stefan Küng <tortoisesvn@gmail.com>
>
> * subversion/libsvn_ra_svn/client.c (ra_svn_open): Initialize the
> progress callback function for svn connections.
Incorrect formatting for the log message here. Not sure whether this
is my client messing up, so just in case.
> * subversion/libsvn_ra_svn/marshal.c
> (writebuf_output): call the progress notification callback.
> (readbuf_input): Take an additional pool param and call the
> progress notification callback.
> (readbuff_fill, readbuf_read, readbuf_skip_leading_garbage,
> svn_ra_svn_skip_leading_garbage):
> Pass an additional pool param to readbuf_input.
> * subversion/libsvn_ra_svn/ra_svn.h: include svn_ra.h explicitely
> (svn_ra_svn_conn_st): Add params for the progress notifications to
> the struct.
> ]]]
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (Revision 15964)
> +++ subversion/libsvn_ra_svn/client.c (Arbeitskopie)
> @@ -644,6 +644,11 @@
> conn = svn_ra_svn_create_conn(sock, NULL, NULL, pool);
> }
>
> + conn->progress_func = callbacks->progress_func;
> + conn->progress_baton = callbacks->progress_baton;
> + conn->progress_progress = 0;
> + conn->progress_total = -1;
Why is progress_total initialized to -1 ? As you're not touching it
again in the patch, I'm supposing this is because you're indicating
the total is "not yet supported". Could you expand on this (in a
mail, plus perhaps a small comment in the code), and perhaps offer a
potential solution to make it supported if you have an idea ?
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (Revision 15964)
> +++ subversion/libsvn_ra_svn/marshal.c (Arbeitskopie)
> @@ -156,6 +156,12 @@
> status = apr_socket_send(conn->sock, data, &count);
> else
> status = apr_file_write(conn->out_file, data, &count);
> + conn->progress_progress += count;
> + if (conn->progress_func)
> + {
> + conn->progress_func(conn->progress_progress,
conn->progress_total,
> + pool, conn->progress_baton);
> + }
> @@ -247,6 +253,12 @@
> status = apr_socket_recv(conn->sock, data, len);
> else
> status = apr_file_read(conn->in_file, data, len);
> + conn->progress_progress += *len;
> + if (conn->progress_func)
> + {
> + conn->progress_func(conn->progress_progress, conn->progress_total,
> + pool, conn->progress_baton);
> + }
How does the progress notification callback know when the progress is
write progress and when it is read progress? You seem to be calling
the exact same callback each time round. What happens if reads and
writes are interlaced? How does the callback figure out what to tell
the user?
> + SVN_ERR(readbuf_input(conn, data, &count, pool));
A global comment on the addition of the extra pool parameter.
Actually, more of a question to know which is preferred: should this
extra parameter be added given that is only concerns progress
notification, or should the pool be passed in the svn_ra_svn_conn_t as
a *progress_pool parameter?
> -static svn_error_t *readbuf_skip_leading_garbage(svn_ra_svn_conn_t
*conn)
> +static svn_error_t *readbuf_skip_leading_garbage(svn_ra_svn_conn_t
*conn,
> + apr_pool_t *pool)
This is basically what prompts my above question. It seems the pool
required by progress notification is propagating change fairly high up
in the caller chain. Is this the preferred way to pass pools around
in this case?
> Index: subversion/libsvn_ra_svn/ra_svn.h
> ===================================================================
> --- subversion/libsvn_ra_svn/ra_svn.h (Revision 15964)
> +++ subversion/libsvn_ra_svn/ra_svn.h (Arbeitskopie)
> @@ -29,6 +29,7 @@
> #include <apr_file_io.h>
> #include <apr_thread_proc.h>
> #include <svn_ra_svn.h>
On a completely unrelated note, shouldn't this be #include
"svn_ra_svn.h" ?
> @@ -53,6 +54,10 @@
> ra_svn_block_handler_t block_handler;
> void *block_baton;
> apr_hash_t *capabilities;
> + svn_ra_progress_notify_func_t progress_func;
> + void *progress_baton;
> + apr_off_t progress_progress;
> + apr_off_t progress_total;
Not indispensable given the surrounding code, but a few comments here,
along with a line skip between *capabilities and progress_func would
be nice. Even though the rest of the structure is somewhat lacking on
the comment side, no need to make it worse by adding *more*
undocumented fields :-).
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Sep 4 14:27:55 2005