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

Re: [PATCH 06/13] Dump the revprops at the start of every revision

From: Jonathan Nieder <jrnieder_at_gmail.com>
Date: Wed, 7 Jul 2010 14:04:34 -0500

Ramkumar Ramachandra wrote:

> Fill in replay_revstart to dump the revprops at the start of every
> revision. Add an additional write_hash_to_stringbuf helper function.

        A write_hash_to_stringbuf helper does the work of
        converting the property hashtable to dumpfile format.

> +++ b/dumpr_util.c
[...]
> +void write_hash_to_stringbuf(apr_hash_t *properties,
> + svn_boolean_t deleted,
> + svn_stringbuf_t **strbuf,
> + apr_pool_t *pool)

This function looks like:

        void write_hash_to_stringbuf(...
        {
                if (!deleted) {
                        for (each prop) {
                                append the new value of the prop;
                        }
                } else {
                        for (each prop) {
                                mention that it has been deleted;
                        }
                }
        }

It would be simpler to put the body of the loop in its own function,
like this:

        static void write_prop_to_stringbuf(...
        {
                if (deleted) {
                        append deletion notice;
                        return;
                }
                append new value of prop;
        }

        void write_hash_to_stringbuf(...
        {
                for (each prop)
                        write_prop_to_stringbuf(...
        }

Or even:

        static void write_prop(...
        static void write_deleted_prop(...

        void write_prop_data_to_stringbuf(...
        {
                for (each prop)
                        write_prop(...
        }
        void write_deleted_prop_data_to_stringbuf(...
        {
                for (each prop)
                        write_deleted_prop(...
        }

which would make the arguments from the caller less opaque.

I did not check whether the "return early in the simpler case" is
idiomatic for svn code. Of course you should respect whatever
convention is prevalent.

> +{
> + apr_hash_index_t *this;
> + const void *key;
> + void *val;
> + apr_ssize_t keylen;
> + svn_string_t *value;
> +
> + if (!deleted) {
> + for (this = apr_hash_first(pool, properties); this;
> + this = apr_hash_next(this)) {
> + /* Get this key and val. */
> + apr_hash_this(this, &key, &keylen, &val);
> + value = val;
> +
> + /* Output name length, then name. */
> + svn_stringbuf_appendcstr(*strbuf,
> + apr_psprintf(pool, "K %" APR_SSIZE_T_FMT "\n",
> + keylen));
> +
> + svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);

Is the cast needed? (The answer might be "yes" if this code is meant
to be usable with C++ compilers.)

> +++ b/svndumpr.c
> @@ -23,6 +23,37 @@ static svn_error_t *replay_revstart(svn_revnum_t revision,
> apr_hash_t *rev_props,
> apr_pool_t *pool)
> {
> + /* Editing this revision has just started; dump the revprops
> + before invoking the editor callbacks */
> + svn_stringbuf_t *propstring = svn_stringbuf_create("", pool);
> + svn_stream_t *stdout_stream;

Style: better to say in comments what we are trying to do than what we
actually do. So:

        /* First, dump revision properties. */

Maybe dumping revision properties should be its own function to make
that comment unnecessary (and make replay_revstart() less daunting as
it grows).

> +
> + /* Create an stdout stream */
> + svn_stream_for_stdout(&stdout_stream, pool);

Useless comment.

> +
> + /* Print revision number and prepare the propstring */
> + SVN_ERR(svn_stream_printf(stdout_stream, pool,
> + SVN_REPOS_DUMPFILE_REVISION_NUMBER
> + ": %ld\n", revision));
> + write_hash_to_stringbuf(rev_props, FALSE, &propstring, pool);
> + svn_stringbuf_appendbytes(propstring, "PROPS-END\n", 10);

Unhelpful comment. Maybe:

        /* Revision-number: 19 */
        SVN_ERR(svn_stream_printf(stdout_stream, pool,
                                  SVN_REPOS_DUMPFILE_REVISION_NUMBER
                                  ": %ld\n", revision));

        write_hash_to_stringbuf(rev_props, FALSE, &propstring, pool);
        svn_stringbuf_appendbytes(propstring, "PROPS-END\n", 10);

        /* Prop-content-length: 13 */
        SVN_ERR(svn_stream_printf(stdout_stream, pool,
                                  SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH
                                  ": %" APR_SIZE_T_FMT "\n", propstring->len));
        ...

This would make it particularly easy to grep for a particular header
(even if grepping for SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH is not
that hard).

[...]
> + /* Print the revprops now */
> + SVN_ERR(svn_stream_write(stdout_stream, propstring->data,
> + &(propstring->len)));

Maybe:

        /* Property data. */
        SVN_ERR(svn_stream_write(stdout_stream, propstring->data,
                                 &(propstring->len)));

The whole function so far has been about printing revprops.

> +
> + svn_stream_close(stdout_stream);

This does not actually fclose(stdout), does it?

> @@ -39,6 +70,9 @@ static svn_error_t *replay_revend(svn_revnum_t revision,
> apr_hash_t *rev_props,
> apr_pool_t *pool)
> {
> + /* Editor has finished for this revision and close_edit has
> + been called; do nothing: just continue to the next
> + revision */

I’d leave out the comment, or:

        /* No resources to free. */

HTH,
Jonathan
Received on 2010-07-07 21:05:58 CEST

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