[PATCH] Re: Working copy locks and client errors.

Greg Hudson <ghudson@MIT.EDU> writes:

> On Sun, 2002-08-11 at 18:57, Philip Martin wrote:
> > Does this sound sensible?
>
> This sounds reasonably. Another approach would be to set a flag in the
> lock indicating that it's currently protecting the running of a log
> file; that seems perhaps cleaner to me but more error-prone; your answer
> is probably better.

I was thinking of something like this patch. When an operation fails
locks are removed if cleanup is not required. Since the decision to
remove locks is not to be taken lightly, I'm posting the patch here
first. The important bit is the decision made by the new function
svn_wc__adm_is_cleanup_required, this is what determines when a lock
is removed; in this patch it is simply the presence of a log file.

Can anyone spot any holes?

* subversion/libsvn_wc/wc.h
  (svn_wc__adm_is_cleanup_required): New function.

* subversion/libsvn_wc/lock.c
  (svn_wc__adm_is_cleanup_required): New function.
  (pool_cleanup): Preserve physical locks only if required by
  svn_wc__is_cleanup_required.

* subversion/libsvn_wc/log.c
  (svn_wc_cleanup): Do critical cleanup only if svn_wc__is_cleanup_required
  requires it.

* subversion/tests/clients/cmdline/basic_tests.py
  (basic_corruption): Remove cleanup operation (reverts r2941).

* subversion/tests/clients/cmdline/copy_tests.py
  (no_wc_copy_overwrites): Remove cleanup operations (reverts r2941).

Index: subversion/libsvn_wc/wc.h
===================================================================
--- subversion/libsvn_wc/wc.h
+++ subversion/libsvn_wc/wc.h Tue Aug 13 19:38:05 2002
@@ -140,7 +140,14 @@
 
 /* Tell ADM_ACCESS that we are removing its physical lock, so that it can
    update its state information. */
-void svn_wc__adm_forced_lock_removal(svn_wc_adm_access_t *adm_access);
+void svn_wc__adm_forced_lock_removal (svn_wc_adm_access_t *adm_access);
+
+
+/* Set *CLEANUP to TRUE if the directory ADM_ACCESS requires cleanup; set
+ *CLEANUP to FALSE otherwise. */
+svn_error_t *svn_wc__adm_is_cleanup_required (svn_boolean_t *cleanup,
+ svn_wc_adm_access_t *adm_access,
+ apr_pool_t *pool);
 
 #ifdef __cplusplus
 }
Index: subversion/libsvn_wc/log.c
===================================================================
--- subversion/libsvn_wc/log.c
+++ subversion/libsvn_wc/log.c Tue Aug 13 19:47:24 2002
@@ -1291,7 +1291,7 @@
                                            SVN_WC__ADM_LOG, NULL);
   enum svn_node_kind kind;
   svn_wc_adm_access_t *adm_access;
- svn_boolean_t is_wc;
+ svn_boolean_t is_wc, cleanup;
 
   SVN_ERR (svn_wc_check_wc (path, &is_wc, pool));
   if (! is_wc)
@@ -1324,10 +1324,19 @@
         }
     }
 
- /* Is there a log? If so, run it. */
- SVN_ERR (svn_io_check_path (log_path, &kind, pool));
- if (kind == svn_node_file)
- SVN_ERR (svn_wc__run_log (adm_access, pool));
+ SVN_ERR (svn_wc__adm_is_cleanup_required (&cleanup, adm_access, pool));
+ if (cleanup)
+ {
+ /* As an attempt to maintain consitency between the decisions made in
+ the access baton lock-removal code, and those made here, we put
+ the important actions within the scope of the same cleanup
+ required test. */
+
+ /* Is there a log? If so, run it. */
+ SVN_ERR (svn_io_check_path (log_path, &kind, pool));
+ if (kind == svn_node_file)
+ SVN_ERR (svn_wc__run_log (adm_access, pool));
+ }
 
   /* Cleanup the tmp area of the admin subdir, if running the log has not
      removed it! The logs have been run, so anything left here has no hope
Index: subversion/libsvn_wc/lock.c
===================================================================
--- subversion/libsvn_wc/lock.c
+++ subversion/libsvn_wc/lock.c Tue Aug 13 19:48:39 2002
@@ -139,9 +139,12 @@
 pool_cleanup (void *p)
 {
   svn_wc_adm_access_t *lock = p;
+ svn_boolean_t cleanup;
   svn_error_t *err;
 
- err = do_close (lock, TRUE);
+ err = svn_wc__adm_is_cleanup_required (&cleanup, lock, lock->pool);
+ if (!err)
+ err = do_close (lock, cleanup);
 
   /* ### Is this the correct way to handle the error? */
   if (err)
@@ -508,6 +511,22 @@
 svn_wc__adm_forced_lock_removal (svn_wc_adm_access_t *adm_access)
 {
   adm_access->lock_exists = FALSE;
+}
+
+svn_error_t *
+svn_wc__adm_is_cleanup_required (svn_boolean_t *cleanup,
+ svn_wc_adm_access_t *adm_access,
+ apr_pool_t *pool)
+{
+ enum svn_node_kind kind;
+ const char *log_path = svn_wc__adm_path (svn_wc_adm_access_path (adm_access),
+ FALSE, pool, SVN_WC__ADM_LOG, NULL);
+
+ /* The presence of a log file demands cleanup */
+ SVN_ERR (svn_io_check_path (log_path, &kind, pool));
+ *cleanup = (kind == svn_node_file);
+
+ return SVN_NO_ERROR;
 }
 
 
Index: subversion/tests/clients/cmdline/copy_tests.py
===================================================================
--- subversion/tests/clients/cmdline/copy_tests.py
+++ subversion/tests/clients/cmdline/copy_tests.py Tue Aug 13 19:53:50 2002
@@ -516,15 +516,12 @@
   outlines, errlines = svntest.main.run_svn(1, 'cp', pi_path, rho_path)
   if not errlines:
     return 1
- svntest.main.run_svn(None, 'cleanup', wc_dir)
   outlines, errlines = svntest.main.run_svn(1, 'cp', pi_path, tau_path)
   if not errlines:
     return 1
- svntest.main.run_svn(None, 'cleanup', wc_dir)
   outlines, errlines = svntest.main.run_svn(1, 'cp', pi_path, alpha_path)
   if not errlines:
     return 1
- svntest.main.run_svn(None, 'cleanup', wc_dir)
 
   # Status after failed copies should not have changed
   extra_files = [ 'tau' ]
Index: subversion/tests/clients/cmdline/basic_tests.py
===================================================================
--- subversion/tests/clients/cmdline/basic_tests.py
+++ subversion/tests/clients/cmdline/basic_tests.py Tue Aug 13 19:53:50 2002
@@ -337,11 +337,8 @@
   os.chmod (tb_dir_path, tb_dir_saved_mode)
   os.chmod (mu_tb_path, mu_tb_saved_mode)
 
- outlines, errlines = svntest.main.run_svn(None, 'cleanup', other_wc)
- if errlines:
- return 1
-
- # This update should succeed.
+ # This update should succeed. (Actually, I'm kind of astonished
+ # that this works without even an intervening "svn cleanup".)
   return svntest.actions.run_and_verify_update (other_wc,
                                                 expected_output,
                                                 expected_disk,

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