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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jul 21, 2010, 18:55 UTC
Message-ID: <20100721185513.GB23839@kytes>
In-Reply-To: <20100707190434.GA2732@burratino>

Hi Jonathan,

I stashed this review away while working on some other important changes. I finally got around to responding to this review- sorry that it took so long.

Jonathan Nieder writes:

Show 12 quoted lines

> > 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)

[...]

Fixed, but not exactly in the way you've suggested.

Show 9 quoted lines

> > +			/* 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.)

These casts are all over in the source tree, so I'm guessing the answer is "yes".

> Style: better to say in comments what we are trying to do than what we
> actually do.  So:
> 
> 	/* First, dump revision properties. */

I've fixed all the comments in the entire source tree. Thanks :)

-- Ram