You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gabriela Gibson <ga...@gmail.com> on 2012/12/18 02:40:21 UTC
[PATCH] Fix for "\r" in "svnrdump load" (issue 4263)
Hi all,
[[
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.
In the case of "\r\n" character sequences, the "\r" is removed.
In the case of "\r" characters which are not followed by "\n", the "\r"
is replaced by a "\n" character.
* subversion/svnrdump/svnrdump.h:
(svn_rdump__normalize_prop): New function declaration.
* subversion/svnrdump/util.c:
(svn_rdump__normalize_prop): New function to allow "svnrdump load" to
use existing logic
(svn_rdump__normalize_props): Refactored to move logic into
(svn_rdump__normalize_prop)
* subversion/svnrdump/load_editor.c:
(set_revision_property): Added call to (svn_rdump__normalize_prop)
(set_node_property): Added call to (svn_rdump__normalize_prop)
* subversion/tests/cmdline/svnrdump_tests.py:
(copy_bad_line_endings_load): Removed "XFail" decorator
]]]
regards,
Gabriela
Re: [PATCH] Fix for "\r" in "svnrdump load" (issue 4263)
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Tue, Dec 18, 2012 at 17:17:08 +0000:
> I'll be taking a little break for a few days
Enjoy!
> and will be doing some more research into the suggested projects
> before I ask on the list for advice as to which project to choose
> next.
Feel free to catch us on IRC, too, if you have any questions before you
post the thread. (#svn-dev on freenode)
Re: [PATCH] Fix for "\r" in "svnrdump load" (issue 4263)
Posted by Gabriela Gibson <ga...@gmail.com>.
On 18/12/12 02:09, Ben Reser wrote:
> On Mon, Dec 17, 2012 at 5:40 PM, Gabriela Gibson
> <ga...@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.
Thank you very much for checking this over, the fixed submission is in
the next post to this thread.
I'll be taking a little break for a few days and will be doing some more
research into the suggested projects before I ask on the list for advice
as to which project to choose next.
regards,
Gabriela
Re: [PATCH] Fix for "\r" in "svnrdump load" (issue 4263)
Posted by Ben Reser <be...@reser.org>.
On Mon, Dec 17, 2012 at 5:40 PM, Gabriela Gibson
<ga...@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).
Re: [PATCH] Fix for "\r" in "svnrdump load" (issue 4263) (reworked)
Posted by Ben Reser <be...@reser.org>.
On Tue, Dec 18, 2012 at 9:17 AM, Gabriela Gibson
<ga...@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.
>
> In the case of "\r\n" character sequences, the "\r" is removed.
>
> In the case of "\r" characters which are not followed by "\n", the "\r"
> is replaced by a "\n" character.
>
> * subversion/svnrdump/svnrdump.h:
> (svn_rdump__normalize_prop): New function declaration.
>
> * subversion/svnrdump/util.c:
> (svn_rdump__normalize_prop): New function to allow "svnrdump load" to
> use existing logic
> (svn_rdump__normalize_props): Refactored to move logic into
> svn_rdump__normalize_prop()
>
> * subversion/svnrdump/load_editor.c:
> (set_revision_property): Added call to svn_rdump__normalize_prop()
> (set_node_property): Added call to svn_rdump__normalize_prop()
>
> * subversion/tests/cmdline/svnrdump_tests.py:
> (copy_bad_line_endings_load): Removed "XFail" decorator
>
> ]]]
Thanks, committed in r1423585, only thing I added was the Patch by:
line to the log to credit you.
Re: [PATCH] Fix for "\r" in "svnrdump load" (issue 4263) (reworked)
Posted by Gabriela Gibson <ga...@gmail.com>.
[[[
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.
In the case of "\r\n" character sequences, the "\r" is removed.
In the case of "\r" characters which are not followed by "\n", the "\r"
is replaced by a "\n" character.
* subversion/svnrdump/svnrdump.h:
(svn_rdump__normalize_prop): New function declaration.
* subversion/svnrdump/util.c:
(svn_rdump__normalize_prop): New function to allow "svnrdump load" to
use existing logic
(svn_rdump__normalize_props): Refactored to move logic into
svn_rdump__normalize_prop()
* subversion/svnrdump/load_editor.c:
(set_revision_property): Added call to svn_rdump__normalize_prop()
(set_node_property): Added call to svn_rdump__normalize_prop()
* subversion/tests/cmdline/svnrdump_tests.py:
(copy_bad_line_endings_load): Removed "XFail" decorator
]]]