On Mon, Dec 17, 2012 at 5:40 PM, Gabriela Gibson <gabriela.gib...@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).
