You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@xbc.nu> on 2004/04/28 07:56:26 UTC
Re: svn commit: r9485 - in trunk/subversion: libsvn_delta libsvn_fs
tests/libsvn_delta
ghudson@tigris.org wrote:
>Author: ghudson
>Date: Sat Apr 24 19:28:41 2004
>New Revision: 9485
>
>Modified:
> trunk/subversion/libsvn_delta/compose_delta.c
> trunk/subversion/libsvn_delta/delta.h
> trunk/subversion/libsvn_fs/reps-strings.c
> trunk/subversion/tests/libsvn_delta/random-test.c
>Log:
>Simplify the delta combiner interface.
>
>
>Modified: trunk/subversion/libsvn_fs/reps-strings.c
>==============================================================================
>--- trunk/subversion/libsvn_fs/reps-strings.c (original)
>+++ trunk/subversion/libsvn_fs/reps-strings.c Sat Apr 24 19:28:41 2004
>@@ -183,30 +183,12 @@
> /* Combine the incoming window with whatever's in the baton. */
> apr_pool_t *composite_pool = svn_pool_create (cb->trail->pool);
> svn_txdelta_window_t *composite;
>- svn_txdelta__compose_ctx_t context = { 0 };
>
>- composite = svn_txdelta__compose_windows
>- (window, cb->window, &context, composite_pool);
>-
>- if (composite)
>- {
>- svn_pool_destroy (cb->window_pool);
>- cb->window = composite;
>- cb->window_pool = composite_pool;
>- }
>- else if (context.use_second)
>- {
>- svn_pool_destroy (composite_pool);
>- cb->window->sview_offset = context.sview_offset;
>- cb->window->sview_len = context.sview_len;
>-
>- /* This can only happen if the window doesn't touch
>- source data; so ... */
>- cb->done = TRUE;
>- }
>- else
>- /* Can't happen, because cb->window can't be NULL. */
>- abort ();
>+ composite = svn_txdelta__compose_windows (window, cb->window,
>+ composite_pool);
>+ svn_pool_destroy (cb->window_pool);
>+ cb->window = composite;
>+ cb->window_pool = composite_pool;
> }
> else if (window)
> {
>
>
A consequence of removing the baton and checks from compose_window is
that now compose_handler only checks B->src_ops when it sets up the
initial baton, but those are never checked later. Which means that if a
windoe in the middle of the delta uses no source ops, we'll still do the
combination. Given your cnahges in this commit, I think
svn_txdelta__compose_windows should check B->src_ops. I'd also change
the allocation guarantee slightly, so that compose_windows can return B
without having to copy it to the pool (the compose_handler
implementation already assumes so, but that't not documented).
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9485 - in trunk/subversion: libsvn_delta libsvn_fs
tests/libsvn_delta
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
>On Wed, 2004-04-28 at 03:56, Branko Čibej wrote:
>
>
>>A consequence of removing the baton and checks from compose_window is
>>that now compose_handler only checks B->src_ops when it sets up the
>>initial baton, but those are never checked later. Which means that if a
>>windoe in the middle of the delta uses no source ops, we'll still do the
>>combination. Given your cnahges in this commit, I think
>>svn_txdelta__compose_windows should check B->src_ops. I'd also change
>>the allocation guarantee slightly, so that compose_windows can return B
>>without having to copy it to the pool (the compose_handler
>>implementation already assumes so, but that't not documented).
>>
>>
>
>Please see http://www.contactor.se/~dast/svn/archive-2004-04/1183.shtml
>if you missed it the first time around.
>
>
Yup, read that; unfortunately didn't have time to respond before.
>I do think that you're correct, and I misread the FS code I was
>modifying. But I think the right fix is more along the lines of:
>
>Index: subversion/libsvn_fs/reps-strings.c
>===================================================================
>--- subversion/libsvn_fs/reps-strings.c (revision 9551)
>+++ subversion/libsvn_fs/reps-strings.c (working copy)
>@@ -189,6 +189,7 @@
> svn_pool_destroy (cb->window_pool);
> cb->window = composite;
> cb->window_pool = composite_pool;
>+ cb->done = (composite->sview_len == 0 || composite->src_ops == 0);
> }
> else if (window)
> {
>
>Does that seem reasonable?
>
>
I personaly prefer functions to be self-contained rather than requiring
their clients to handle preconditions. In this case I have a hard time
finding any other argument, as long as libsvn_fs is the only client of
svn_delta__compose_windows.
Since these are internal interfaces, I can live with this solution. +1.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9485 - in trunk/subversion: libsvn_delta
libsvn_fs tests/libsvn_delta
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2004-04-28 at 03:56, Branko Čibej wrote:
> A consequence of removing the baton and checks from compose_window is
> that now compose_handler only checks B->src_ops when it sets up the
> initial baton, but those are never checked later. Which means that if a
> windoe in the middle of the delta uses no source ops, we'll still do the
> combination. Given your cnahges in this commit, I think
> svn_txdelta__compose_windows should check B->src_ops. I'd also change
> the allocation guarantee slightly, so that compose_windows can return B
> without having to copy it to the pool (the compose_handler
> implementation already assumes so, but that't not documented).
Please see http://www.contactor.se/~dast/svn/archive-2004-04/1183.shtml
if you missed it the first time around.
I do think that you're correct, and I misread the FS code I was
modifying. But I think the right fix is more along the lines of:
Index: subversion/libsvn_fs/reps-strings.c
===================================================================
--- subversion/libsvn_fs/reps-strings.c (revision 9551)
+++ subversion/libsvn_fs/reps-strings.c (working copy)
@@ -189,6 +189,7 @@
svn_pool_destroy (cb->window_pool);
cb->window = composite;
cb->window_pool = composite_pool;
+ cb->done = (composite->sview_len == 0 || composite->src_ops == 0);
}
else if (window)
{
Does that seem reasonable?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org