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
