On Mon, Sep 28, 2015 at 10:55 AM, Stefan Fuhrmann <
stefan.fuhrm...@wandisco.com> wrote:
> On Wed, Sep 23, 2015 at 12:55 PM, Julian Foad <julianf...@btopenworld.com>
> wrote:
>
>> > Johan Corveleyn wrote:
>> >> [...] stefan2 told me in person that that part of the
>> >> change in r1572363 was unintentional :-). IIUC, he didn't realize that
>> >> it would have this effect on the output of dump.
>> [...]
>> >>>> I think the dump.c part of r1572363 and r1573111 should be reverted /
>> >>>> fixed so that we get the previous behaviour again, and this should be
>> >>>> backported to 1.9. At this point, IMO 'svnadmin dump' is broken in
>> >>>> 1.9.
>>
>> 1. I pretty much agree now that we should revert the change.
>>
>
> I suggest that we apply the patch below. It is more efficient
> that the original behaviour but ensures that "no-op" changes
> will be retained.
>
Now that I've read through the whole thread, here my view
on this problem plus an explanation why I think my patch is
the right approach.
First some facts and things that I believe:
* Internally, FSFS can record no-op prop and no-op content
changes (the delta between new representation and the
respective proplist / text of its predecessor is empty). There
is even the possibility of a no-op node change, i.e. a new
node without touching text nor props (not happening atm).
* SVN versions file trees and the decision how to represent
them (e.g. as delta) is an implementation detail.
* Relying on implementation details of a lower layer is bad.
* Via FS and Repos layer API, new instances of no-op changes
to either text and properties can be created.
* Dump files (produced by 3rd party tools or non-incremental
svnadmin dumps) are another source of those no-op changes.
* SVN editors don't produce no-op changes.
* A commit links user-controlled revision meta data (e.g. comment)
to a user-controlled list of changes. That link shall not be
lost when tweak implementation details.
* Dump / load is a compatibility interface between SVN versions
and allows for forward and backward migration.
* A plain node modification (no text nor prop change) will be
ignored by "svnadmin load". Even if we changed that on /trunk,
it would not fix old releases.
>From that, we can derive a few requirements:
* If the a revision contains a "M" change, svnadmin must produce
a dump file that causes previous releases to produce the "M"
change - within reason.
* Load shall produce no-op changes for the same data as in previous
releases - within reason. If the FS API would no longer support
creating no-op changes at some point in the future, the load process
should issue a warning.
* The dump file contents shall be independent of FS implementation
details, i.e. use "strict" FS API functions.
No-op changes are rare, hence including them into a dump file
does not significantly increase its size or processing time. The
text / proplist asymmetry in the patch is due to the fact that all
files due have a text but maybe no proplist representation. So,
dumping the text is always safe.
V2 of the patch also covers no-op prop changes to directories.
-- Stefan^2.
Index: subversion/libsvn_repos/dump.c
===================================================================
--- subversion/libsvn_repos/dump.c (revision 1705740)
+++ subversion/libsvn_repos/dump.c (working copy)
@@ -1168,9 +1168,28 @@
compare_root, compare_path,
eb->fs_root, path, pool));
if (kind == svn_node_file)
- SVN_ERR(svn_fs_contents_different(&must_dump_text,
- compare_root, compare_path,
- eb->fs_root, path, pool));
+ {
+ /* No-op changes are entirely legal. To cause 'svnadmin load' to
+ * re-create the node change, we must either include a prop or a
+ * text change (or both). Even if those end up to not change the
+ * contents, this will trigger the creation of the noderev itself
+ * and the respective changed paths list entry. */
+ if (must_dump_props)
+ SVN_ERR(svn_fs_contents_different(&must_dump_text,
+ compare_root, compare_path,
+ eb->fs_root, path, pool));
+ else
+ must_dump_text = TRUE;
+ }
+ else if (kind == svn_node_dir)
+ {
+ /* Modifying directories requires modifying their properties.
+ * Even if that was a no-op change, we again must dump the prop
+ * list to ensure 'svnadmin load' produces a modification on
+ * the directory. */
+ must_dump_props = TRUE;
+ }
+
break;
case svn_node_action_delete:
