Re: Fixing issue 644 [PATCH]

Philip Martin <philip@codematters.co.uk> writes:

> I suppose that rather than a PROPFIND we could use a CHECKOUT to
> detect if the file already exists. That looks quite simple to me (but
> I might be talking rubbish, I only started looking at HTTP/WebDAV 2
> days ago :-)

Yes, I probably was talking rubbish. Anyway, having tried a few
things here is a patch based on my original PROPFIND idea. Unlike the
other things I tried this method works, and has the benefit of
detecting the conflict in the early stages, before we get to sending
the postfix deltas.

Anybody think of a better way to do it?

* subversion/include/svn_error_codes.h: Add SVN_ERR_RA_ALREADY_EXISTS

* subversion/libsvn_ra_dav/ra_dav.h
  (svn_ra_dav__get_starting_props): New function.

* subversion/libsvn_ra_dav/props.c
  (svn_ra_dav__get_starting_props): New function

  (svn_ra_dav__get_baseline_info): Use svn_ra_dav__get_starting_props
  instead of svn_ra_dav__get_props_resource.

* subversion/libsvn_ra_dav/commit.c
  (commit_add_file): Call svn_ra_dav__get_starting_props to check whether
  file already exists.

  (resource_baton_t): Add created flag.

  (commit_open_root, commit_add_dir, commit_open_dir, commit_add_file,
  commit_open_file): Set created flag.

* subversion/tests/client/cmdline/commit_tests.py
  (commit_add_file_twice): New test

Index: ./subversion/include/svn_error_codes.h
===================================================================
--- ./subversion/include/svn_error_codes.h
+++ ./subversion/include/svn_error_codes.h Fri Apr 19 23:53:40 2002
@@ -349,6 +349,9 @@
     SVN_ERRDEF (SVN_ERR_RA_PROPS_NOT_FOUND,
                 "RA layer failed to fetch properties")
 
+ SVN_ERRDEF (SVN_ERR_RA_ALREADY_EXISTS,
+ "RA layer detected file already exists")
+
   /* End of ra_dav errors */
 
   /* These RA errors are specific to ra_local */
Index: ./subversion/tests/clients/cmdline/commit_tests.py
===================================================================
--- ./subversion/tests/clients/cmdline/commit_tests.py
+++ ./subversion/tests/clients/cmdline/commit_tests.py Sat Apr 20 00:22:03 2002
@@ -1286,6 +1286,66 @@
 
   return 0
 
+#----------------------------------------------------------------------
+
+# Issue #644 which failed over ra_dav.
+def commit_add_file_twice(sbox):
+ "issue 644 attempt to add a file twice"
+
+ if sbox.build():
+ return 1
+
+ wc_dir = sbox.wc_dir
+
+ # Create a file
+ gloo_path = os.path.join(wc_dir, 'A', 'D', 'H', 'gloo')
+ svntest.main.file_append(gloo_path, "hello")
+ svntest.main.run_svn(None, 'add', gloo_path)
+
+ # Create expected output tree.
+ output_list = [ [gloo_path, None, {}, {'verb' : 'Adding' }] ]
+ expected_output_tree = svntest.tree.build_generic_tree(output_list)
+
+ # Created expected status tree.
+ status_list = svntest.actions.get_virginal_status_list(wc_dir, '1')
+ status_list.append([gloo_path, None, {},
+ {'status' : 'A ',
+ 'wc_rev' : '0',
+ 'repos_rev' : '1'}]) # pre-commit status
+ for item in status_list:
+ item[3]['repos_rev'] = '2' # post-commit status
+ if (item[0] == gloo_path):
+ item[3]['wc_rev'] = '2'
+ item[3]['status'] = '_ '
+ expected_status_tree = svntest.tree.build_generic_tree(status_list)
+
+ # Commit should succeed
+ if svntest.actions.run_and_verify_commit (wc_dir,
+ expected_output_tree,
+ expected_status_tree,
+ None,
+ None, None,
+ None, None,
+ wc_dir):
+ return 1
+
+ # Update to state before commit
+ svntest.main.run_svn(None, 'up', '-r1', wc_dir)
+
+ # Create the file again
+ gloo_path = os.path.join(wc_dir, 'A', 'D', 'H', 'gloo')
+ svntest.main.file_append(gloo_path, "hello")
+ svntest.main.run_svn(None, 'add', gloo_path)
+
+ # Commit and *expect* a failure:
+ return svntest.actions.run_and_verify_commit (wc_dir,
+ None,
+ None,
+ "already exists",
+ None, None,
+ None, None,
+ wc_dir)
+
 ########################################################################
 # Run the tests
 
@@ -1311,6 +1371,7 @@
               commit_deleted_edited,
               commit_in_dir_scheduled_for_addition,
               commit_rmd_and_deleted_file,
+ commit_add_file_twice,
              ]
 
 if __name__ == '__main__':
Index: ./subversion/libsvn_ra_dav/ra_dav.h
===================================================================
--- ./subversion/libsvn_ra_dav/ra_dav.h
+++ ./subversion/libsvn_ra_dav/ra_dav.h Fri Apr 19 22:13:37 2002
@@ -214,6 +214,13 @@
                                              const ne_propname *which_props,
                                              apr_pool_t *pool);
 
+/* fetch a single resource's starting props from the server. */
+svn_error_t * svn_ra_dav__get_starting_props(svn_ra_dav_resource_t **rsrc,
+ ne_session *sess,
+ const char *url,
+ const char *label,
+ apr_pool_t *pool);
+
 /* fetch a single property from a single resource */
 svn_error_t * svn_ra_dav__get_one_prop(const svn_string_t **propval,
                                        ne_session *sess,
Index: ./subversion/libsvn_ra_dav/props.c
===================================================================
--- ./subversion/libsvn_ra_dav/props.c
+++ ./subversion/libsvn_ra_dav/props.c Fri Apr 19 22:15:15 2002
@@ -446,6 +446,16 @@
   return SVN_NO_ERROR;
 }
 
+svn_error_t * svn_ra_dav__get_starting_props(svn_ra_dav_resource_t **rsrc,
+ ne_session *sess,
+ const char *url,
+ const char *label,
+ apr_pool_t *pool)
+{
+ return svn_ra_dav__get_props_resource(rsrc, sess, url, label, starting_props,
+ pool);
+}
+
 svn_error_t *svn_ra_dav__get_baseline_info(svn_boolean_t *is_dir,
                                            svn_string_t *bc_url,
                                            svn_string_t *bc_relative,
@@ -503,8 +513,8 @@
 
     while (! svn_path_is_empty (path_s))
       {
- err = svn_ra_dav__get_props_resource(&rsrc, sess, path_s->data,
- NULL, starting_props, pool);
+ err = svn_ra_dav__get_starting_props(&rsrc, sess, path_s->data,
+ NULL, pool);
         if (! err)
           break; /* found an existing parent! */
 
Index: ./subversion/libsvn_ra_dav/commit.c
===================================================================
--- ./subversion/libsvn_ra_dav/commit.c
+++ ./subversion/libsvn_ra_dav/commit.c Fri Apr 19 23:53:57 2002
@@ -105,6 +105,7 @@
   resource_t *rsrc;
   apr_table_t *prop_changes; /* name/values pairs of changed (or new) properties. */
   apr_array_header_t *prop_deletes; /* names of properties to delete. */
+ svn_boolean_t created; /* set if this is an add rather than an update */
 } resource_baton_t;
 
 typedef struct
@@ -583,6 +584,7 @@
   root = apr_pcalloc(dir_pool, sizeof(*root));
   root->cc = cc;
   root->rsrc = rsrc;
+ root->created = FALSE;
 
   *root_baton = root;
 
@@ -649,6 +651,7 @@
   /* create a child object that contains all the resource urls */
   child = apr_pcalloc(dir_pool, sizeof(*child));
   child->cc = parent->cc;
+ child->created = TRUE;
   SVN_ERR( add_child(&child->rsrc, parent->cc, parent->rsrc,
                      name, 1, SVN_INVALID_REVNUM, dir_pool) );
 
@@ -726,6 +729,7 @@
   const char *name = svn_path_basename(path, dir_pool);
 
   child->cc = parent->cc;
+ child->created = FALSE;
   SVN_ERR( add_child(&child->rsrc, parent->cc, parent->rsrc,
                      name, 0, base_revision, dir_pool) );
 
@@ -800,9 +804,40 @@
   /* Construct a file_baton that contains all the resource urls. */
   file = apr_pcalloc(file_pool, sizeof(*file));
   file->cc = parent->cc;
+ file->created = TRUE;
   SVN_ERR( add_child(&file->rsrc, parent->cc, parent->rsrc,
                      name, 1, SVN_INVALID_REVNUM, file_pool) );
 
+ /* If the parent directory existed before this commit then there may be a
+ file with this URL already. We need to ensure such a file does not
+ exist, which we do by attempting a PROPFIND */
+ if (!parent->created)
+ {
+ svn_ra_dav_resource_t *res;
+ svn_error_t *err = svn_ra_dav__get_starting_props(&res,
+ file->cc->ras->sess,
+ file->rsrc->url, NULL,
+ file_pool);
+ if (!err)
+ {
+ /* If the PROPFIND succeeds the file already exists */
+ return svn_error_createf(SVN_ERR_RA_ALREADY_EXISTS, 0, NULL,
+ file_pool,
+ "file '%s' already exists", file->rsrc->url);
+ }
+ else if (err->apr_err == SVN_ERR_RA_REQUEST_FAILED)
+ {
+ /* ### TODO: This is what we get if the file doesn't exist
+ but an explicit not-found error might be better */
+ svn_error_clear_all(err);
+ }
+ else
+ {
+ /* A real error */
+ return err;
+ }
+ }
+
   if (! copyfrom_path)
     {
       /* This a truly new file. */
@@ -873,6 +908,7 @@
 
   file = apr_pcalloc(file_pool, sizeof(*file));
   file->cc = parent->cc;
+ file->created = FALSE;
   SVN_ERR( add_child(&file->rsrc, parent->cc, parent->rsrc,
                      name, 0, base_revision, file_pool) );
 

-- 
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org