Re: [PATCH] Fix for "\r" in "svnrdump load" (issue 4263)

On Mon, Dec 17, 2012 at 5:40 PM, Gabriela Gibson
<gabriela.gibson_at_gmail.com> wrote:
> [[
> Fix issue #4263: svnrdump: E125005: Cannot accept non-LF line endings in
> 'svn:log' property
>
> Fix to ensure that no "\r" characters are present in revision or node
> props.

Looks pretty good to me with a few relatively minor comments.

I'd remove the test for svn_prop_needs_translation() in
svn_rdump__normalize_props since you're doing it in
svn_rdump__normalize_prop().

I'd include the argument names in your comment for the
svn_rdump__normalize_prop definition. I note that svnrdump.h has two
different styles for doing this. svn_rdump__get_dump_editor() and
svn_rdump__load_dumpstring() use Doxygen mark-up and
svn_rdump__normalize_props() is just using caps for the argument
names. I'd say the later format is the norm for internal APIs like
svn_rdump__normalize_prop().

You've got a couple spurious whitespace changes in your patch that I
would have probably removed.

You have tab indentation in your code, we only use spaces.

> (svn_rdump__normalize_props): Refactored to move logic into
> (svn_rdump__normalize_prop)

We usually only indent that second line with 2 spaces. (though I'm
noticing our community guide is only using 1 character, hmm)

Function references in the descriptive text is usually written as
svn_rdump__normalize_prop() (which unfortunately is not really
referenced in our community guide).
Received on 2012-12-18 03:10:02 CET