Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords

Candidate for 1.0.1:

   * r8839,r8932
     Finish Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
     Justification: Allows malformed data into the repository.
     Votes:
       +1: kfogel, sussman
     Old votes (for r8839 alone):
       +1: cmpilato, jszakmeister
       -1: kfogel (doc string bug, fixed in r8932)

I am voting -1 for this, due to the "repair" of line endings. For the other reasons mentioned below I would have voted "-0".

A normal commit, via svn_wc_transmit_text_deltas, does not force repair of inconsistent line endings, but fails with an error. This patch to "import", on the other hand, silently "repairs" inconsistent line endings. I think that is the wrong thing for it to do, as I don't see any reason for it to behave differently from "commit".

This patch is on the fuzzy line between fix and enhancement. Lack of this patch only "allows malformed data" in the same way that any commit does if "svn:eol-style" and "svn:keywords" are not as intended. Behaviour without this patch is to preserve the file contents exactly. For these reasons I don't think there is a strong need to put this patch in 1.0.1.

I have reviewed the code but have not tested it. Of course, it would be nice to see regression tests for it, but I can't insist on that. I hope I can assume the "+1" reviewers tested it :-)

One minor thing I found in review is that, while the patch uses svn_string_create generally to create strings, in one place it was done long-hand. This additional patch tidies it up. but there is certainly no need to back-port this:

Index: subversion/libsvn_client/add.c
===================================================================
--- subversion/libsvn_client/add.c (revision 8939)
+++ subversion/libsvn_client/add.c (working copy)
@@ -121,12 +121,8 @@
       len = strlen (property);
       if (len > 0)
         {
- svn_string_t *propval = apr_pcalloc (autoprops->pool,
- sizeof (*propval));
- propval->data = this_value;
- propval->len = strlen (this_value);
-
- apr_hash_set (autoprops->properties, property, len, propval);
+ apr_hash_set (autoprops->properties, property, len,
+ svn_string_create (this_value, autoprops->pool));
           if (strcmp (property, SVN_PROP_MIME_TYPE) == 0)
             autoprops->mimetype = this_value;
           else if (strcmp (property, SVN_PROP_EXECUTABLE) == 0)

I will commit that tidy-up separately. (You know how much I like tidying up. I also know how dangerous it can be.)

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 9 00:29:17 2004