Skip to content

Fix property propagation on file copies (issue 198).

Joseph Myers requested to merge jsm28/reposurgeon:copyperm into newsvn3

In some cases, SVN properties (and thus execute permission) are not properly propagated on file copies.

There are two different places in the SVN dump processing code that attempt to propagate properties from one node to another, whether when the node is changed (e.g. file content change) without repeating the unchanged properties in the dump file, or when it is copied as an SVN copy.

One such place uses propertyStash code in parseSubversion. This code is suspect, since it does not allow for directory copies or deletions, which is necessary to get the right set of properties for a given file at a given revision, and some of the logic only handles copying properties on a copy operation if those properties were actually set in the exact revision copied from.

Another place, in svnGenerateCommits, copies in the case where the node does not have properties but its ancestor does. That might be sufficient (if propertyStash had not actually set incorrect properties), except that the identification of ancestors is currently only sufficient for getting the right content, not the right properties, because of the shortcut case using node.fromHash.

This patch eliminates the shortcut in seekAncestor based on node.fromHash, instead checking at the end that, if a hash was provided, it does match the ancestor obtained by the other checks for sources of copies. This should ensure an ancestor with the correct properties, not just the correct blob. The logic in svnGenerateCommits is slightly changed, as it seems more correct to use the condition !node.propchange as necessary and sufficient for propagating properties from an ancestor (this should also fix any incorrect properties from the propertyStash code). The patch doesn't propagate properties in the "replace" case; it seems logically incorrect to do so (and empirically this may not matter because SVN seems to output properties explicitly for "replace" if there were any either before or after the replacement).

The included tests pass (and by inspection, the resulting permissions are right in a git checkout where they weren't before) and there are no regressions on other tests, but this has not yet been tested on the full GCC conversion.

I suspect that with this code, the propertyStash code should be useless at least as regards file properties. More work might be needed to eliminate it for directory properties.

Merge request reports