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

[Jonathan Nieder <jrnieder@gmail.com>]

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