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

Re: svn commit: rev 1851 - trunk/subversion/include trunk/subversion/libsvn_fs trunk/subversion/tests/libsvn_fs

From: Greg Stein <gstein_at_lyra.org>
Date: 2002-05-14 04:24:46 CEST

On Thu, May 02, 2002 at 09:49:06AM -0500, cmpilato@tigris.org wrote:
>...
> Log:
> This affects issue #656.
>
> Begin the work of separating the internal filesystem structures from
> the list/string skeleton library routines. This is a requirement for
> pluggable database backends, and will (I think) assist in the
> conversion to the new NodeId interface.
>...

One overall comment is that I saw a number of uses of apr_pstrndup() when
using apr_pstrmemdup() will be faster (better).

I also really liked the clarification of FOO_key, so it was easier to
understand that only the key was being passed, not the value.

>...
> +++ trunk/subversion/libsvn_fs/reps-table.c Thu May 2 09:48:48 2002
>...
> @@ -99,11 +103,16 @@
> svn_error_t *
> svn_fs__write_rep (svn_fs_t *fs,
> const char *key,
> - skel_t *skel,
> + svn_fs__representation_t *rep,

Should make this 'const'

>...
> @@ -117,7 +126,7 @@
> svn_error_t *
> svn_fs__write_new_rep (const char **key,
> svn_fs_t *fs,
> - skel_t *skel,
> + svn_fs__representation_t *rep,

'const' here, too.

(and propagate these into reps-table.h)

>...
> +++ trunk/subversion/libsvn_fs/dag.c Thu May 2 09:48:48 2002
>...
> @@ -330,14 +331,15 @@
> "revisions" : 0 -> "(revision 3 0.0 ())" */
> {
> /* ### this should be const. we should make parse_skel() take a const */
> - static char rev_skel[] = "(revision 3 0.0 ())";
> + static char rev_skel_str[] = "(revision 3 0.0 ())";
> + svn_fs__revision_t *revision;
> + skel_t *rev_skel;
> svn_revnum_t rev = 0;
> - SVN_ERR (svn_fs__put_rev (&rev, fs,
> - svn_fs__parse_skel (rev_skel,
> - sizeof (rev_skel) - 1,
> - trail->pool),
> - trail));
> -
> +
> + rev_skel = svn_fs__parse_skel (rev_skel_str, strlen (rev_skel_str),
> + trail->pool);
> + SVN_ERR (svn_fs__parse_revision_skel (&revision, rev_skel, trail->pool));
> + SVN_ERR (svn_fs__put_rev (&rev, fs, revision, trail));

There is no reason to construct a skel from a constant string, then parse
that into a revision structure, just to put the structre. Instead, just
manually build the structure and put it.

>...
> +++ trunk/subversion/libsvn_fs/txn-table.c Thu May 2 09:48:48 2002
>...
> static svn_error_t *
> put_txn (svn_fs_t *fs,
> - const char *svn_txn,
> - const svn_fs_id_t *root_id,
> - const svn_fs_id_t *base_root_id,
> - skel_t *proplist,
> + svn_fs__transaction_t *txn,

Should be 'const'

>...
> +svn_fs__create_txn (char **txn_name_p,
> svn_fs_t *fs,
> const svn_fs_id_t *root_id,
> trail_t *trail)
> {
> - char *svn_txn;
> + char *txn_name;
> + svn_fs__transaction_t txn;
> + svn_fs_id_t *id_copy = svn_fs__id_copy (root_id, trail->pool);

It wasn't immediately obvious why the ID was copied. I just realized it is
to get the ID from whereever the caller had it into the trail pool. A
comment might be nice.

(the main point is: it is being copied for pool mgmt purposes, not that it
 will be modified)

Further, a reader might also think, "hey. the txn this goes into doesn't
survive the function, so I don't even need a local copy anyways." But
through the trail does, *does* it survive the function? I'm not entirely
clear. In any case, if it does *NOT* survive, then you definitely don't need
to copy the thing.

>...
> + /* Unparse TRANSACTION skel */
> + skel = svn_fs__parse_skel (value.data, value.size, trail->pool);

Um. You're *parsing* the skel, not "Unparse"

>...
> svn_fs__set_txn_base (svn_fs_t *fs,
>...
> + if (! svn_fs__id_eq (txn->base_root_id, new_id))
> + {
> + txn->base_root_id = svn_fs__id_copy (new_id, trail->pool);

Same thing about lifetimes, function survival, adding a comment, etc.

>...
> +++ trunk/subversion/libsvn_fs/reps-strings.c Thu May 2 09:48:49 2002
>...
> +/* Return a `fulltext' representation which references the string
> + STR_KEY, performing allocations in POOL. If MUTABLE is non-zero,
> + make the representation mutable. If non-NULL, STR_KEY will be
> + copied into an allocation of POOL. */
> +static svn_fs__representation_t *
> +make_fulltext_rep (const char *str_key,
>...
> + svn_fs__representation_t *rep = apr_palloc (pool, sizeof (*rep));
> + rep->is_mutable = 1;
> + rep->kind = svn_fs__rep_kind_fulltext;
> + rep->contents.fulltext.string_key = apr_pstrdup (pool, str_key);

The documentation says that STR_KEY might be NULL. Thus, this strdup is
going to fail in that case. Change the doc or the code...

>...
> +delta_string_keys (apr_array_header_t **keys,
> + svn_fs__representation_t *rep,

Should be 'const'

>...
> - if (! rep_is_delta (rep))
> + if (! (rep->kind == svn_fs__rep_kind_delta))

Heh. A lot easier to write that as:

     if (rep->kind != svn_fs__rep_kind_delta)

:-)

>...
> + /* Set up a convenience variable. */
> + chunks = rep->contents.delta.chunks;
> + if (! chunks->nelts)
> {
> - num_windows++;
> - window = window->next;
> + *keys = NULL;
> + return SVN_NO_ERROR;

Woah. This changes the semantics of the function. Further, this return of
NULL is not documented in the function's doc comment.

>...
> svn_fs__rep_contents_clear (svn_fs_t *fs,
> - const char *rep,
> + const char *rep_key,
> trail_t *trail)
> {
>...
> - /* Else, clear it. */
> + /* Else, clear it the string the rep has. */

That comment doesn't make any sense :-)

>...
> + /* Allocate a chunk and its window */
> + chunk = apr_palloc (pool, sizeof (*chunk));
> + chunk->offset = ww->text_off;
> + chunk->window = apr_palloc (pool, sizeof (*chunk->window));
> +
> + /* Populate the window */
> + chunk->window->string_key = ww->key;
> + chunk->window->size = ww->text_len;
> + memcpy (&(chunk->window->checksum), digest,
> + (MD5_DIGESTSIZE / sizeof (*digest)));

That division isn't needed. Simply using MD5_DIGESTSIZE should be fine.

>...
> +++ trunk/subversion/libsvn_fs/fs_skels.c Thu May 2 09:48:49 2002
>...
> +svn_error_t *
> +svn_fs__unparse_revision_skel (skel_t **skel_p,
> + svn_fs__revision_t *revision,

Should be 'const'

>...
> +svn_error_t *
> +svn_fs__unparse_transaction_skel (skel_t **skel_p,
> + svn_fs__transaction_t *transaction,

Should be 'const'

>...
> +svn_error_t *
> +svn_fs__unparse_representation_skel (skel_t **skel_p,
> + svn_fs__representation_t *rep,

Should be 'const'

(and the corresponding changes in fs_skels.h for these)

>...
> +++ trunk/subversion/libsvn_fs/fs.h Thu May 2 09:48:55 2002
>...
> +/*** Filesystem Transaction ***/
> +typedef struct
> +{
> + /* id of the root node */
> + svn_fs_id_t *root_id;
> +
> + /* id of the revision root node upon which this txn is base */
> + svn_fs_id_t *base_root_id;
> +
> + /* property list (const char * name, svn_string_t * value) */
> + apr_hash_t *proplist;

svn_fs__create_txn() might set 'proplist' to NULL. This should be doc'd.

>...
> +++ trunk/subversion/libsvn_fs/rev-table.c Thu May 2 09:49:00 2002
>...
> @@ -98,31 +78,50 @@
> /* Handle any other error conditions. */
> SVN_ERR (DB_WRAP (fs, "reading filesystem revision", db_err));
>
> - /* Parse and check the REVISION skel. */
> + /* Unparse REVISION skel. */
> skel = svn_fs__parse_skel (value.data, value.size, trail->pool);

You're parsing, not "Unparse"

>...
> {
> struct revision_prop_args *args = baton;
> -
> - skel_t *skel;
> + svn_fs__revision_t *revision;
> skel_t *proplist;
>
> - SVN_ERR (svn_fs__get_rev (&skel, args->fs, args->rev, trail));
> -
> - /* PROPLIST is the third element of revision skel. */
> - proplist = skel->children->next->next;
> + SVN_ERR (svn_fs__get_rev (&revision, args->fs, args->rev, trail));
> + SVN_ERR (svn_fs__unparse_proplist_skel (&proplist,
> + revision->proplist,
> + trail->pool));
>
> /* Return the results of the generic property getting function. */
> return svn_fs__get_prop (args->value_p,

Ah. I see this was simplified in a later commit. Goodness(tm).

>...
> +++ trunk/subversion/libsvn_fs/rev-table.h Thu May 2 09:49:00 2002
>...
> svn_error_t *svn_fs__put_rev (svn_revnum_t *rev,
> svn_fs_t *fs,
> - skel_t *skel,
> + svn_fs__revision_t *revision,

Should be 'const'

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 14 04:23:14 2002

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