Re: svn commit: r33306 - in branches/fs-rep-sharing/subversion/libsvn_fs_base: . bdb

Greg Stein wrote:
> On Thu, Sep 25, 2008 at 9:35 PM, <hwright_at_tigris.org> wrote:
>> ...
>> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/dag.c Thu Sep 25 21:35:28 2008 (r33306)
>> ...
>> @@ -1245,8 +1245,10 @@ svn_fs_base__dag_finalize_edits(dag_node
>> SVN_ERR(svn_fs_base__rep_contents_checksum(&rep_checksum, fs,
>> noderev->edit_key, trail, pool));
>>
>> - /* If our caller provided a checksum to compare, do so. */
>> - if (checksum && !svn_checksum_match(checksum, rep_checksum))
>> + /* If our caller provided a checksum of the right kind to compare, do so. */
>> + if (checksum
>> + && checksum->kind == rep_checksum->kind
>> + && !svn_checksum_match(checksum, rep_checksum))
>
> Why check the kind? Doesn't svn_checksum_match() do that? And if not,
> then it *should*.

If the kinds differ, svn_checksum_match() will return FALSE. In this case, we
don't want that, we don't even want to check different kinds of checksums. The
root of the problem here is that since we're storing sha1 checksums, but dump
files and ra frontends are giving us md5 checksums, we don't want to fail just
because a checksum mismatch.

>> ...
>> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/reps-strings.c Thu Sep 25 21:35:28 2008 (r33306)
>> ...
>> @@ -638,12 +638,19 @@ struct rep_read_baton
>> ...
>> + /* MD5 checksum context. Initialized when the baton is created, updated as
>> + we read data, and finalized when the stream is closed. */
>> + svn_checksum_ctx_t *sha1_checksum_ctx;
>
> Bad comment. That's the SHA1 checksum context.
>
> And do we really need *both* forms? Why?

See above. This is an attempt (albeit fruitless at this point), to compute
*both* checksums, so that we can compare the same type in situations such as the
above.

>> ...
>> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/tree.c Thu Sep 25 21:35:28 2008 (r33306)
>> @@ -3672,7 +3672,12 @@ txn_body_apply_textdelta(void *baton, tr
>> contents, in other words, the base text. */
>> SVN_ERR(svn_fs_base__dag_file_checksum(&checksum, tb->node,
>> trail, trail->pool));
>> - if (!svn_checksum_match(tb->base_checksum, checksum))
>> + /* TODO: This only compares checksums if they are the same kind, but
>> + we're calculating both SHA1 and MD5 checksums somewhere in
>> + reps-strings.c. Could we keep them both around somehow so this
>> + check could be more comprehensive? */
>> + if (tb->base_checksum->kind == checksum->kind
>> + && !svn_checksum_match(tb->base_checksum, checksum))
>> return svn_error_createf
>> (SVN_ERR_CHECKSUM_MISMATCH,
>
> Again, why check the kind? Should be able to just call svn_checksum_match().
>
> Note that you may want to consider something like:
>
> svn_checksum_match_either(checksum, md5_desired, sha1_desired)
>
> Then the checksum can be checked against the appropriate kind.

Hmmm....I'll take a look at this.

-Hyrum