subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From svn-r...@apache.org
Subject svn commit: r1667837 - in /subversion/branches/1.9.x: ./ STATUS subversion/libsvn_fs/fs-loader.c subversion/tests/cmdline/svnlook_tests.py subversion/tests/libsvn_fs/fs-test.c
Date Thu, 19 Mar 2015 18:24:01 GMT
Author: svn-role
Date: Thu Mar 19 18:24:00 2015
New Revision: 1667837

URL: http://svn.apache.org/r1667837
Log:
Merge r1663738 from trunk:

 * r1663738
   Stop exposing and prohibit changing internal txn props through FS API
   Justification:
     Prevents an implementation detail leak.  Prohibits changing the
     internal behavior of our transactions via public API.  Avoids a situation
     with the API function call discarding the data and falsely reporting
     success for "set" operations with 'svn:client-date' properties, i.e.,
     with the API telling us that the operation completed successfully when
     the change itself was not applied.  (The last part is new-in-1.9.)
   Notes:
     While this is mostly an API correctness fix, there is a user-visible
     consequence of 'svnlook proplist' no longer leaking internal properties
     like svn:check-locks for transactions.
   Votes:
     +1: kotkov, rhuijben, stefan2
     +0: philip (the new behaviour is fine but so is the old behaviour.
         svn:client-date is internal so does not have to obey the
         rules that apply to user properties. The proplist "leak" is
         strictly a regression as it is no longer possible to determine
         whether the CHECK_LOCKS flag is set on a txn.)

Modified:
    subversion/branches/1.9.x/   (props changed)
    subversion/branches/1.9.x/STATUS
    subversion/branches/1.9.x/subversion/libsvn_fs/fs-loader.c
    subversion/branches/1.9.x/subversion/tests/cmdline/svnlook_tests.py
    subversion/branches/1.9.x/subversion/tests/libsvn_fs/fs-test.c

Propchange: subversion/branches/1.9.x/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Thu Mar 19 18:24:00 2015
@@ -89,4 +89,4 @@
 /subversion/branches/verify-at-commit:1462039-1462408
 /subversion/branches/verify-keep-going:1439280-1546110
 /subversion/branches/wc-collate-path:1402685-1480384
-/subversion/trunk:1660545-1660547,1660549-1662901,1663003,1663183-1663184,1663338,1663347,1663374,1663450,1663697,1663706,1663749,1663791,1664078,1664080,1664084-1664085,1664187,1664191,1664200,1664344,1664476,1664480-1664481,1664483,1664507,1664520-1664521,1664523,1664526-1664527,1664531-1664532,1664588,1664927,1665164,1665611-1665612,1665845,1665850,1665852,1665886,1666270,1666272,1666690,1666851
+/subversion/trunk:1660545-1660547,1660549-1662901,1663003,1663183-1663184,1663338,1663347,1663374,1663450,1663697,1663706,1663738,1663749,1663791,1664078,1664080,1664084-1664085,1664187,1664191,1664200,1664344,1664476,1664480-1664481,1664483,1664507,1664520-1664521,1664523,1664526-1664527,1664531-1664532,1664588,1664927,1665164,1665611-1665612,1665845,1665850,1665852,1665886,1666270,1666272,1666690,1666851

Modified: subversion/branches/1.9.x/STATUS
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/STATUS?rev=1667837&r1=1667836&r2=1667837&view=diff
==============================================================================
--- subversion/branches/1.9.x/STATUS (original)
+++ subversion/branches/1.9.x/STATUS Thu Mar 19 18:24:00 2015
@@ -178,27 +178,6 @@ Veto-blocked changes:
 Approved changes:
 =================
 
- * r1663738
-   Stop exposing and prohibit changing internal txn props through FS API
-   Justification:
-     Prevents an implementation detail leak.  Prohibits changing the
-     internal behavior of our transactions via public API.  Avoids a situation
-     with the API function call discarding the data and falsely reporting
-     success for "set" operations with 'svn:client-date' properties, i.e.,
-     with the API telling us that the operation completed successfully when
-     the change itself was not applied.  (The last part is new-in-1.9.)
-   Notes:
-     While this is mostly an API correctness fix, there is a user-visible
-     consequence of 'svnlook proplist' no longer leaking internal properties
-     like svn:check-locks for transactions.
-   Votes:
-     +1: kotkov, rhuijben, stefan2
-     +0: philip (the new behaviour is fine but so is the old behaviour.
-         svn:client-date is internal so does not have to obey the
-         rules that apply to user properties. The proplist "leak" is
-         strictly a regression as it is no longer possible to determine
-         whether the CHECK_LOCKS flag is set on a txn.)
-
  * r1664193
    Fix win32 resource generation for svnbench.exe
    Justification:

Modified: subversion/branches/1.9.x/subversion/libsvn_fs/fs-loader.c
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/subversion/libsvn_fs/fs-loader.c?rev=1667837&r1=1667836&r2=1667837&view=diff
==============================================================================
--- subversion/branches/1.9.x/subversion/libsvn_fs/fs-loader.c (original)
+++ subversion/branches/1.9.x/subversion/libsvn_fs/fs-loader.c Thu Mar 19 18:24:00 2015
@@ -916,26 +916,48 @@ svn_fs_list_transactions(apr_array_heade
   return svn_error_trace(fs->vtable->list_transactions(names_p, fs, pool));
 }
 
+static svn_boolean_t
+is_internal_txn_prop(const char *name)
+{
+  return strcmp(name, SVN_FS__PROP_TXN_CHECK_LOCKS) == 0 ||
+         strcmp(name, SVN_FS__PROP_TXN_CHECK_OOD) == 0 ||
+         strcmp(name, SVN_FS__PROP_TXN_CLIENT_DATE) == 0;
+}
+
 svn_error_t *
 svn_fs_txn_prop(svn_string_t **value_p, svn_fs_txn_t *txn,
                 const char *propname, apr_pool_t *pool)
 {
+  if (is_internal_txn_prop(propname))
+    {
+      *value_p = NULL;
+      return SVN_NO_ERROR;
+    }
+
   return svn_error_trace(txn->vtable->get_prop(value_p, txn, propname, pool));
 }
 
 svn_error_t *
 svn_fs_txn_proplist(apr_hash_t **table_p, svn_fs_txn_t *txn, apr_pool_t *pool)
 {
-  return svn_error_trace(txn->vtable->get_proplist(table_p, txn, pool));
+  SVN_ERR(txn->vtable->get_proplist(table_p, txn, pool));
+
+  /* Don't give away internal transaction properties. */
+  svn_hash_sets(*table_p, SVN_FS__PROP_TXN_CHECK_LOCKS, NULL);
+  svn_hash_sets(*table_p, SVN_FS__PROP_TXN_CHECK_OOD, NULL);
+  svn_hash_sets(*table_p, SVN_FS__PROP_TXN_CLIENT_DATE, NULL);
+
+  return SVN_NO_ERROR;
 }
 
 svn_error_t *
 svn_fs_change_txn_prop(svn_fs_txn_t *txn, const char *name,
                        const svn_string_t *value, apr_pool_t *pool)
 {
-  /* Silently drop attempts to modify the internal property. */
-  if (!strcmp(name, SVN_FS__PROP_TXN_CLIENT_DATE))
-    return SVN_NO_ERROR;
+  if (is_internal_txn_prop(name))
+    return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
+                             _("Attempt to modify internal transaction "
+                               "property '%s'"), name);
 
   return svn_error_trace(txn->vtable->change_prop(txn, name, value, pool));
 }
@@ -946,25 +968,14 @@ svn_fs_change_txn_props(svn_fs_txn_t *tx
 {
   int i;
 
-  /* Silently drop attempts to modify the internal property. */
   for (i = 0; i < props->nelts; ++i)
     {
       svn_prop_t *prop = &APR_ARRAY_IDX(props, i, svn_prop_t);
 
-      if (!strcmp(prop->name, SVN_FS__PROP_TXN_CLIENT_DATE))
-        {
-          apr_array_header_t *reduced_props
-            = apr_array_make(pool, props->nelts - 1, sizeof(svn_prop_t));
-
-          for (i = 0; i < props->nelts; ++i)
-            {
-              prop = &APR_ARRAY_IDX(props, i, svn_prop_t);
-              if (strcmp(prop->name, SVN_FS__PROP_TXN_CLIENT_DATE))
-                APR_ARRAY_PUSH(reduced_props, svn_prop_t) = *prop;
-            }
-          props = reduced_props;
-          break;
-        }
+      if (is_internal_txn_prop(prop->name))
+        return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
+                                 _("Attempt to modify internal transaction "
+                                   "property '%s'"), prop->name);
     }
 
   return svn_error_trace(txn->vtable->change_props(txn, props, pool));

Modified: subversion/branches/1.9.x/subversion/tests/cmdline/svnlook_tests.py
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/subversion/tests/cmdline/svnlook_tests.py?rev=1667837&r1=1667836&r2=1667837&view=diff
==============================================================================
--- subversion/branches/1.9.x/subversion/tests/cmdline/svnlook_tests.py (original)
+++ subversion/branches/1.9.x/subversion/tests/cmdline/svnlook_tests.py Thu Mar 19 18:24:00
2015
@@ -695,7 +695,6 @@ fp.close()"""
                     "Properties on '/A':\n",
                     '  bogus_prop\n',
                     '  svn:log\n', '  svn:author\n',
-                    '  svn:check-locks\n', # internal prop, not really expected
                     '  bogus_rev_prop\n',
                     '  svn:date\n',
                     '  svn:txn-client-compat-version\n',

Modified: subversion/branches/1.9.x/subversion/tests/libsvn_fs/fs-test.c
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/subversion/tests/libsvn_fs/fs-test.c?rev=1667837&r1=1667836&r2=1667837&view=diff
==============================================================================
--- subversion/branches/1.9.x/subversion/tests/libsvn_fs/fs-test.c (original)
+++ subversion/branches/1.9.x/subversion/tests/libsvn_fs/fs-test.c Thu Mar 19 18:24:00 2015
@@ -5208,29 +5208,6 @@ commit_timestamp(const svn_test_opts_t *
 
   /* Commit that overwrites the specified svn:date. */
   SVN_ERR(svn_fs_begin_txn(&txn, fs, rev, pool));
-  {
-    /* Setting the internal property doesn't enable svn:date behaviour. */
-    apr_array_header_t *props = apr_array_make(pool, 3, sizeof(svn_prop_t));
-    svn_prop_t prop, other_prop1, other_prop2;
-    svn_string_t *val;
-
-    prop.name = SVN_FS__PROP_TXN_CLIENT_DATE;
-    prop.value = svn_string_create("1", pool);
-    other_prop1.name = "foo";
-    other_prop1.value = svn_string_create("fooval", pool);
-    other_prop2.name = "bar";
-    other_prop2.value = svn_string_create("barval", pool);
-    APR_ARRAY_PUSH(props, svn_prop_t) = other_prop1;
-    APR_ARRAY_PUSH(props, svn_prop_t) = prop;
-    APR_ARRAY_PUSH(props, svn_prop_t) = other_prop2;
-    SVN_ERR(svn_fs_change_txn_props(txn, props, pool));
-    SVN_ERR(svn_fs_txn_prop(&val, txn, other_prop1.name, pool));
-    SVN_TEST_ASSERT(val && !strcmp(val->data, other_prop1.value->data));
-    SVN_ERR(svn_fs_txn_prop(&val, txn, other_prop2.name, pool));
-    SVN_TEST_ASSERT(val && !strcmp(val->data, other_prop2.value->data));
-
-    SVN_ERR(svn_fs_change_txn_prop(txn, prop.name, prop.value, pool));
-  }
   SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
   SVN_ERR(svn_fs_make_dir(txn_root, "/bar", pool));
   SVN_ERR(svn_fs_change_txn_prop(txn, SVN_PROP_REVISION_DATE, date, pool));
@@ -6741,6 +6718,70 @@ test_prop_and_text_rep_sharing_collision
   return SVN_NO_ERROR;
 }
 
+static svn_error_t *
+test_internal_txn_props(const svn_test_opts_t *opts,
+                        apr_pool_t *pool)
+{
+  svn_fs_t *fs;
+  svn_fs_txn_t *txn;
+  svn_string_t *val;
+  svn_prop_t prop;
+  svn_prop_t internal_prop;
+  apr_array_header_t *props;
+  apr_hash_t *proplist;
+  svn_error_t *err;
+
+  SVN_ERR(svn_test__create_fs(&fs, "test-repo-internal-txn-props",
+                              opts, pool));
+  SVN_ERR(svn_fs_begin_txn2(&txn, fs, 0,
+                            SVN_FS_TXN_CHECK_LOCKS |
+                            SVN_FS_TXN_CHECK_OOD |
+                            SVN_FS_TXN_CLIENT_DATE, pool));
+
+  /* Ensure that we cannot read internal transaction properties. */
+  SVN_ERR(svn_fs_txn_prop(&val, txn, SVN_FS__PROP_TXN_CHECK_LOCKS, pool));
+  SVN_TEST_ASSERT(!val);
+  SVN_ERR(svn_fs_txn_prop(&val, txn, SVN_FS__PROP_TXN_CHECK_OOD, pool));
+  SVN_TEST_ASSERT(!val);
+  SVN_ERR(svn_fs_txn_prop(&val, txn, SVN_FS__PROP_TXN_CLIENT_DATE, pool));
+  SVN_TEST_ASSERT(!val);
+
+  SVN_ERR(svn_fs_txn_proplist(&proplist, txn, pool));
+  SVN_TEST_ASSERT(apr_hash_count(proplist) == 1);
+  val = svn_hash_gets(proplist, SVN_PROP_REVISION_DATE);
+  SVN_TEST_ASSERT(val);
+
+  /* We also cannot change or discard them. */
+  val = svn_string_create("Ooops!", pool);
+
+  err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_LOCKS, val, pool);
+  SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+  err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_LOCKS, NULL, pool);
+  SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+  err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_OOD, val, pool);
+  SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+  err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_OOD, NULL, pool);
+  SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+  err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CLIENT_DATE, val, pool);
+  SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+  err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CLIENT_DATE, NULL, pool);
+  SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+
+  prop.name = "foo";
+  prop.value = svn_string_create("bar", pool);
+  internal_prop.name = SVN_FS__PROP_TXN_CHECK_LOCKS;
+  internal_prop.value = svn_string_create("Ooops!", pool);
+
+  props = apr_array_make(pool, 2, sizeof(svn_prop_t));
+  APR_ARRAY_PUSH(props, svn_prop_t) = prop;
+  APR_ARRAY_PUSH(props, svn_prop_t) = internal_prop;
+
+  err = svn_fs_change_txn_props(txn, props, pool);
+  SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+
+  return SVN_NO_ERROR;
+}
+
 /* ------------------------------------------------------------------------ */
 
 /* The test table.  */
@@ -6872,6 +6913,8 @@ static struct svn_test_descriptor_t test
                        "test modify txn being written in FSFS"),
     SVN_TEST_OPTS_PASS(test_prop_and_text_rep_sharing_collision,
                        "test property and text rep-sharing collision"),
+    SVN_TEST_OPTS_PASS(test_internal_txn_props,
+                       "test setting and getting internal txn props"),
     SVN_TEST_NULL
   };
 



Mime
View raw message