Greg Stein wrote:
> On Mon, Aug 18, 2008 at 1:05 PM, <hwright_at_tigris.org> wrote:
>> ...
>> * If the filesystem does not have a prerecorded checksum for @a path,
>> - * do not calculate a checksum dynamically, just put all 0's into @a
>> - * digest. (By convention, the all-zero checksum is considered to
>> - * match any checksum.)
>> + * and @a force is not TRUE, do not calculate a checksum dynamically, just
>> + * put NULL into @a checksum. (By convention, the NULL checksum is
>> + * considered to match any checksum.)
>
> Nit: "does not have a prerecorded checksum [of the right kind]"
>
> The function will recompute a checksum if it is the wrong kind, and
> force is set.
I've clarified the doc string in r32532.
>> ...
>> +++ trunk/subversion/libsvn_fs/fs-loader.h Mon Aug 18 13:05:09 2008 (r32527)
>> @@ -273,9 +273,8 @@ typedef struct root_vtable_t
>> /* Files */
>> svn_error_t *(*file_length)(svn_filesize_t *length_p, svn_fs_root_t *root,
>> const char *path, apr_pool_t *pool);
>> - svn_error_t *(*file_md5_checksum)(unsigned char digest[],
>> - svn_fs_root_t *root,
>> - const char *path, apr_pool_t *pool);
>> + svn_error_t *(*file_checksum)(svn_checksum_t **checksum, svn_fs_root_t *root,
>> + const char *path, apr_pool_t *pool);
>
> Should there be a "kind" in this API?
>
> If not, then how is the API defined? "Whatever checksum is available,
> otherwise a SHA1 checksum." ?
I don't think there needs to be a kind in this API. The semantics of this API
are "give me the checksum you may have recorded." We worry about recalculating
it if forced in libsvn_fs. I don't see any vtable APIs we currently document,
but I suppose can do so if we need to.
>> ...
>> +++ trunk/subversion/libsvn_fs_base/dag.c Mon Aug 18 13:05:09 2008 (r32527)
>> ...
>> - if (checksum)
>> - {
>> - unsigned char digest[APR_MD5_DIGESTSIZE];
>> - const char *hex;
>> -
>> - SVN_ERR(svn_fs_base__rep_contents_checksum
>> - (digest, fs, noderev->edit_key, trail, pool));
>> -
>> - hex = svn_md5_digest_to_cstring_display(digest, pool);
>> - if (strcmp(checksum, hex) != 0)
>> - return svn_error_createf
>> - (SVN_ERR_CHECKSUM_MISMATCH,
>> - NULL,
>> - _("Checksum mismatch, rep '%s':\n"
>> - " expected: %s\n"
>> - " actual: %s\n"),
>> - noderev->edit_key, checksum, hex);
>> - }
>> + /* Get our representation's checksum. */
>> + SVN_ERR(svn_fs_base__rep_contents_checksum(&rep_checksum, fs,
>> + noderev->edit_key, trail, pool));
>
> Woah. This will always compute it now, which is quite different from
> before. I think you should retain the optional behavior of "if they
> provided a checksum, *then* compute it and compare it."
Per the doc string for svn_fs_base__rep_contents_checksum(), it doesn't
recalculate the checksum, just gives the checksum already stored. We can still
make that conditional, but this seems more straightforward.
>> ...
>> @@ -1416,6 +1412,18 @@ svn_fs_base__dag_deltify(dag_node_t *tar
>> }
>>
>>
>> +svn_error_t *
>> +svn_fs_base__dag_index_checksums(dag_node_t *node,
>> + trail_t *trail,
>> + apr_pool_t *pool)
>> +{
>> + node_revision_t *node_rev;
>> +
>> + SVN_ERR(svn_fs_bdb__get_node_revision(&node_rev, trail->fs, node->id,
>> + trail, pool));
>> + return SVN_NO_ERROR;
>> +}
>
>
>> ...
>> +++ trunk/subversion/libsvn_fs_base/dag.h Mon Aug 18 13:05:09 2008 (r32527)
>> ...
>> -/* Put the recorded MD5 checksum of FILE into DIGEST, as part of
>> - * TRAIL. DIGEST must point to APR_MD5_DIGESTSIZE bytes of storage.
>> +/* Put the recorded checksum of FILE into CHECKSUM, as part of
>> + * TRAIL.
>> *
>> * If no stored checksum is available, do not calculate the checksum,
>> * just put all 0's into DIGEST.
>
> Doesn't it set *checksum to NULL?
Correct. Fixed in r32532.
>> ...
>> +++ trunk/subversion/libsvn_fs_base/reps-strings.c Mon Aug 18 13:05:09 2008 (r32527)
>> ...
>> @@ -797,22 +799,20 @@ svn_fs_base__rep_contents(svn_string_t *
>> /* Just the standard paranoia. */
>> {
>> representation_t *rep;
>> - apr_md5_ctx_t md5_context;
>> - unsigned char checksum[APR_MD5_DIGESTSIZE];
>> + svn_checksum_t *checksum;
>>
>> - apr_md5_init(&md5_context);
>> - apr_md5_update(&md5_context, str->data, str->len);
>> - apr_md5_final(checksum, &md5_context);
>> + SVN_ERR(svn_checksum(&checksum, svn_checksum_md5, str->data, str->len,
>
> Leave a ### comment that this should switch to svn_checksum_sha1?
Possibly, but I think that it would be a little superfluous.
>> ...
>> +++ trunk/subversion/libsvn_fs_base/reps-strings.h Mon Aug 18 13:05:09 2008 (r32527)
>> @@ -76,12 +76,12 @@ svn_error_t *svn_fs_base__rep_contents_s
>> apr_pool_t *pool);
>>
>>
>> -/* Put into DIGEST the MD5 checksum for REP_KEY in FS, as part of TRAIL.
>> - This is the prerecorded checksum for the rep's contents' fulltext.
>> - If no checksum is available, do not calculate one dynamically, just
>> - put all 0's into DIGEST. (By convention, the all-zero checksum is
>> - considered to match any checksum.) */
>> -svn_error_t *svn_fs_base__rep_contents_checksum(unsigned char digest[],
>> +/* Put into CHECKSUM the checksum for REP_KEY in FS, as part of TRAIL. This
>> + is the prerecorded checksum for the rep's contents' fulltext. If no checksum
>> + of this type is available, do not calculate one dynamically, just put NULL
>
> "of this type" ?? There is no requested type. Should the kind be an
> argument? Or will this function always produce a checksum of kind X?
Updated in r32532.
>> ...
>> +++ trunk/subversion/libsvn_fs_base/tree.c Mon Aug 18 13:05:09 2008 (r32527)
>> ...
>> + {
>> + tb->base_checksum = svn_checksum_create(svn_checksum_md5, pool);
>> + SVN_ERR(svn_checksum_parse_hex(tb->base_checksum, base_checksum));
>> + }
>> else
>> tb->base_checksum = NULL;
>>
>> if (result_checksum)
>> - tb->result_checksum = apr_pstrdup(pool, result_checksum);
>> + {
>> + tb->result_checksum = svn_checksum_create(svn_checksum_md5, pool);
>> + SVN_ERR(svn_checksum_parse_hex(tb->result_checksum, result_checksum));
>
> Won't it always be a pair of calls like this? Shouldn't parse_hex()
> just be a constructor, rather than a modifier?
I've addressed this in r32534.
>> ...
>> +++ trunk/subversion/libsvn_fs_base/util/fs_skels.c Mon Aug 18 13:05:09 2008 (r32527)
>> ...
>> +++ trunk/subversion/libsvn_fs_fs/tree.c Mon Aug 18 13:05:09 2008 (r32527)
>> ...
>> +fs_file_checksum(svn_checksum_t **checksum,
>> + svn_fs_root_t *root,
>> + const char *path,
>> + apr_pool_t *pool)
>> {
>> dag_node_t *file;
>>
>> SVN_ERR(get_dag(&file, root, path, pool));
>> - return svn_fs_fs__dag_file_checksum(digest, file, pool);
>> +
>> + *checksum = svn_checksum_create(svn_checksum_md5, pool);
>
> Leave a ### marker to change this, or take the kind as an argument.
As above, I think the comment is a bit superfluous. I have a patch in progress
which updates libsvn_fs_fs to use svn_checksum_t.
Thanks for the review. Feel free to follow up here or in the repo.
-Hyrum
Received on 2008-08-19 08:52:04 CEST