You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2012/06/10 23:34:22 UTC
svn commit: r1348666 - in /subversion/trunk/subversion: include/
libsvn_delta/ libsvn_fs_fs/ libsvn_ra/ tests/libsvn_delta/
Author: stefan2
Date: Sun Jun 10 21:34:21 2012
New Revision: 1348666
URL: http://svn.apache.org/viewvc?rev=1348666&view=rev
Log:
When handing out node contents, the delta streams don't need
to calculate MD5 checksums as the result will not be used and the
check would be redundant even if it were made.
Thus, rev the svn_txdelta API to calculate the checksum only
upon specific request and update all callers to use the new API.
* subversion/include/svn_delta.h
(svn_txdelta2): declare new, extended API
(svn_txdelta): deprecate the old one
* subversion/libsvn_delta/text_delta.c
(txdelta_baton): extend docstring
(txdelta_md5_digest): handle the no-checksum-requested case
(svn_txdelta2): implement
(svn_txdelta): re-implement in terms of svn_txdelta2
* subversion/libsvn_fs_fs/fs_fs.c
(svn_fs_fs__get_file_delta_stream): eliminate duplicate MD5 calculation
* subversion/libsvn_ra/compat.c
(svn_ra__file_revs_from_log): switch to reved API, MD5 not used / required
* subversion/tests/libsvn_delta/random-test.c
(random_test): ditto
* subversion/tests/libsvn_delta/svndiff-test.c
(main): same here
* subversion/tests/libsvn_delta/vdelta-test.c
(do_one_diff, main): and here
* subversion/tests/libsvn_delta/window-test.c
(stream_window_test): switch to reved API, request MD5
Modified:
subversion/trunk/subversion/include/svn_delta.h
subversion/trunk/subversion/libsvn_delta/text_delta.c
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
subversion/trunk/subversion/libsvn_ra/compat.c
subversion/trunk/subversion/tests/libsvn_delta/random-test.c
subversion/trunk/subversion/tests/libsvn_delta/svndiff-test.c
subversion/trunk/subversion/tests/libsvn_delta/vdelta-test.c
subversion/trunk/subversion/tests/libsvn_delta/window-test.c
Modified: subversion/trunk/subversion/include/svn_delta.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_delta.h?rev=1348666&r1=1348665&r2=1348666&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_delta.h (original)
+++ subversion/trunk/subversion/include/svn_delta.h Sun Jun 10 21:34:21 2012
@@ -367,10 +367,26 @@ svn_txdelta_md5_digest(svn_txdelta_strea
*
* @a source and @a target are both readable generic streams. When we call
* svn_txdelta_next_window() on @a *stream, it will read from @a source and
- * @a target to gather as much data as it needs.
+ * @a target to gather as much data as it needs. If @a calculate_checksum
+ * is set, you may call @ref svn_txdelta_md5_digest to get an MD5 checksum
+ * for @a target.
*
* Do any necessary allocation in a sub-pool of @a pool.
+ *
+ * @since New in 1.8.
+ */
+void
+svn_txdelta2(svn_txdelta_stream_t **stream,
+ svn_stream_t *source,
+ svn_stream_t *target,
+ svn_boolean_t calculate_checksum,
+ apr_pool_t *pool);
+
+/** Similar to svn_txdelta2 but always calculating the target checksum.
+ *
+ * @deprecated Provided for backward compatibility with the 1.7 API.
*/
+SVN_DEPRECATED
void
svn_txdelta(svn_txdelta_stream_t **stream,
svn_stream_t *source,
Modified: subversion/trunk/subversion/libsvn_delta/text_delta.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_delta/text_delta.c?rev=1348666&r1=1348665&r2=1348666&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_delta/text_delta.c (original)
+++ subversion/trunk/subversion/libsvn_delta/text_delta.c Sun Jun 10 21:34:21 2012
@@ -57,7 +57,8 @@ struct txdelta_baton {
svn_filesize_t pos; /* Offset of next read in source file. */
char *buf; /* Buffer for input data. */
- svn_checksum_ctx_t *context; /* Context for computing the checksum. */
+ svn_checksum_ctx_t *context; /* If not NULL, the context for computing
+ the checksum. */
svn_checksum_t *checksum; /* If non-NULL, the checksum of TARGET. */
apr_pool_t *result_pool; /* For results (e.g. checksum) */
@@ -405,6 +406,10 @@ txdelta_md5_digest(void *baton)
if (b->more)
return NULL;
+ /* If checksumming has not been activated, there will be no digest. */
+ if (b->context == NULL)
+ return NULL;
+
/* The checksum should be there. */
return b->checksum->digest;
}
@@ -463,10 +468,11 @@ svn_txdelta_run(svn_stream_t *source,
void
-svn_txdelta(svn_txdelta_stream_t **stream,
- svn_stream_t *source,
- svn_stream_t *target,
- apr_pool_t *pool)
+svn_txdelta2(svn_txdelta_stream_t **stream,
+ svn_stream_t *source,
+ svn_stream_t *target,
+ svn_boolean_t calculate_checksum,
+ apr_pool_t *pool)
{
struct txdelta_baton *b = apr_pcalloc(pool, sizeof(*b));
@@ -475,13 +481,24 @@ svn_txdelta(svn_txdelta_stream_t **strea
b->more_source = TRUE;
b->more = TRUE;
b->buf = apr_palloc(pool, 2 * SVN_DELTA_WINDOW_SIZE);
- b->context = svn_checksum_ctx_create(svn_checksum_md5, pool);
+ b->context = calculate_checksum
+ ? svn_checksum_ctx_create(svn_checksum_md5, pool)
+ : NULL;
b->result_pool = pool;
*stream = svn_txdelta_stream_create(b, txdelta_next_window,
txdelta_md5_digest, pool);
}
+void
+svn_txdelta(svn_txdelta_stream_t **stream,
+ svn_stream_t *source,
+ svn_stream_t *target,
+ apr_pool_t *pool)
+{
+ return svn_txdelta2(stream, source, target, TRUE, pool);
+}
+
/* Functions for implementing a "target push" delta. */
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1348666&r1=1348665&r2=1348666&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sun Jun 10 21:34:21 2012
@@ -4115,7 +4115,11 @@ svn_fs_fs__get_file_delta_stream(svn_txd
else
source_stream = svn_stream_empty(pool);
SVN_ERR(read_representation(&target_stream, fs, target->data_rep, pool));
- svn_txdelta(stream_p, source_stream, target_stream, pool);
+
+ /* Because source and target stream will already verify their content,
+ * there is no need to do this once more. In particular if the stream
+ * content is being fetched from cache. */
+ svn_txdelta2(stream_p, source_stream, target_stream, FALSE, pool);
return SVN_NO_ERROR;
}
Modified: subversion/trunk/subversion/libsvn_ra/compat.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra/compat.c?rev=1348666&r1=1348665&r2=1348666&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra/compat.c (original)
+++ subversion/trunk/subversion/libsvn_ra/compat.c Sun Jun 10 21:34:21 2012
@@ -757,8 +757,9 @@ svn_ra__file_revs_from_log(svn_ra_sessio
/* Compute and send delta if client asked for it. */
if (delta_handler)
{
- /* Get the content delta. */
- svn_txdelta(&delta_stream, last_stream, stream, lastpool);
+ /* Get the content delta. Don't calculate checksums as we don't
+ * use them. */
+ svn_txdelta2(&delta_stream, last_stream, stream, FALSE, lastpool);
/* And send. */
SVN_ERR(svn_txdelta_send_txstream(delta_stream, delta_handler,
Modified: subversion/trunk/subversion/tests/libsvn_delta/random-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_delta/random-test.c?rev=1348666&r1=1348665&r2=1348666&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_delta/random-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_delta/random-test.c Sun Jun 10 21:34:21 2012
@@ -336,10 +336,11 @@ random_test(apr_pool_t *pool)
delta_pool);
/* Make stage 1: create the text delta. */
- svn_txdelta(&txdelta_stream,
- svn_stream_from_aprfile(source, delta_pool),
- svn_stream_from_aprfile(target, delta_pool),
- delta_pool);
+ svn_txdelta2(&txdelta_stream,
+ svn_stream_from_aprfile(source, delta_pool),
+ svn_stream_from_aprfile(target, delta_pool),
+ FALSE,
+ delta_pool);
SVN_ERR(svn_txdelta_send_txstream(txdelta_stream,
handler,
@@ -421,15 +422,17 @@ do_random_combine_test(apr_pool_t *pool,
/* Make stage 1: create the text deltas. */
- svn_txdelta(&txdelta_stream_A,
- svn_stream_from_aprfile(source, delta_pool),
- svn_stream_from_aprfile(middle, delta_pool),
- delta_pool);
-
- svn_txdelta(&txdelta_stream_B,
- svn_stream_from_aprfile(middle_copy, delta_pool),
- svn_stream_from_aprfile(target, delta_pool),
- delta_pool);
+ svn_txdelta2(&txdelta_stream_A,
+ svn_stream_from_aprfile(source, delta_pool),
+ svn_stream_from_aprfile(middle, delta_pool),
+ FALSE,
+ delta_pool);
+
+ svn_txdelta2(&txdelta_stream_B,
+ svn_stream_from_aprfile(middle_copy, delta_pool),
+ svn_stream_from_aprfile(target, delta_pool),
+ FALSE,
+ delta_pool);
{
svn_txdelta_window_t *window_A;
Modified: subversion/trunk/subversion/tests/libsvn_delta/svndiff-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_delta/svndiff-test.c?rev=1348666&r1=1348665&r2=1348666&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_delta/svndiff-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_delta/svndiff-test.c Sun Jun 10 21:34:21 2012
@@ -75,10 +75,11 @@ main(int argc, char **argv)
if (argc == 4)
version = atoi(argv[3]);
- svn_txdelta(&txdelta_stream,
- svn_stream_from_aprfile(source_file, pool),
- svn_stream_from_aprfile(target_file, pool),
- pool);
+ svn_txdelta2(&txdelta_stream,
+ svn_stream_from_aprfile(source_file, pool),
+ svn_stream_from_aprfile(target_file, pool),
+ FALSE,
+ pool);
err = svn_stream_for_stdout(&stdout_stream, pool);
if (err)
Modified: subversion/trunk/subversion/tests/libsvn_delta/vdelta-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_delta/vdelta-test.c?rev=1348666&r1=1348665&r2=1348666&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_delta/vdelta-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_delta/vdelta-test.c Sun Jun 10 21:34:21 2012
@@ -60,10 +60,11 @@ do_one_diff(apr_file_t *source_file, apr
*count = 0;
*len = 0;
- svn_txdelta(&delta_stream,
- svn_stream_from_aprfile(source_file, fpool),
- svn_stream_from_aprfile(target_file, fpool),
- fpool);
+ svn_txdelta2(&delta_stream,
+ svn_stream_from_aprfile(source_file, fpool),
+ svn_stream_from_aprfile(target_file, fpool),
+ FALSE,
+ fpool);
do {
svn_error_t *err;
err = svn_txdelta_next_window(&delta_window, delta_stream, wpool);
@@ -124,14 +125,16 @@ do_one_test_cycle(apr_file_t *source_fil
apr_file_seek(target_file_B, APR_SET, &offset);
}
- svn_txdelta(&stream_A,
- svn_stream_from_aprfile(source_file_A, fpool),
- svn_stream_from_aprfile(target_file_A, fpool),
- fpool);
- svn_txdelta(&stream_B,
- svn_stream_from_aprfile(source_file_B, fpool),
- svn_stream_from_aprfile(target_file_B, fpool),
- fpool);
+ svn_txdelta2(&stream_A,
+ svn_stream_from_aprfile(source_file_A, fpool),
+ svn_stream_from_aprfile(target_file_A, fpool),
+ FALSE,
+ fpool);
+ svn_txdelta2(&stream_B,
+ svn_stream_from_aprfile(source_file_B, fpool),
+ svn_stream_from_aprfile(target_file_B, fpool),
+ FALSE,
+ fpool);
for (count_AB = 0; count_AB < count_B; ++count_AB)
{
Modified: subversion/trunk/subversion/tests/libsvn_delta/window-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_delta/window-test.c?rev=1348666&r1=1348665&r2=1348666&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_delta/window-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_delta/window-test.c Sun Jun 10 21:34:21 2012
@@ -70,7 +70,7 @@ stream_window_test(apr_pool_t *pool)
target_str.len = 109000;
target_stream = svn_stream_from_string(&target_str, pool);
- svn_txdelta(&txstream, source_stream, target_stream, pool);
+ svn_txdelta2(&txstream, source_stream, target_stream, TRUE, pool);
while (1)
{
Re: API simplification opportunity Re: svn commit: r1348666 - in
/subversion/trunk/subversion: include/ libsvn_delta/ libsvn_fs_fs/
libsvn_ra/ tests/libsvn_delta/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Sun, Oct 21, 2012 at 19:43:16 +0200:
> On Sun, Oct 21, 2012 at 5:55 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
>
> > Stefan Fuhrmann wrote on Sat, Oct 20, 2012 at 15:14:11 +0200:
> > > On Thu, Oct 18, 2012 at 10:22 PM, Daniel Shahaf <d.s@daniel.shahaf.name
> > >wrote:
> > >
> > > > Stefan Fuhrmann wrote on Thu, Oct 18, 2012 at 02:56:15 +0200:
> > > > > On Tue, Oct 16, 2012 at 12:55 AM, Daniel Shahaf <
> > d.s@daniel.shahaf.name
> > > > >wrote:
> > > > >
> > > > > > stefan2@apache.org wrote on Sun, Jun 10, 2012 at 21:34:22 -0000:
> > > > > > > Author: stefan2
> > > > > > > Date: Sun Jun 10 21:34:21 2012
> > > > > > > New Revision: 1348666
> > > > > > >
> > > > > > > URL: http://svn.apache.org/viewvc?rev=1348666&view=rev
> > > > > > > Log:
> > > > > > > When handing out node contents, the delta streams don't need
> > > > > > > to calculate MD5 checksums as the result will not be used and the
> > > > > > > check would be redundant even if it were made.
> > > > > > >
> > > > > > > Thus, rev the svn_txdelta API to calculate the checksum only
> > > > > > > upon specific request and update all callers to use the new API.
> > > > > > >
> > > > > > ...
> > > > > > > Modified: subversion/trunk/subversion/include/svn_delta.h
> > > > > > > URL:
> > > > > >
> > > >
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_delta.h?rev=1348666&r1=1348665&r2=1348666&view=diff
> > > > > > >
> > > > > >
> > > >
> > ==============================================================================
> > > > > > > --- subversion/trunk/subversion/include/svn_delta.h (original)
> > > > > > > +++ subversion/trunk/subversion/include/svn_delta.h Sun Jun 10
> > > > 21:34:21
> > > > > > 2012
> > > > > > > @@ -367,10 +367,26 @@ svn_txdelta_md5_digest(svn_txdelta_strea
> > > > > > > + * If @a calculate_checksum
> > > > > > > + * is set, you may call @ref svn_txdelta_md5_digest to get an
> > MD5
> > > > > > checksum
> > > > > > > + * for @a target.
> > > > > > > *
> > > > > > > * Do any necessary allocation in a sub-pool of @a pool.
> > > > > > > + *
> > > > > > > + * @since New in 1.8.
> > > > > > > + */
> > > > > > > +void
> > > > > > > +svn_txdelta2(svn_txdelta_stream_t **stream,
> > > > > > > + svn_stream_t *source,
> > > > > > > + svn_stream_t *target,
> > > > > > > + svn_boolean_t calculate_checksum,
> > > > > >
> > > > > > As of today, only svn_txdelta() and window-test.c pass TRUE for @a
> > > > > > calculate_checksum. Do we anticipate needing to pass TRUE For it
> > in
> > > > any
> > > > > > new code?
> > > > > >
> > > > >
> > > > > No, I don't anticipate extra need for that parameter
> > > > > neither do I feel that it is useless. How would you
> > > > > implement svn_txdelta() without that parameter?
> > > >
> > >
> > > Hm .. wrong question :/
> > >
> > > I would remove the parameter from the public API only. Internally we
> > > > will define both svn_txdelta() and svn_txdelta2() as one-line wrappers
> > > > around some third function which does take an 'svn_boolean_t
> > > > calculate_checksum' parameter.
> > > >
> > >
> > > That would only be o.k., if that internal function was internal
> > > to libsvn_delta. But we would need to call it from other libs
> > > which I see as a strong indicator that others might want this
> > > functionality, too.
> >
> > We wouldn't need to call the internal function from other libs.
> >
> > static void
> > foo_internal(svn_txdelta_stream_t **stream,
> > svn_stream_t *source,
> > svn_stream_t *target,
> > svn_boolean_t calculate_checksum,
> > apr_pool_t *pool);
> >
> > void
> > svn_txdelta2(stream, source, target, pool) {
> > foo_internal(stream, source, target, FALSE, pool);
> > }
> >
> > void
> > svn_txdelta(stream, source, target, pool) {
> > foo_internal(stream, source, target, TRUE, pool);
> > }
> >
>
> We seem to talk past each other here. My problem
> is that the BDB / FSFS libs would need to call the
> *deprecated* svn_txdelta because they need the MD5
> checksum.
Odd. I thought that there were no calls to svn_txdelta() left in the
codebase, and that all calls to svn_txdelta2() passed FALSE for the new
boolean parameter.
Anyway, shrug. I think this thread is too long for the prospective
benefit (one fewer boolean parameter on a 5-ary function).
Cheers
Daniel
Re: API simplification opportunity Re: svn commit: r1348666 - in
/subversion/trunk/subversion: include/ libsvn_delta/ libsvn_fs_fs/ libsvn_ra/ tests/libsvn_delta/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Oct 21, 2012 at 5:55 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> Stefan Fuhrmann wrote on Sat, Oct 20, 2012 at 15:14:11 +0200:
> > On Thu, Oct 18, 2012 at 10:22 PM, Daniel Shahaf <d.s@daniel.shahaf.name
> >wrote:
> >
> > > Stefan Fuhrmann wrote on Thu, Oct 18, 2012 at 02:56:15 +0200:
> > > > On Tue, Oct 16, 2012 at 12:55 AM, Daniel Shahaf <
> d.s@daniel.shahaf.name
> > > >wrote:
> > > >
> > > > > stefan2@apache.org wrote on Sun, Jun 10, 2012 at 21:34:22 -0000:
> > > > > > Author: stefan2
> > > > > > Date: Sun Jun 10 21:34:21 2012
> > > > > > New Revision: 1348666
> > > > > >
> > > > > > URL: http://svn.apache.org/viewvc?rev=1348666&view=rev
> > > > > > Log:
> > > > > > When handing out node contents, the delta streams don't need
> > > > > > to calculate MD5 checksums as the result will not be used and the
> > > > > > check would be redundant even if it were made.
> > > > > >
> > > > > > Thus, rev the svn_txdelta API to calculate the checksum only
> > > > > > upon specific request and update all callers to use the new API.
> > > > > >
> > > > > ...
> > > > > > Modified: subversion/trunk/subversion/include/svn_delta.h
> > > > > > URL:
> > > > >
> > >
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_delta.h?rev=1348666&r1=1348665&r2=1348666&view=diff
> > > > > >
> > > > >
> > >
> ==============================================================================
> > > > > > --- subversion/trunk/subversion/include/svn_delta.h (original)
> > > > > > +++ subversion/trunk/subversion/include/svn_delta.h Sun Jun 10
> > > 21:34:21
> > > > > 2012
> > > > > > @@ -367,10 +367,26 @@ svn_txdelta_md5_digest(svn_txdelta_strea
> > > > > > + * If @a calculate_checksum
> > > > > > + * is set, you may call @ref svn_txdelta_md5_digest to get an
> MD5
> > > > > checksum
> > > > > > + * for @a target.
> > > > > > *
> > > > > > * Do any necessary allocation in a sub-pool of @a pool.
> > > > > > + *
> > > > > > + * @since New in 1.8.
> > > > > > + */
> > > > > > +void
> > > > > > +svn_txdelta2(svn_txdelta_stream_t **stream,
> > > > > > + svn_stream_t *source,
> > > > > > + svn_stream_t *target,
> > > > > > + svn_boolean_t calculate_checksum,
> > > > >
> > > > > As of today, only svn_txdelta() and window-test.c pass TRUE for @a
> > > > > calculate_checksum. Do we anticipate needing to pass TRUE For it
> in
> > > any
> > > > > new code?
> > > > >
> > > >
> > > > No, I don't anticipate extra need for that parameter
> > > > neither do I feel that it is useless. How would you
> > > > implement svn_txdelta() without that parameter?
> > >
> >
> > Hm .. wrong question :/
> >
> > I would remove the parameter from the public API only. Internally we
> > > will define both svn_txdelta() and svn_txdelta2() as one-line wrappers
> > > around some third function which does take an 'svn_boolean_t
> > > calculate_checksum' parameter.
> > >
> >
> > That would only be o.k., if that internal function was internal
> > to libsvn_delta. But we would need to call it from other libs
> > which I see as a strong indicator that others might want this
> > functionality, too.
>
> We wouldn't need to call the internal function from other libs.
>
> static void
> foo_internal(svn_txdelta_stream_t **stream,
> svn_stream_t *source,
> svn_stream_t *target,
> svn_boolean_t calculate_checksum,
> apr_pool_t *pool);
>
> void
> svn_txdelta2(stream, source, target, pool) {
> foo_internal(stream, source, target, FALSE, pool);
> }
>
> void
> svn_txdelta(stream, source, target, pool) {
> foo_internal(stream, source, target, TRUE, pool);
> }
>
We seem to talk past each other here. My problem
is that the BDB / FSFS libs would need to call the
*deprecated* svn_txdelta because they need the MD5
checksum.
-- Stefan^2.
--
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*
Re: API simplification opportunity Re: svn commit: r1348666 - in
/subversion/trunk/subversion: include/ libsvn_delta/ libsvn_fs_fs/
libsvn_ra/ tests/libsvn_delta/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Sat, Oct 20, 2012 at 15:14:11 +0200:
> On Thu, Oct 18, 2012 at 10:22 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
>
> > Stefan Fuhrmann wrote on Thu, Oct 18, 2012 at 02:56:15 +0200:
> > > On Tue, Oct 16, 2012 at 12:55 AM, Daniel Shahaf <d.s@daniel.shahaf.name
> > >wrote:
> > >
> > > > stefan2@apache.org wrote on Sun, Jun 10, 2012 at 21:34:22 -0000:
> > > > > Author: stefan2
> > > > > Date: Sun Jun 10 21:34:21 2012
> > > > > New Revision: 1348666
> > > > >
> > > > > URL: http://svn.apache.org/viewvc?rev=1348666&view=rev
> > > > > Log:
> > > > > When handing out node contents, the delta streams don't need
> > > > > to calculate MD5 checksums as the result will not be used and the
> > > > > check would be redundant even if it were made.
> > > > >
> > > > > Thus, rev the svn_txdelta API to calculate the checksum only
> > > > > upon specific request and update all callers to use the new API.
> > > > >
> > > > ...
> > > > > Modified: subversion/trunk/subversion/include/svn_delta.h
> > > > > URL:
> > > >
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_delta.h?rev=1348666&r1=1348665&r2=1348666&view=diff
> > > > >
> > > >
> > ==============================================================================
> > > > > --- subversion/trunk/subversion/include/svn_delta.h (original)
> > > > > +++ subversion/trunk/subversion/include/svn_delta.h Sun Jun 10
> > 21:34:21
> > > > 2012
> > > > > @@ -367,10 +367,26 @@ svn_txdelta_md5_digest(svn_txdelta_strea
> > > > > + * If @a calculate_checksum
> > > > > + * is set, you may call @ref svn_txdelta_md5_digest to get an MD5
> > > > checksum
> > > > > + * for @a target.
> > > > > *
> > > > > * Do any necessary allocation in a sub-pool of @a pool.
> > > > > + *
> > > > > + * @since New in 1.8.
> > > > > + */
> > > > > +void
> > > > > +svn_txdelta2(svn_txdelta_stream_t **stream,
> > > > > + svn_stream_t *source,
> > > > > + svn_stream_t *target,
> > > > > + svn_boolean_t calculate_checksum,
> > > >
> > > > As of today, only svn_txdelta() and window-test.c pass TRUE for @a
> > > > calculate_checksum. Do we anticipate needing to pass TRUE For it in
> > any
> > > > new code?
> > > >
> > >
> > > No, I don't anticipate extra need for that parameter
> > > neither do I feel that it is useless. How would you
> > > implement svn_txdelta() without that parameter?
> >
>
> Hm .. wrong question :/
>
> I would remove the parameter from the public API only. Internally we
> > will define both svn_txdelta() and svn_txdelta2() as one-line wrappers
> > around some third function which does take an 'svn_boolean_t
> > calculate_checksum' parameter.
> >
>
> That would only be o.k., if that internal function was internal
> to libsvn_delta. But we would need to call it from other libs
> which I see as a strong indicator that others might want this
> functionality, too.
We wouldn't need to call the internal function from other libs.
static void
foo_internal(svn_txdelta_stream_t **stream,
svn_stream_t *source,
svn_stream_t *target,
svn_boolean_t calculate_checksum,
apr_pool_t *pool);
void
svn_txdelta2(stream, source, target, pool) {
foo_internal(stream, source, target, FALSE, pool);
}
void
svn_txdelta(stream, source, target, pool) {
foo_internal(stream, source, target, TRUE, pool);
}
Re: API simplification opportunity Re: svn commit: r1348666 - in
/subversion/trunk/subversion: include/ libsvn_delta/ libsvn_fs_fs/ libsvn_ra/ tests/libsvn_delta/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Oct 18, 2012 at 10:22 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> Stefan Fuhrmann wrote on Thu, Oct 18, 2012 at 02:56:15 +0200:
> > On Tue, Oct 16, 2012 at 12:55 AM, Daniel Shahaf <d.s@daniel.shahaf.name
> >wrote:
> >
> > > stefan2@apache.org wrote on Sun, Jun 10, 2012 at 21:34:22 -0000:
> > > > Author: stefan2
> > > > Date: Sun Jun 10 21:34:21 2012
> > > > New Revision: 1348666
> > > >
> > > > URL: http://svn.apache.org/viewvc?rev=1348666&view=rev
> > > > Log:
> > > > When handing out node contents, the delta streams don't need
> > > > to calculate MD5 checksums as the result will not be used and the
> > > > check would be redundant even if it were made.
> > > >
> > > > Thus, rev the svn_txdelta API to calculate the checksum only
> > > > upon specific request and update all callers to use the new API.
> > > >
> > > ...
> > > > Modified: subversion/trunk/subversion/include/svn_delta.h
> > > > URL:
> > >
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_delta.h?rev=1348666&r1=1348665&r2=1348666&view=diff
> > > >
> > >
> ==============================================================================
> > > > --- subversion/trunk/subversion/include/svn_delta.h (original)
> > > > +++ subversion/trunk/subversion/include/svn_delta.h Sun Jun 10
> 21:34:21
> > > 2012
> > > > @@ -367,10 +367,26 @@ svn_txdelta_md5_digest(svn_txdelta_strea
> > > > + * If @a calculate_checksum
> > > > + * is set, you may call @ref svn_txdelta_md5_digest to get an MD5
> > > checksum
> > > > + * for @a target.
> > > > *
> > > > * Do any necessary allocation in a sub-pool of @a pool.
> > > > + *
> > > > + * @since New in 1.8.
> > > > + */
> > > > +void
> > > > +svn_txdelta2(svn_txdelta_stream_t **stream,
> > > > + svn_stream_t *source,
> > > > + svn_stream_t *target,
> > > > + svn_boolean_t calculate_checksum,
> > >
> > > As of today, only svn_txdelta() and window-test.c pass TRUE for @a
> > > calculate_checksum. Do we anticipate needing to pass TRUE For it in
> any
> > > new code?
> > >
> >
> > No, I don't anticipate extra need for that parameter
> > neither do I feel that it is useless. How would you
> > implement svn_txdelta() without that parameter?
>
Hm .. wrong question :/
I would remove the parameter from the public API only. Internally we
> will define both svn_txdelta() and svn_txdelta2() as one-line wrappers
> around some third function which does take an 'svn_boolean_t
> calculate_checksum' parameter.
>
That would only be o.k., if that internal function was internal
to libsvn_delta. But we would need to call it from other libs
which I see as a strong indicator that others might want this
functionality, too.
-- Stefan^2.
--
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*
Re: API simplification opportunity Re: svn commit: r1348666 - in
/subversion/trunk/subversion: include/ libsvn_delta/ libsvn_fs_fs/
libsvn_ra/ tests/libsvn_delta/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Thu, Oct 18, 2012 at 02:56:15 +0200:
> On Tue, Oct 16, 2012 at 12:55 AM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
>
> > stefan2@apache.org wrote on Sun, Jun 10, 2012 at 21:34:22 -0000:
> > > Author: stefan2
> > > Date: Sun Jun 10 21:34:21 2012
> > > New Revision: 1348666
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1348666&view=rev
> > > Log:
> > > When handing out node contents, the delta streams don't need
> > > to calculate MD5 checksums as the result will not be used and the
> > > check would be redundant even if it were made.
> > >
> > > Thus, rev the svn_txdelta API to calculate the checksum only
> > > upon specific request and update all callers to use the new API.
> > >
> > ...
> > > Modified: subversion/trunk/subversion/include/svn_delta.h
> > > URL:
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_delta.h?rev=1348666&r1=1348665&r2=1348666&view=diff
> > >
> > ==============================================================================
> > > --- subversion/trunk/subversion/include/svn_delta.h (original)
> > > +++ subversion/trunk/subversion/include/svn_delta.h Sun Jun 10 21:34:21
> > 2012
> > > @@ -367,10 +367,26 @@ svn_txdelta_md5_digest(svn_txdelta_strea
> > > + * If @a calculate_checksum
> > > + * is set, you may call @ref svn_txdelta_md5_digest to get an MD5
> > checksum
> > > + * for @a target.
> > > *
> > > * Do any necessary allocation in a sub-pool of @a pool.
> > > + *
> > > + * @since New in 1.8.
> > > + */
> > > +void
> > > +svn_txdelta2(svn_txdelta_stream_t **stream,
> > > + svn_stream_t *source,
> > > + svn_stream_t *target,
> > > + svn_boolean_t calculate_checksum,
> >
> > As of today, only svn_txdelta() and window-test.c pass TRUE for @a
> > calculate_checksum. Do we anticipate needing to pass TRUE For it in any
> > new code?
> >
>
> No, I don't anticipate extra need for that parameter
> neither do I feel that it is useless. How would you
> implement svn_txdelta() without that parameter?
>
I would remove the parameter from the public API only. Internally we
will define both svn_txdelta() and svn_txdelta2() as one-line wrappers
around some third function which does take an 'svn_boolean_t
calculate_checksum' parameter.
Re: API simplification opportunity Re: svn commit: r1348666 - in
/subversion/trunk/subversion: include/ libsvn_delta/ libsvn_fs_fs/ libsvn_ra/ tests/libsvn_delta/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Oct 16, 2012 at 12:55 AM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> stefan2@apache.org wrote on Sun, Jun 10, 2012 at 21:34:22 -0000:
> > Author: stefan2
> > Date: Sun Jun 10 21:34:21 2012
> > New Revision: 1348666
> >
> > URL: http://svn.apache.org/viewvc?rev=1348666&view=rev
> > Log:
> > When handing out node contents, the delta streams don't need
> > to calculate MD5 checksums as the result will not be used and the
> > check would be redundant even if it were made.
> >
> > Thus, rev the svn_txdelta API to calculate the checksum only
> > upon specific request and update all callers to use the new API.
> >
> ...
> > Modified: subversion/trunk/subversion/include/svn_delta.h
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_delta.h?rev=1348666&r1=1348665&r2=1348666&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/include/svn_delta.h (original)
> > +++ subversion/trunk/subversion/include/svn_delta.h Sun Jun 10 21:34:21
> 2012
> > @@ -367,10 +367,26 @@ svn_txdelta_md5_digest(svn_txdelta_strea
> > + * If @a calculate_checksum
> > + * is set, you may call @ref svn_txdelta_md5_digest to get an MD5
> checksum
> > + * for @a target.
> > *
> > * Do any necessary allocation in a sub-pool of @a pool.
> > + *
> > + * @since New in 1.8.
> > + */
> > +void
> > +svn_txdelta2(svn_txdelta_stream_t **stream,
> > + svn_stream_t *source,
> > + svn_stream_t *target,
> > + svn_boolean_t calculate_checksum,
>
> As of today, only svn_txdelta() and window-test.c pass TRUE for @a
> calculate_checksum. Do we anticipate needing to pass TRUE For it in any
> new code?
>
No, I don't anticipate extra need for that parameter
neither do I feel that it is useless. How would you
implement svn_txdelta() without that parameter?
> If not, we could take this opportunity to remove this parameter from the
> public API (it's a trivial patch).
>
If we can do without, I'd say let's lose the parameter
and make old and new API only differ in behavior.
> > + apr_pool_t *pool);
> > +
> > +/** Similar to svn_txdelta2 but always calculating the target checksum.
> > + *
> > + * @deprecated Provided for backward compatibility with the 1.7 API.
> > */
> > +SVN_DEPRECATED
> > void
> > svn_txdelta(svn_txdelta_stream_t **stream,
> > svn_stream_t *source,
> >
>
-- Stefan^2.
--
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*
API simplification opportunity Re: svn commit: r1348666 - in
/subversion/trunk/subversion: include/ libsvn_delta/ libsvn_fs_fs/
libsvn_ra/ tests/libsvn_delta/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stefan2@apache.org wrote on Sun, Jun 10, 2012 at 21:34:22 -0000:
> Author: stefan2
> Date: Sun Jun 10 21:34:21 2012
> New Revision: 1348666
>
> URL: http://svn.apache.org/viewvc?rev=1348666&view=rev
> Log:
> When handing out node contents, the delta streams don't need
> to calculate MD5 checksums as the result will not be used and the
> check would be redundant even if it were made.
>
> Thus, rev the svn_txdelta API to calculate the checksum only
> upon specific request and update all callers to use the new API.
>
...
> Modified: subversion/trunk/subversion/include/svn_delta.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_delta.h?rev=1348666&r1=1348665&r2=1348666&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_delta.h (original)
> +++ subversion/trunk/subversion/include/svn_delta.h Sun Jun 10 21:34:21 2012
> @@ -367,10 +367,26 @@ svn_txdelta_md5_digest(svn_txdelta_strea
> + * If @a calculate_checksum
> + * is set, you may call @ref svn_txdelta_md5_digest to get an MD5 checksum
> + * for @a target.
> *
> * Do any necessary allocation in a sub-pool of @a pool.
> + *
> + * @since New in 1.8.
> + */
> +void
> +svn_txdelta2(svn_txdelta_stream_t **stream,
> + svn_stream_t *source,
> + svn_stream_t *target,
> + svn_boolean_t calculate_checksum,
As of today, only svn_txdelta() and window-test.c pass TRUE for @a
calculate_checksum. Do we anticipate needing to pass TRUE For it in any
new code?
If not, we could take this opportunity to remove this parameter from the
public API (it's a trivial patch).
> + apr_pool_t *pool);
> +
> +/** Similar to svn_txdelta2 but always calculating the target checksum.
> + *
> + * @deprecated Provided for backward compatibility with the 1.7 API.
> */
> +SVN_DEPRECATED
> void
> svn_txdelta(svn_txdelta_stream_t **stream,
> svn_stream_t *source,
>