[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1803143 - in /subversion/trunk/subversion: include/ libsvn_delta/ libsvn_ra_serf/ mod_dav_svn/

From: Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
Date: Thu, 31 Aug 2017 19:05:25 +0300

Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:

>> + *
>> + * 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.

[...]

>> + const svn_delta_editor_t *editor,
>
> This parameter is undocumented.

Thank you for the review. I agree with these three points, and will try
to make the suggested tweaks to the documentation.

>> + /** 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)

My interpretation of the documentation is that "the result checksum must
be validated by the implementation, whereas validating the checksum of the
base text may be omitted".

Perhaps, there are cases where the base checksum won't be validated by
our existing implementations (what about BDB, for instance?), but in the
meanwhile, I'm not too sure about the gain from _always_ requiring such
checks.

I think that, from the editor driver's point of view, the important thing
is the result checksum. If the implementation also validates the base
checksum, that may allow it to skip actually applying the delta in case
of the early mismatch, give away better error messages ("I have a base
checksum mismatch" instead of "I can't apply instruction 7 at offset
12345") and maybe even detect the coding mistakes which cause the
delta to be applied to an unexpected base, but still yielding the
expected resulting fulltext.

Having all these properties is nice, but probably not mandatory. And I
think that lifting the optionality of this check could shoot us in the foot
in the future, if we find ourselves writing an implementation where it is
particularly tricky to always implement the base text checks — for instance,
due to the used protocol or any other technical reasons.

Hope that I am not missing something subtle here :)

>> + * 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())?

I agree that this part of the docstring (same in svn_fs_apply_textdelta())
looks odd, and I don't think that the digest of an empty string is specially
handled somewhere in the implementations.

Perhaps, it would make sense to check on whether this has been true at
some point in the past and also tweak the docstring.

> 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.

I think that adding this callback does not affect the ABI compatibility.
The note says "Don't try to allocate one of these yourself", whereas the
malloc(..., sizeof(svn_delta_editor_t)) example does exactly the opposite.

Thanks,
Evgeny Kotkov
Received on 2017-08-31 18:06:02 CEST

This is an archived mail posted to the Subversion Dev mailing list.