Good morning Evgeny,
kotkov_at_apache.org wrote on Thu, 27 Jul 2017 09:00 +0000:
> +++ subversion/trunk/subversion/include/svn_delta.h Thu Jul 27 09:00:43 2017
> @@ -678,6 +690,9 @@ svn_txdelta_skip_svndiff_window(apr_file
> * @{
> */
>
> +/* Forward declarations. */
> +typedef struct svn_delta_editor_t svn_delta_editor_t;
> +
> /** A structure full of callback functions the delta source will invoke
> * as it produces the delta.
> *
> @@ -859,7 +874,7 @@ svn_txdelta_skip_svndiff_window(apr_file
> * dead; the only further operation which may be called on the editor
> * is @c abort_edit.
> */
> -typedef struct svn_delta_editor_t
> +struct svn_delta_editor_t
> {
> /** Set the target revision for this edit to @a target_revision. This
> * call, if used, should precede all other editor calls.
> @@ -1131,9 +1146,38 @@ typedef struct svn_delta_editor_t
> svn_error_t *(*abort_edit)(void *edit_baton,
> apr_pool_t *scratch_pool);
>
> + /** Apply a text delta stream, yielding the new revision of a file.
> + *
> + * @a file_baton indicates the file we're creating or updating, and the
> + * ancestor file on which it is based; it is the baton set by some
> + * prior @c add_file or @c open_file callback.
> + *
> + * @a open_func is a function that opens a #svn_txdelta_stream_t object.
> + * @a open_baton is provided by the caller.
> + *
> + * @a base_checksum is the hex MD5 digest for the base text against
> + * which the delta is being applied; it is ignored if NULL, and may
> + * be ignored even if not NULL. If it is not ignored, it must match
What's the rationale for allowing drivees to ignore the checksum?
This leeway enables failure modes that wouldn't be possible without it.
(Drivers that are aware of this leeway will validate checksums even if the
drivee doesn't, leading to duplicate work; drivers that are unaware of this
requirement might not get checksum errors they should have.)
I get that you just copied this part of the docstring from apply_textdelta(),
but I'd like to understand what's the rationale here. (And to see if this
leeway should be deprecated)
> + * the checksum of the base text against which svndiff data is being
> + * applied; if it does not, @c apply_textdelta_stream call which detects
> + * the mismatch will return the error SVN_ERR_CHECKSUM_MISMATCH
> + * (if there is no base text, there may still be an error if
> + * @a base_checksum is neither NULL nor the hex MD5 checksum of the
> + * empty string).
To the best of my knowledge, we don't special case the empty string's checksum,
d41d8cd98f00b204e9800998ecf8427e, anywhere. We do special-case
00000000000000000000000000000000, though. I assume the parenthetical should be
fixed accordingly (both here and in apply_textdelta())?
> + *
> + * Any temporary allocations may be performed in @a scratch_pool.
Need to add an @since tag here.
> + */
> + svn_error_t *(*apply_textdelta_stream)(
Could you update the docstring of svn_delta_editor_t itself to mention this new
callback? All other callbacks are discussed there.
>
Is adding this function an ABI-compatible change? The docstring of
svn_delta_editor_t does say """
* @note Don't try to allocate one of these yourself. Instead, always
* use svn_delta_default_editor() or some other constructor, to ensure
* that unused slots are filled in with no-op functions.
""", but an API consumer might have interpreted this note as meaning "You may
use malloc(..., sizeof(svn_delta_editor_t)) if you take care to initialize all
struct members", in which case, his code will not be ABI compatible with 1.10.
> + const svn_delta_editor_t *editor,
This parameter is undocumented.
Cheers,
Daniel
> + void *file_baton,
> + const char *base_checksum,
> + svn_txdelta_stream_open_func_t open_func,
> + void *open_baton,
> + apr_pool_t *scratch_pool);
> +
> /* Be sure to update svn_delta_get_cancellation_editor() and
> * svn_delta_default_editor() if you add a new callback here. */
> -} svn_delta_editor_t;
> +};
>
>
> /** Return a default delta editor template, allocated in @a pool.
>
Received on 2017-08-30 07:07:26 CEST