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 2008/08/02 07:40:17 UTC
[PATCH] fix progress callback for ra_svn
Hi,
Some users reported that TSVN shows unbelievable amounts of bytes
transferred for all operations, but only when used with svn://.
I've tried ra_serf and ra_neon, those work ok. But ra_svn passes the sum
of bytes transferred to the progress callback, while neon and serf pass
only the bytes just transferred to the callback (they don't sum up the
transferred bytes).
I'd like to have all three layers call the progress callback the same
way. Since adjusting ra_neon to do the same as ra_svn does would require
a change in the neon library, I think it's best to change ra_svn (also,
the callback was inspired by the neon progress callback, so I'd say that
it's ra_svn which does it wrong).
the attached patch fixes this.
I like to propose this also for backporting to 1.5.x.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] fix progress callback for ra_svn
Posted by Stefan Küng <to...@gmail.com>.
Joe Orton wrote:
> On Sat, Aug 02, 2008 at 01:31:40PM +0200, Stefan Küng wrote:
>> An example (pseudo-checkout):
>> callback(200, -1) // for connecting
>> callback(300, -1) // for info packets
>> callback(512, 123000) // first file
>> callback(512, 123000) // first file
>> callback(512, 123000) // first file
>> ...
>> callback(30, 123000) // first file
>> callback(300, -1) // for info packets
>> callback(512, 234000) // second file
>> callback(512, 234000) // second file
>> callback(512, 234000) // second file
>> ...
>> callback(30, 234000) // second file
>
> The progress parameter has been a counter which increments for each
> block transferred in all version of neon except 0.27.0, which had a bug
> in the ne_set_progress() compat API which was fixed in 0.27.1 (and
> didn't exist in any earlier version). Are you using 0.27.0 here?
I'm using the latest neon (0.28.2).
But seems I was wrong here: the problem is not in how the callback is
called (I already deal with that in TSVN correctly).
The problem comes from the fact that some svn commands use more than one
session. When that happens, the client (i.e., TSVN) does not know that
the progress information is passed from more than one session and that
leads to really confusing progress info. I've noticed that for example
merges use two sessions (in ra_svn, haven't checked the other layers).
And each session calls the progress callback.
Example (s1 = session 1, s2 = session 2):
s1: callback( 0, -1 )
s2: callback( 0, -1 )
s1: callback( 512, -1 )
s2: callback( 256, -1 )
s1: callback( 1024, -1 )
s2: callback( 512, -1)
...
(this is just an example, real info is usually different, but you
hopefully get what my problem is)
as you can see, the callback is called with 'switching' progress
information, but of course the callback consumer has no information
about the session from which it is called.
Any idea how I could handle this? IMHO Subversion should either
'combine' the progress information of the sessions or otherwise extend
the callback api to include info about the session. Otherwise I don't
know how I can make good use of the progress info.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] fix progress callback for ra_svn
Posted by Joe Orton <jo...@redhat.com>.
On Sat, Aug 02, 2008 at 01:31:40PM +0200, Stefan Küng wrote:
> An example (pseudo-checkout):
> callback(200, -1) // for connecting
> callback(300, -1) // for info packets
> callback(512, 123000) // first file
> callback(512, 123000) // first file
> callback(512, 123000) // first file
> ...
> callback(30, 123000) // first file
> callback(300, -1) // for info packets
> callback(512, 234000) // second file
> callback(512, 234000) // second file
> callback(512, 234000) // second file
> ...
> callback(30, 234000) // second file
The progress parameter has been a counter which increments for each
block transferred in all version of neon except 0.27.0, which had a bug
in the ne_set_progress() compat API which was fixed in 0.27.1 (and
didn't exist in any earlier version). Are you using 0.27.0 here?
joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ra_serf/ra_neon/ra_svn comparison
Posted by Mark Phippard <ma...@gmail.com>.
On Sun, Aug 3, 2008 at 8:52 AM, Stefan Küng <to...@gmail.com> wrote:
> Daniel Shahaf wrote:
>>
>> I was about to commit the latest patch, but then I thought to see what
>> ra_serf does. (A little late, I know.)
>>
>> ra_serf, like ra_svn (and unlike ra_neon), currently reports the totals,
>> not the deltas (see svn_ra_serf__progress and serf_context_progress_delta).
>>
>> Any ideas how to make all three ra layers consistent? Given that two
>> different ra layers implemented the "wrong" (= different from neon's)
>> (but, IMHO, consistent with the docstring) semantics, I don't think we
>> can just ignore that and change svn_ra_progress_notify_func_t's doc
>> string now.
>
> I don't mind changing it, as long as it is consistend over all layers.
> But for ra_neon, this would require a change in the neon library. And I'm
> not sure, but I think that progress notifications for ra_serf were also not
> available in 1.4 yet, while neon had this from 1.3 on.
I think that they should all be made consistent with Neon and if the
docstring needs to be fixed, then fix that too.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ra_serf/ra_neon/ra_svn comparison
Posted by Stefan Küng <to...@gmail.com>.
Daniel Shahaf wrote:
> I was about to commit the latest patch, but then I thought to see what
> ra_serf does. (A little late, I know.)
>
> ra_serf, like ra_svn (and unlike ra_neon), currently reports the totals,
> not the deltas (see svn_ra_serf__progress and serf_context_progress_delta).
>
> Any ideas how to make all three ra layers consistent? Given that two
> different ra layers implemented the "wrong" (= different from neon's)
> (but, IMHO, consistent with the docstring) semantics, I don't think we
> can just ignore that and change svn_ra_progress_notify_func_t's doc
> string now.
I don't mind changing it, as long as it is consistend over all layers.
But for ra_neon, this would require a change in the neon library. And
I'm not sure, but I think that progress notifications for ra_serf were
also not available in 1.4 yet, while neon had this from 1.3 on.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
ra_serf/ra_neon/ra_svn comparison (was: Re: [PATCH] fix progress
callback for ra_svn)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I was about to commit the latest patch, but then I thought to see what
ra_serf does. (A little late, I know.)
ra_serf, like ra_svn (and unlike ra_neon), currently reports the totals,
not the deltas (see svn_ra_serf__progress and serf_context_progress_delta).
Any ideas how to make all three ra layers consistent? Given that two
different ra layers implemented the "wrong" (= different from neon's)
(but, IMHO, consistent with the docstring) semantics, I don't think we
can just ignore that and change svn_ra_progress_notify_func_t's doc
string now.
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix progress callback for ra_svn
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Aug 03, 2008 at 09:58:07AM +0200, Stefan Küng wrote:
> Daniel Shahaf wrote:
> > Index: subversion/libsvn_ra_svn/ra_svn.h
> > ===================================================================
> > --- subversion/libsvn_ra_svn/ra_svn.h (revision 32316)
> > +++ subversion/libsvn_ra_svn/ra_svn.h (working copy)
> > @@ -95,8 +95,6 @@ struct svn_ra_svn__session_baton_t {
> > const char **tunnel_argv;
> > const svn_ra_callbacks2_t *callbacks;
> > void *callbacks_baton;
> > - apr_off_t bytes_read, bytes_written; /* apr_off_t's because that's what
> > - the callback interface uses */
> > };
>
> this changes the struct svn_ra_svn__session_baton_t, which is AFAIK ok
> for trunk. But wouldn't that break some compatibility rules when
> backporting to 1.5.x?
The struct is private to libsvn_ra_svn, so this change does not
affect public API.
Nobody except libsvn_ra_svn should be using this struct directly.
So I don't think it's a problem, even for backports -- people will
just upgrade to a new libsvn_ra_svn shared library and won't see
a difference from the outside.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix progress callback for ra_svn
Posted by Stefan Küng <to...@gmail.com>.
Daniel Shahaf wrote:
> Stefan Küng wrote on Sun, 3 Aug 2008 at 08:50 +0200:
>> new patch attached with hopefully better docstring.
>
> Docstring looks good, but why can't we just get rid of the 'bytes_read'
> and 'bytes_written' struct members? The semantics would be slightly
> different than with your patch, though...
>
> e.g. (untested)
That patch looks good to me. Just one question (see below)
> Index: subversion/libsvn_ra_svn/ra_svn.h
> ===================================================================
> --- subversion/libsvn_ra_svn/ra_svn.h (revision 32316)
> +++ subversion/libsvn_ra_svn/ra_svn.h (working copy)
> @@ -95,8 +95,6 @@ struct svn_ra_svn__session_baton_t {
> const char **tunnel_argv;
> const svn_ra_callbacks2_t *callbacks;
> void *callbacks_baton;
> - apr_off_t bytes_read, bytes_written; /* apr_off_t's because that's what
> - the callback interface uses */
> };
this changes the struct svn_ra_svn__session_baton_t, which is AFAIK ok
for trunk. But wouldn't that break some compatibility rules when
backporting to 1.5.x?
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] fix progress callback for ra_svn
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Küng wrote on Sun, 3 Aug 2008 at 08:50 +0200:
> new patch attached with hopefully better docstring.
Docstring looks good, but why can't we just get rid of the 'bytes_read'
and 'bytes_written' struct members? The semantics would be slightly
different than with your patch, though...
e.g. (untested)
Index: subversion/libsvn_ra_svn/ra_svn.h
===================================================================
--- subversion/libsvn_ra_svn/ra_svn.h (revision 32316)
+++ subversion/libsvn_ra_svn/ra_svn.h (working copy)
@@ -95,8 +95,6 @@ struct svn_ra_svn__session_baton_t {
const char **tunnel_argv;
const svn_ra_callbacks2_t *callbacks;
void *callbacks_baton;
- apr_off_t bytes_read, bytes_written; /* apr_off_t's because that's what
- the callback interface uses */
};
/* Set a callback for blocked writes on conn. This handler may
Index: subversion/libsvn_ra_svn/client.c
===================================================================
--- subversion/libsvn_ra_svn/client.c (revision 32316)
+++ subversion/libsvn_ra_svn/client.c (working copy)
@@ -543,7 +543,6 @@ static svn_error_t *open_session(svn_ra_svn__sessi
sess->tunnel_argv = tunnel_argv;
sess->callbacks = callbacks;
sess->callbacks_baton = callbacks_baton;
- sess->bytes_read = sess->bytes_written = 0;
if (tunnel_argv)
SVN_ERR(make_tunnel(tunnel_argv, &conn, pool));
Index: subversion/libsvn_ra_svn/marshal.c
===================================================================
--- subversion/libsvn_ra_svn/marshal.c (revision 32316)
+++ subversion/libsvn_ra_svn/marshal.c (working copy)
@@ -175,11 +175,9 @@ static svn_error_t *writebuf_output(svn_ra_svn_con
if (session)
{
const svn_ra_callbacks2_t *cb = session->callbacks;
- session->bytes_written += count;
if (cb && cb->progress_func)
- (cb->progress_func)(session->bytes_written + session->bytes_read,
- -1, cb->progress_baton, subpool);
+ (cb->progress_func)(count, -1, cb->progress_baton, subpool);
}
}
@@ -266,11 +264,9 @@ static svn_error_t *readbuf_input(svn_ra_svn_conn_
if (session)
{
const svn_ra_callbacks2_t *cb = session->callbacks;
- session->bytes_read += *len;
if (cb && cb->progress_func)
- (cb->progress_func)(session->bytes_read + session->bytes_written,
- -1, cb->progress_baton, pool);
+ (cb->progress_func)(*len, -1, cb->progress_baton, pool);
}
return SVN_NO_ERROR;
Index: subversion/include/svn_ra.h
===================================================================
--- subversion/include/svn_ra.h (revision 32316)
+++ subversion/include/svn_ra.h (working copy)
@@ -183,9 +183,12 @@ typedef svn_error_t *(*svn_ra_lock_callback_t)(voi
/**
* 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.
+ * @a progress is the number of bytes transferred since the last call
+ * of the callback funtion, @a total is the total number of bytes to
+ * transfer or -1 if it's not known, @a baton is the callback baton.
+ * The total number of bytes to transfer can either be the real total
+ * of the whole command or just the total of a known piece, e.g., a
+ * file.
*
* @since New in 1.3.
*/
Re: [PATCH] fix progress callback for ra_svn
Posted by Stefan Küng <to...@gmail.com>.
Daniel Shahaf wrote:
> Stefan Küng wrote on Sat, 2 Aug 2008 at 14:45 +0200:
>> Daniel Shahaf wrote:
>>>> An example (pseudo-checkout):
>>>> callback(200, -1) // for connecting
>>>> callback(300, -1) // for info packets
>>>> callback(512, 123000) // first file
>>>> callback(512, 123000) // first file
>>>> callback(512, 123000) // first file
>>>> ...
>>>> callback(30, 123000) // first file
>>>> callback(300, -1) // for info packets
>>>> callback(512, 234000) // second file
>>>> callback(512, 234000) // second file
>>>> callback(512, 234000) // second file
>>>> ...
>>>> callback(30, 234000) // second file
>>>>
>>>> Does this explain it more clear? If not, just ask.
>>>>
>>> I don't understand anything here. The 'total' values you provide are
>>> not very helpful -- they are neither a lower bound nor an upper bound on
>>> the number of bytes that will be transferred -- so what *do* they mean?
>>>
>>> (They are not a lower bound because the last callback() on the last file
>>> will still report that file's size as 'total'. They are not an upper
>>> bound because after the series of calls for the first file, with
>>> total=123000, comes another file.)
>>>
>>> For reference, svn_ra_progress_notify_func_t's says that "@a total is
>>> the total number of bytes to transfer or -1 if it's not known".
>> I use this in TSVN when big files are transferred. Imagine a file has 100MB in
>> size. Without that 'intermediate total' I get from neon, there would be
>> nothing to show to the user. But with that intermediate total, I can show a
>> progress bar so the user can see how long it will take to transfer that big
>> file (the progress bar is hidden again after that big file has been
>> transferred - I only show the bar for big file, not the small ones).
>>
>> I know it's not as good as an upper bound total, but that would require big
>> changes to the svn code to really get that number. And it's better than
>> nothing - I take what I can get.
>>
>
> Okay, thanks for the explanation. It makes sense, but it needs to be
> documented -- our docstring doesn't even say the word "file" right now.
new patch attached with hopefully better docstring.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] fix progress callback for ra_svn
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Küng wrote on Sat, 2 Aug 2008 at 14:45 +0200:
> Daniel Shahaf wrote:
> > > An example (pseudo-checkout):
> > > callback(200, -1) // for connecting
> > > callback(300, -1) // for info packets
> > > callback(512, 123000) // first file
> > > callback(512, 123000) // first file
> > > callback(512, 123000) // first file
> > > ...
> > > callback(30, 123000) // first file
> > > callback(300, -1) // for info packets
> > > callback(512, 234000) // second file
> > > callback(512, 234000) // second file
> > > callback(512, 234000) // second file
> > > ...
> > > callback(30, 234000) // second file
> > >
> > > Does this explain it more clear? If not, just ask.
> > >
> >
> > I don't understand anything here. The 'total' values you provide are
> > not very helpful -- they are neither a lower bound nor an upper bound on
> > the number of bytes that will be transferred -- so what *do* they mean?
> >
> > (They are not a lower bound because the last callback() on the last file
> > will still report that file's size as 'total'. They are not an upper
> > bound because after the series of calls for the first file, with
> > total=123000, comes another file.)
> >
> > For reference, svn_ra_progress_notify_func_t's says that "@a total is
> > the total number of bytes to transfer or -1 if it's not known".
>
> I use this in TSVN when big files are transferred. Imagine a file has 100MB in
> size. Without that 'intermediate total' I get from neon, there would be
> nothing to show to the user. But with that intermediate total, I can show a
> progress bar so the user can see how long it will take to transfer that big
> file (the progress bar is hidden again after that big file has been
> transferred - I only show the bar for big file, not the small ones).
>
> I know it's not as good as an upper bound total, but that would require big
> changes to the svn code to really get that number. And it's better than
> nothing - I take what I can get.
>
Okay, thanks for the explanation. It makes sense, but it needs to be
documented -- our docstring doesn't even say the word "file" right now.
Re: [PATCH] fix progress callback for ra_svn
Posted by Stefan Küng <to...@gmail.com>.
Daniel Shahaf wrote:
>> An example (pseudo-checkout):
>> callback(200, -1) // for connecting
>> callback(300, -1) // for info packets
>> callback(512, 123000) // first file
>> callback(512, 123000) // first file
>> callback(512, 123000) // first file
>> ...
>> callback(30, 123000) // first file
>> callback(300, -1) // for info packets
>> callback(512, 234000) // second file
>> callback(512, 234000) // second file
>> callback(512, 234000) // second file
>> ...
>> callback(30, 234000) // second file
>>
>> Does this explain it more clear? If not, just ask.
>>
>
> I don't understand anything here. The 'total' values you provide are
> not very helpful -- they are neither a lower bound nor an upper bound on
> the number of bytes that will be transferred -- so what *do* they mean?
>
> (They are not a lower bound because the last callback() on the last file
> will still report that file's size as 'total'. They are not an upper
> bound because after the series of calls for the first file, with
> total=123000, comes another file.)
>
> For reference, svn_ra_progress_notify_func_t's says that "@a total is
> the total number of bytes to transfer or -1 if it's not known".
I use this in TSVN when big files are transferred. Imagine a file has
100MB in size. Without that 'intermediate total' I get from neon, there
would be nothing to show to the user. But with that intermediate total,
I can show a progress bar so the user can see how long it will take to
transfer that big file (the progress bar is hidden again after that big
file has been transferred - I only show the bar for big file, not the
small ones).
I know it's not as good as an upper bound total, but that would require
big changes to the svn code to really get that number. And it's better
than nothing - I take what I can get.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] fix progress callback for ra_svn
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Küng wrote on Sat, 2 Aug 2008 at 13:31 +0200:
> Daniel Shahaf wrote:
> > Stefan Küng wrote on Sat, 2 Aug 2008 at 12:40 +0200:
> > > Daniel Shahaf wrote:
> > > > As I read it, it promises the (current) ra_svn behaviour.
> > > Yes, but it works not the same way as neon and serf. Since the
> > > callback for neon is actually done in the neon library, we can't
> > > change that.
> >
> > We could write a wrapper callback that manipulates (sums) what neon
> > reports.
>
> But that would break the compatibility.
> Progress info for ra_svn was added in 1.5, while it was available for
> ra_neon in 1.3.
>
This convinces me; it's better to break compat with ra_svn@1.5.0 than
with ra_neon@1.3.0. (It also explains how no one spotted this bug
before.)
> >
> > > Neon does it this way because it can 'restart' when needed. For
> > > example, most of the time the total is not known beforehand. But
> > > sometimes it is. Neon then fills in the 'total' param (instead of
> > > passing -1 there) and starts counting the bytes again.
> > >
> >
> > I'm not familiar with 'restarts' in this context. From your description
> > it sounds that they have to do with how neon calculates the number of
> > bytes it has transferred, not with whether it chooses to report that
> > number directly or as a difference since the preceding reported amount.
> >
> > Am I missing something?
>
> An example (pseudo-checkout):
> callback(200, -1) // for connecting
> callback(300, -1) // for info packets
> callback(512, 123000) // first file
> callback(512, 123000) // first file
> callback(512, 123000) // first file
> ...
> callback(30, 123000) // first file
> callback(300, -1) // for info packets
> callback(512, 234000) // second file
> callback(512, 234000) // second file
> callback(512, 234000) // second file
> ...
> callback(30, 234000) // second file
>
> Does this explain it more clear? If not, just ask.
>
I don't understand anything here. The 'total' values you provide are
not very helpful -- they are neither a lower bound nor an upper bound on
the number of bytes that will be transferred -- so what *do* they mean?
(They are not a lower bound because the last callback() on the last file
will still report that file's size as 'total'. They are not an upper
bound because after the series of calls for the first file, with
total=123000, comes another file.)
For reference, svn_ra_progress_notify_func_t's says that "@a total is
the total number of bytes to transfer or -1 if it's not known".
Daniel
Re: [PATCH] fix progress callback for ra_svn
Posted by Stefan Küng <to...@gmail.com>.
Daniel Shahaf wrote:
> Stefan Küng wrote on Sat, 2 Aug 2008 at 12:40 +0200:
>> Daniel Shahaf wrote:
>>> Stefan Küng wrote on Sat, 2 Aug 2008 at 12:27 +0200:
>>>> Stefan Sperling wrote:
>>>>> The API docs for svn_ra_progress_notify_func_t do not say
>>>>> whether the callback should pass the sum of bytes transferred
>>>>> so far, or just the amount of bytes transferred since the last
>>>>> time the callback was called:
>>>>>
>>>>> /**
>>>>> * Callback function type for progress notification.
>>>>> *
>>>>> * @a progress is the number of bytes already transferred, @a total is
>>> As I read it, it promises the (current) ra_svn behaviour.
>> Yes, but it works not the same way as neon and serf. Since the callback for
>> neon is actually done in the neon library, we can't change that.
>
> We could write a wrapper callback that manipulates (sums) what neon
> reports.
But that would break the compatibility.
Progress info for ra_svn was added in 1.5, while it was available for
ra_neon in 1.3.
>
>> Neon does it this way because it can 'restart' when needed. For example, most
>> of the time the total is not known beforehand. But sometimes it is. Neon then
>> fills in the 'total' param (instead of passing -1 there) and starts counting
>> the bytes again.
>>
>
> I'm not familiar with 'restarts' in this context. From your description
> it sounds that they have to do with how neon calculates the number of
> bytes it has transferred, not with whether it chooses to report that
> number directly or as a difference since the preceding reported amount.
>
> Am I missing something?
An example (pseudo-checkout):
callback(200, -1) // for connecting
callback(300, -1) // for info packets
callback(512, 123000) // first file
callback(512, 123000) // first file
callback(512, 123000) // first file
...
callback(30, 123000) // first file
callback(300, -1) // for info packets
callback(512, 234000) // second file
callback(512, 234000) // second file
callback(512, 234000) // second file
...
callback(30, 234000) // second file
Does this explain it more clear? If not, just ask.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] fix progress callback for ra_svn
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Küng wrote on Sat, 2 Aug 2008 at 12:40 +0200:
> Daniel Shahaf wrote:
> > Stefan Küng wrote on Sat, 2 Aug 2008 at 12:27 +0200:
> > > Stefan Sperling wrote:
> > > > The API docs for svn_ra_progress_notify_func_t do not say
> > > > whether the callback should pass the sum of bytes transferred
> > > > so far, or just the amount of bytes transferred since the last
> > > > time the callback was called:
> > > >
> > > > /**
> > > > * Callback function type for progress notification.
> > > > *
> > > > * @a progress is the number of bytes already transferred, @a total is
> >
> > As I read it, it promises the (current) ra_svn behaviour.
>
> Yes, but it works not the same way as neon and serf. Since the callback for
> neon is actually done in the neon library, we can't change that.
We could write a wrapper callback that manipulates (sums) what neon
reports.
> Neon does it this way because it can 'restart' when needed. For example, most
> of the time the total is not known beforehand. But sometimes it is. Neon then
> fills in the 'total' param (instead of passing -1 there) and starts counting
> the bytes again.
>
I'm not familiar with 'restarts' in this context. From your description
it sounds that they have to do with how neon calculates the number of
bytes it has transferred, not with whether it chooses to report that
number directly or as a difference since the preceding reported amount.
Am I missing something?
> So, I think the way neon/serf handle this is the correct way, even though the
> docs string says otherwise and ra_svn does it that way.
>
Re: [PATCH] fix progress callback for ra_svn
Posted by Stefan Küng <to...@gmail.com>.
Daniel Shahaf wrote:
> Stefan Küng wrote on Sat, 2 Aug 2008 at 12:27 +0200:
>> Stefan Sperling wrote:
>>> The API docs for svn_ra_progress_notify_func_t do not say
>>> whether the callback should pass the sum of bytes transferred
>>> so far, or just the amount of bytes transferred since the last
>>> time the callback was called:
>>>
>>> /**
>>> * Callback function type for progress notification.
>>> *
>>> * @a progress is the number of bytes already transferred, @a total is
>
> As I read it, it promises the (current) ra_svn behaviour.
Yes, but it works not the same way as neon and serf. Since the callback
for neon is actually done in the neon library, we can't change that.
Neon does it this way because it can 'restart' when needed. For example,
most of the time the total is not known beforehand. But sometimes it is.
Neon then fills in the 'total' param (instead of passing -1 there) and
starts counting the bytes again.
So, I think the way neon/serf handle this is the correct way, even
though the docs string says otherwise and ra_svn does it that way.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] fix progress callback for ra_svn
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Küng wrote on Sat, 2 Aug 2008 at 12:27 +0200:
> Stefan Sperling wrote:
> > The API docs for svn_ra_progress_notify_func_t do not say
> > whether the callback should pass the sum of bytes transferred
> > so far, or just the amount of bytes transferred since the last
> > time the callback was called:
> >
> > /**
> > * Callback function type for progress notification.
> > *
> > * @a progress is the number of bytes already transferred, @a total is
As I read it, it promises the (current) ra_svn behaviour.
2¢,
Daniel
> > * 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,
> > apr_pool_t *pool);
> >
> > We should probably make the API docs explicit as well as making
> > all RA layers do consistent reporting.
>
> New patch attached, this time with correction to the doc string.
>
> Stefan
>
>
Re: [PATCH] fix progress callback for ra_svn
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Aug 02, 2008 at 12:27:15PM +0200, Stefan Küng wrote:
> New patch attached, this time with correction to the doc string.
I think the fix is correct, but I'd like to have another committer's
OK before committing this. I'm not sure how the callback was originally
intended to be used, and whether maybe a different fix is needed.
Opinions?
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix progress callback for ra_svn
Posted by Stefan Küng <to...@gmail.com>.
Stefan Sperling wrote:
> On Sat, Aug 02, 2008 at 09:40:17AM +0200, Stefan Küng wrote:
>> Hi,
>>
>> Some users reported that TSVN shows unbelievable amounts of bytes
>> transferred for all operations, but only when used with svn://.
>>
>> I've tried ra_serf and ra_neon, those work ok. But ra_svn passes the sum
>> of bytes transferred to the progress callback, while neon and serf pass
>> only the bytes just transferred to the callback (they don't sum up the
>> transferred bytes).
>>
>> I'd like to have all three layers call the progress callback the same
>> way. Since adjusting ra_neon to do the same as ra_svn does would require
>> a change in the neon library, I think it's best to change ra_svn (also,
>> the callback was inspired by the neon progress callback, so I'd say that
>> it's ra_svn which does it wrong).
>>
>> the attached patch fixes this.
>
> The API docs for svn_ra_progress_notify_func_t do not say
> whether the callback should pass the sum of bytes transferred
> so far, or just the amount of bytes transferred since the last
> time the callback was called:
>
> /**
> * 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,
> apr_pool_t *pool);
>
> We should probably make the API docs explicit as well as making
> all RA layers do consistent reporting.
New patch attached, this time with correction to the doc string.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] fix progress callback for ra_svn
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Aug 02, 2008 at 09:40:17AM +0200, Stefan Küng wrote:
> Hi,
>
> Some users reported that TSVN shows unbelievable amounts of bytes
> transferred for all operations, but only when used with svn://.
>
> I've tried ra_serf and ra_neon, those work ok. But ra_svn passes the sum
> of bytes transferred to the progress callback, while neon and serf pass
> only the bytes just transferred to the callback (they don't sum up the
> transferred bytes).
>
> I'd like to have all three layers call the progress callback the same
> way. Since adjusting ra_neon to do the same as ra_svn does would require
> a change in the neon library, I think it's best to change ra_svn (also,
> the callback was inspired by the neon progress callback, so I'd say that
> it's ra_svn which does it wrong).
>
> the attached patch fixes this.
The API docs for svn_ra_progress_notify_func_t do not say
whether the callback should pass the sum of bytes transferred
so far, or just the amount of bytes transferred since the last
time the callback was called:
/**
* 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,
apr_pool_t *pool);
We should probably make the API docs explicit as well as making
all RA layers do consistent reporting.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org