Re: Issue SVN-4767: svnadmin dump shouldn't canonicalize svn:date

Branko Čibej wrote on 2018-07-31:
> 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.
> > - 2015-01-01T00:00:00.000000Z
> > + 2015-01-01T00:00:00.0Z

I wasn't clear but the repository contains ".0Z" in the revprop value, explicitly set by a "propset"; the ".000000Z" line is produced by "svnadmin dump" after going through from_cstring() and to_cstring(); and the ".0Z" line is produced by "svnrdump dump" which prints the property value sent by the server (after normalizing EOLs).

> > "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. [...]

That's not what's happening.

> > [...] such a transformation should have been done during "svnadmin load" [...]
>
> 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"? [...]
>
> 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.

I'm not yet sure what "load" should do.

> > 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.

In general I agree that's the right way round, but in this case I think the overriding decision is that this is a bug in "svnadmin dump" that the other utilities happen to have not copied.

> > 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).

Makes sense.

> It's possible
> that some 3rd-party tools rely on "svnadmin dump" producing
> timestamps in the ISO format.

There are two kinds of change in question: changing old format to new (ISO) format, and canonicalizing details (such as number of decimal places) in an ISO format date. The only case where dump would stop producing ISO format is where the repository contains old format dates. That can't happen "naturally" because a dump-load was required between v0.14 and v1.0 (and the dump was programmed to convert the format). It could only happen if users have loaded those old-format dates in again.

Before r871212 there was no validation of svn:date value entering the repo, I think, so old-format dates could have been put in.

But if anyone put old-format dates into their repo, and even if users do currently have some tools that expect the conversion to happen, I still don't see that as a good enough reason why future 'svnadmin dump' should not read out exactly what's in there.

It also happens to be a data bug that's extremely easy to fix -- just enable revprop changes and script the propedits.

> 2. Our dumpfile format is documented. With your proposed change, there
> is a slight chance that "svnadmin dump" could produce invalid dump
> files.

Invalid how? I don't see the contents of svn:date documented in 'notes/dump-load-format.txt'.

> 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.

-- 
- Julian