On 31.07.2018 13:04, Julian Foad wrote:
> https://issues.apache.org/jira/browse/SVN-4767
>
> Running tests with the '--dump-load-cross-check' option revealed a minor discrepancy between the dump files produced by 'svnadmin dump' and 'svnrdump dump' in some test cases.
>
> [...]
> K
> svn:date
> V
> - 2015-01-01T00:00:00.000000Z
> + 2015-01-01T00:00:00.0Z
> [...]
>
> "svnadmin dump" "canonicalizes" each svn:date revprop while dumping, in the function write_revision_record().
>
> This seems to have been done in r842390 in order to upgrade from pre-0.14 repository format to the new timestamp format introduced in 0.14 -- see issue SVN-614 "DAV:creationdate needs to be an ISO8601 date". svn_time_from_cstring() reads either new or old format, and then svn_time_to_cstring() writes the new format.
There's another possible interpretation here: that the millisecond field
should be 6 digits wide, but for some reason svn_time_to_cstring doesn't
format it that way when the value is zero. I don't have the actual
standard, so I can't check what it says ... and, of course, in the case
of 0 there is no semantic difference.
> However, this does not only convert old to new format, but could also make textual changes to the string if the revprop value is not already canonical. Dump should carefully output exactly what is in the repository and not gratuitously change it. In retrospect, such a transformation should have been done during "svnadmin load" instead of in "dump".
Agreed, this fits with what "svnadmin load" already does with the
--normalize-props option. But if the timestamp is *not* in the right
format, "load" should fix it. Maybe "except with
--bypass-prop-validation"? Another option that controls "load" behaviour
WRT dates is --ignore-dates, though I can't remember why we introduced that.
In any case I think the logic should be changed a bit: Only reformat the
svn:date property if it is not already in ISO8601 format; otherwise,
leave it alone.
> While "svnadmin dump" makes this transformation, "svnrdump dump" and "svndumpfilter" do not. This could lead to unintended differences in dump output depending on which tool is used.
"svnadmin dump" should always define the canonical behaviour. Both
"svnrdump" and "svndumpfilter" are newer; they should behave the way
"svnadmin dump" does, not the other way around. One _could_ argue that
this is an omission in svnrdump.
> Therefore I will remove this code path. It even has a comment saying "### Remove this when it is no longer needed for sure" which referred to being needed for pre-0.14 upgrades. We definitely no longer need that.
+0, and I'll explain why:
1. This is not a trivial change and should be documented in 1.11
release notes (hence, it stays on trunk until then). It's possible
that some 3rd-party tools rely on "svnadmin dump" producing
timestamps in the ISO format.
2. Our dumpfile format is documented. With your proposed change, there
is a slight chance that "svnadmin dump" could produce invalid dump
files. This is in effect a corollary of point 1., above. Note that
"svndrump dump" is already "infected" by this potential bug, whereas
"svndumpfilter" is not because it starts with a presumably valid
dumpfile.
-- Brane
Received on 2018-07-31 13:44:05 CEST