You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2014/03/18 00:54:57 UTC
svn commit: r1578670 -
/subversion/trunk/subversion/svndumpfilter/svndumpfilter.c
Author: philip
Date: Mon Mar 17 23:54:57 2014
New Revision: 1578670
URL: http://svn.apache.org/r1578670
Log:
Fix the order of node record headers written by svndumpfilter.
Reported by: "Brown, Jonathan W" <JonathanW.Brown{_AT_}nuance.com>
* subversion/svndumpfilter/svndumpfilter.c
(new_node_record): Output 'Node-path' first.
Modified:
subversion/trunk/subversion/svndumpfilter/svndumpfilter.c
Modified: subversion/trunk/subversion/svndumpfilter/svndumpfilter.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svndumpfilter/svndumpfilter.c?rev=1578670&r1=1578669&r2=1578670&view=diff
==============================================================================
--- subversion/trunk/subversion/svndumpfilter/svndumpfilter.c (original)
+++ subversion/trunk/subversion/svndumpfilter/svndumpfilter.c Mon Mar 17 23:54:57 2014
@@ -623,6 +623,11 @@ new_node_record(void **node_baton,
if (! nb->rb->writing_begun)
SVN_ERR(output_revision(nb->rb));
+ /* A node record is required to begin with 'Node-path'. */
+ SVN_ERR(svn_stream_printf(nb->rb->pb->out_stream,
+ pool, "%s: %s\n",
+ SVN_REPOS_DUMPFILE_NODE_PATH, node_path));
+
for (hi = apr_hash_first(pool, headers); hi; hi = apr_hash_next(hi))
{
const char *key = svn__apr_hash_index_key(hi);
@@ -638,7 +643,8 @@ new_node_record(void **node_baton,
if ((!strcmp(key, SVN_REPOS_DUMPFILE_CONTENT_LENGTH))
|| (!strcmp(key, SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH))
- || (!strcmp(key, SVN_REPOS_DUMPFILE_TEXT_CONTENT_LENGTH)))
+ || (!strcmp(key, SVN_REPOS_DUMPFILE_TEXT_CONTENT_LENGTH))
+ || (!strcmp(key, SVN_REPOS_DUMPFILE_NODE_PATH)))
continue;
/* Rewrite Node-Copyfrom-Rev if we are renumbering revisions.
Re: r1578670 - Fix the order of node record headers written by svndumpfilter
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Philip Martin writes:
>> Julian Foad writes:
>>> This commit changes the output format of paths from no leading slash
>>> ('relpath' style) to having a leading slash ('fspath' style). This
>>> seems to be against the desired path format for a dump file, although
>>> the documentation in <notes/dump-load-format.txt> is not explicit.
>>>
>>> I noticed this because it crashes 'svnrdump load':
>>>
>>> $ svnrdump load file://$PWD/rr < the-dump-filtered
>>> lt-svnrdump: subversion/libsvn_subr/dirent_uri.c:1256: svn_relpath_dirname: Assertion `relpath_is_canonical(relpath)' failed.
>>> Aborted (core dumped)
>>>
>>> That's probably a bug in itself; 'svnadmin load' accepts them.
I raised issue #4492 "svnrdump load assertion failure if Node-path starts with a slash".
[...]
>> The code at the top of new_node_record explicitly adds a leading '/' if
>> none is present so "node_path + 1" should be a safe way to print the
>> path without a leading '/'.
Philip committed a fix in r1587511.
> dump-load-format.txt says that Node-path is "relative to the repository
> root", and the examples don't have leading slashes, however nothing
> explicitly states that leading slashes are prohibited and a path with a
> leading slash can be interpreted as relative to the root.
I mentioned the need to clarify this within issue #4492.
- Julian
Re: r1578670 - Fix the order of node record headers written by svndumpfilter
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@wandisco.com> writes:
> Julian Foad <ju...@btopenworld.com> writes:
>
>> This commit changes the output format of paths from no leading slash
>> ('relpath' style) to having a leading slash ('fspath' style). This
>> seems to be against the desired path format for a dump file, although
>> the documentation in <notes/dump-load-format.txt> is not explicit.
>>
>> I noticed this because it crashes 'svnrdump load':
>>
>> $ svnrdump load file://$PWD/rr < the-dump-filtered
>> lt-svnrdump: subversion/libsvn_subr/dirent_uri.c:1256: svn_relpath_dirname: Assertion `relpath_is_canonical(relpath)' failed.
>> Aborted (core dumped)
>>
>> That's probably a bug in itself; 'svnadmin load' accepts them.
>
> I can't reproduce the SEGV with 1.6, 1.7, 1.8 or trunk.
Ah, missed it was svnrdump.
> The code at the top of new_node_record explicitly adds a leading '/' if
> none is present so "node_path + 1" should be a safe way to print the
> path without a leading '/'.
dump-load-format.txt says that Node-path is "relative to the repository
root", and the examples don't have leading slashes, however nothing
explicitly states that leading slashes are prohibited and a path with a
leading slash can be interpreted as relative to the root.
--
Philip
Re: r1578670 - Fix the order of node record headers written by svndumpfilter
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
> This commit changes the output format of paths from no leading slash
> ('relpath' style) to having a leading slash ('fspath' style). This
> seems to be against the desired path format for a dump file, although
> the documentation in <notes/dump-load-format.txt> is not explicit.
>
> I noticed this because it crashes 'svnrdump load':
>
> $ svnrdump load file://$PWD/rr < the-dump-filtered
> lt-svnrdump: subversion/libsvn_subr/dirent_uri.c:1256: svn_relpath_dirname: Assertion `relpath_is_canonical(relpath)' failed.
> Aborted (core dumped)
>
> That's probably a bug in itself; 'svnadmin load' accepts them.
I can't reproduce the SEGV with 1.6, 1.7, 1.8 or trunk.
The code at the top of new_node_record explicitly adds a leading '/' if
none is present so "node_path + 1" should be a safe way to print the
path without a leading '/'.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: r1578670 - Fix the order of node record headers written by svndumpfilter
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Philip,
This commit changes the output format of paths from no leading slash ('relpath' style) to having a leading slash ('fspath' style). This seems to be against the desired path format for a dump file, although the documentation in <notes/dump-load-format.txt> is not explicit.
I noticed this because it crashes 'svnrdump load':
$ svnrdump load file://$PWD/rr < the-dump-filtered
lt-svnrdump: subversion/libsvn_subr/dirent_uri.c:1256: svn_relpath_dirname: Assertion `relpath_is_canonical(relpath)' failed.
Aborted (core dumped)
That's probably a bug in itself; 'svnadmin load' accepts them.
The code should probably use the path as it was read from the input stream, so as not to make unnecessary changes to the data. (However, preserving the exact input for this particular header is a drop in the ocean, as svndumpfilter also makes many other other unnecessary changes such as reordering headers and adding redundant headers, and I think it would be better if it did not, but that's another story.)
- Julian
> Author: philip
> URL: http://svn.apache.org/r1578670
> Log:
> Fix the order of node record headers written by svndumpfilter.
>
> * subversion/svndumpfilter/svndumpfilter.c
> (new_node_record): Output 'Node-path' first.
>
> Modified: subversion/trunk/subversion/svndumpfilter/svndumpfilter.c
> ==============================================================================
> + /* A node record is required to begin with 'Node-path'. */
> + SVN_ERR(svn_stream_printf(nb->rb->pb->out_stream,
> + pool, "%s: %s\n",
> + SVN_REPOS_DUMPFILE_NODE_PATH, node_path));
> +
> for (hi = apr_hash_first(pool, headers); hi; hi = apr_hash_next(hi))
> {
> const char *key = svn__apr_hash_index_key(hi);
> @@ -638,7 +643,8 @@ new_node_record(void **node_baton,
>
> if ((!strcmp(key, SVN_REPOS_DUMPFILE_CONTENT_LENGTH))
> || (!strcmp(key, SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH))
> - || (!strcmp(key, SVN_REPOS_DUMPFILE_TEXT_CONTENT_LENGTH)))
> + || (!strcmp(key, SVN_REPOS_DUMPFILE_TEXT_CONTENT_LENGTH))
> + || (!strcmp(key, SVN_REPOS_DUMPFILE_NODE_PATH)))
> continue;