You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2008/08/18 21:10:28 UTC
Re: svn commit: r32527 - in trunk: . subversion/include subversion/libsvn_fs subversion/libsvn_fs_base subversion/libsvn_fs_base/util subversion/libsvn_fs_fs subversion/libsvn_subr subversion/tests/libsvn_fs subversion/tests/libsvn_subr tools/server-
On Mon, Aug 18, 2008 at 1:05 PM, <hw...@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.
>...
> +++ 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." ?
>...
> +++ 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."
>...
> @@ -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?
>...
> +++ 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?
>...
> +++ 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?
>...
> +++ 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?
>...
> +++ 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.
>...
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r32527 - in trunk: . subversion/include subversion/libsvn_fs
subversion/libsvn_fs_base subversion/libsvn_fs_base/util subversion/libsvn_fs_fs
subversion/libsvn_subr subversion/tests/libsvn_fs subversion/tests/libsvn_subr
tools/server-
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Greg Stein wrote:
> On Mon, Aug 18, 2008 at 1:05 PM, <hw...@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