Mailing-List: contact commits-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@subversion.apache.org
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r930111 - in /subversion/trunk/subversion:
 libsvn_wc/update_editor.c tests/cmdline/trans_tests.py
Date: Thu, 01 Apr 2010 22:09:10 -0000
To: commits@subversion.apache.org
From: gstein@apache.org
Message-Id: <20100401220910.185E023889B3@eris.apache.org>

Author: gstein
Date: Thu Apr  1 22:09:09 2010
New Revision: 930111

URL: http://svn.apache.org/viewvc?rev=930111&view=rev
Log:
Fix a leaking temporary file in the update editor, including a test to
verify the leakage (and its fix).

Expand a bunch of comments, and relocate some work items for clarity.

* subversion/libsvn_wc/update_editor.c:
  (merge_file): remove the LEFT_VERSION and RIGHT_VERSION localvars. they
    can be reintroduced if they ever get used. add a bunch of commentary
    around the computation if IS_LOCALLY_MODIFIED. make sure that we
    delete the TMPTEXT temporary file after we've used it. relocated the
    deletion of the "copied text base" over to close_file().
  (close_file): remove the copied text base once we're done with it.

* subversion/tests/cmdline/trans_test.py:
  (props_only_file_update): new test to ensure that a file is
    retranslated on a props-only update.
  (test_list): add new test

Modified:
    subversion/trunk/subversion/libsvn_wc/update_editor.c
    subversion/trunk/subversion/tests/cmdline/trans_tests.py

Modified: subversion/trunk/subversion/libsvn_wc/update_editor.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/update_editor.c?rev=930111&r1=930110&r2=930111&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/update_editor.c (original)
+++ subversion/trunk/subversion/libsvn_wc/update_editor.c Thu Apr  1 22:09:09 2010
@@ -4269,10 +4269,6 @@ merge_file(svn_stringbuf_t **log_accum,
   svn_boolean_t is_replaced = FALSE;
   svn_boolean_t magic_props_changed;
   enum svn_wc_merge_outcome_t merge_outcome = svn_wc_merge_unchanged;
-  svn_wc_conflict_version_t *left_version = NULL; /* ### Fill */
-  svn_wc_conflict_version_t *right_version = NULL; /* ### Fill */
-
-  /* Accumulated entry modifications. */
   svn_wc_entry_t tmp_entry;
   apr_uint64_t flags = 0;
 
@@ -4285,6 +4281,7 @@ merge_file(svn_stringbuf_t **log_accum,
 
          - The .svn/entries file still reflects the old version of F.
 
+         ### there is no fb->old_text_base_path
          - fb->old_text_base_path is the old pristine F.
            (This is only set if there's a new text base).
 
@@ -4324,16 +4321,34 @@ merge_file(svn_stringbuf_t **log_accum,
      call reads the entries from the database the returned entry is
      svn_wc_schedule_replace.  2 lines marked ### EBUG below. */
   if (fb->copied_working_text)
-    is_locally_modified = TRUE;
+    {
+      /* The file was copied here, and it came with both a (new) pristine
+         and a working file. Presumably, the working file is modified
+         relative to the new pristine.
+
+         The new pristine is in NEW_TEXT_BASE_ABSPATH, which should also
+         be FB->COPIED_TEXT_BASE.  */
+      is_locally_modified = TRUE;
+    }
   else if (entry && entry->file_external_path
            && entry->schedule == svn_wc_schedule_replace) /* ###EBUG */
-    is_locally_modified = FALSE;
+    {
+      is_locally_modified = FALSE;
+    }
   else if (! fb->obstruction_found)
-    SVN_ERR(svn_wc__internal_text_modified_p(&is_locally_modified, eb->db,
-                                             fb->local_abspath, FALSE, FALSE,
-                                             pool));
+    {
+      /* The working file is not an obstruction. So: is the file modified,
+         relative to its ORIGINAL pristine?  */
+      SVN_ERR(svn_wc__internal_text_modified_p(&is_locally_modified, eb->db,
+                                               fb->local_abspath,
+                                               FALSE /* force_comparison */,
+                                               FALSE /* compare_textbases */,
+                                               pool));
+    }
   else if (new_text_base_abspath)
     {
+      /* We have a new pristine to install. Is the file modified relative
+         to this new pristine?  */
       SVN_ERR(svn_wc__internal_versioned_file_modcheck(&is_locally_modified,
                                                        eb->db,
                                                        fb->local_abspath,
@@ -4341,7 +4356,10 @@ merge_file(svn_stringbuf_t **log_accum,
                                                        FALSE, pool));
     }
   else
-    is_locally_modified = FALSE;
+    {
+      /* No other potential changes, so the working file is NOT modified.  */
+      is_locally_modified = FALSE;
+    }
 
   if (entry && entry->schedule == svn_wc_schedule_replace
       && ! entry->file_external_path)  /* ### EBUG */
@@ -4495,8 +4513,8 @@ merge_file(svn_stringbuf_t **log_accum,
               SVN_ERR(svn_wc__internal_merge(
                         log_accum, &merge_outcome,
                         eb->db,
-                        merge_left, left_version,
-                        new_text_base_abspath, right_version,
+                        merge_left, NULL,
+                        new_text_base_abspath, NULL,
                         fb->local_abspath,
                         fb->copied_working_text,
                         oldrev_str, newrev_str, mine_str,
@@ -4504,6 +4522,9 @@ merge_file(svn_stringbuf_t **log_accum,
                         eb->conflict_func, eb->conflict_baton,
                         eb->cancel_func, eb->cancel_baton,
                         pool));
+
+              /* ### now that we're past the internal_merge() (which might
+                 ### cause an exit), we can start writing work items.  */
               SVN_WC__FLUSH_LOG_ACCUM(eb->db, pb->local_abspath, *log_accum,
                                       pool);
 
@@ -4563,6 +4584,18 @@ merge_file(svn_stringbuf_t **log_accum,
           SVN_ERR(svn_wc__loggy_copy(log_accum, pb->local_abspath,
                                      tmptext, fb->local_abspath, pool, pool));
           SVN_WC__FLUSH_LOG_ACCUM(eb->db, pb->local_abspath, *log_accum, pool);
+
+          /* Done with the temporary file. Toss it.  */
+          {
+            const svn_skel_t *work_item;
+
+            SVN_ERR(svn_wc__wq_build_file_remove(&work_item,
+                                                 eb->db,
+                                                 tmptext,
+                                                 pool, pool));
+            SVN_ERR(svn_wc__db_wq_add(eb->db, pb->local_abspath,
+                                      work_item, pool));
+          }
         }
 
       if (lock_removed)
@@ -4628,17 +4661,6 @@ merge_file(svn_stringbuf_t **log_accum,
       SVN_WC__FLUSH_LOG_ACCUM(eb->db, pb->local_abspath, *log_accum, pool);
     }
 
-  /* Clean up add-with-history temp file. */
-  if (fb->copied_text_base)
-    {
-      const svn_skel_t *work_item;
-
-      SVN_ERR(svn_wc__wq_build_file_remove(&work_item,
-                                           eb->db, fb->copied_text_base,
-                                           pool, pool));
-      SVN_ERR(svn_wc__db_wq_add(eb->db, pb->local_abspath, work_item, pool));
-    }
-
   /* Set the returned content state. */
 
   /* This is kind of interesting.  Even if no new text was
@@ -4939,6 +4961,18 @@ close_file(void *file_baton,
   SVN_WC__FLUSH_LOG_ACCUM(eb->db, fb->dir_baton->local_abspath,
                           delayed_log_accum, pool);
 
+  /* Clean up any temporary files.  */
+  if (fb->copied_text_base)
+    {
+      const svn_skel_t *work_item;
+
+      SVN_ERR(svn_wc__wq_build_file_remove(&work_item,
+                                           eb->db, fb->copied_text_base,
+                                           pool, pool));
+      SVN_ERR(svn_wc__db_wq_add(eb->db, fb->dir_baton->local_abspath,
+                                work_item, pool));
+    }
+
   /* We have one less referrer to the directory's bump information. */
   SVN_ERR(maybe_bump_dir_info(eb, fb->bump_info, pool));
 

Modified: subversion/trunk/subversion/tests/cmdline/trans_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/trans_tests.py?rev=930111&r1=930110&r2=930111&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/trans_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/trans_tests.py Thu Apr  1 22:09:09 2010
@@ -790,6 +790,92 @@ def propset_revert_noerror(sbox):
   svntest.actions.run_and_verify_status(wc_dir, expected_status)
 
 
+def props_only_file_update(sbox):
+  "retranslation occurs on a props-only update"
+
+  sbox.build()
+  wc_dir = sbox.wc_dir
+
+  iota_path = os.path.join(wc_dir, 'iota')
+  content = ["This is the file 'iota'.\n",
+             "$Author$\n",
+             ]
+  content_expanded = ["This is the file 'iota'.\n",
+                      "$Author: jrandom $\n",
+                      ]
+
+  # Create r2 with iota's contents and svn:keywords modified
+  open(iota_path, 'w').writelines(content)
+  svntest.main.run_svn(None, 'propset', 'svn:keywords', 'Author', iota_path)
+
+  expected_output = wc.State(wc_dir, {
+    'iota' : Item(verb='Sending'),
+    })
+
+  expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+  expected_status.tweak('iota', wc_rev=2)
+
+  svntest.actions.run_and_verify_commit(wc_dir,
+                                        expected_output,
+                                        expected_status,
+                                        None,
+                                        wc_dir)
+
+  # Create r3 that drops svn:keywords
+
+  # put the content back to its untranslated form
+  open(iota_path, 'w').writelines(content)
+
+  svntest.main.run_svn(None, 'propset', 'svn:keywords', 'Id', iota_path)
+
+#  expected_output = wc.State(wc_dir, {
+#    'iota' : Item(verb='Sending'),
+#    })
+
+  expected_status.tweak('iota', wc_rev=3)
+
+  svntest.actions.run_and_verify_commit(wc_dir,
+                                        expected_output,
+                                        expected_status,
+                                        None,
+                                        wc_dir)
+
+  # Now, go back to r2. iota should have the Author keyword expanded.
+  expected_disk = svntest.main.greek_state.copy()
+  expected_disk.tweak('iota', contents=''.join(content_expanded))
+
+  expected_status = svntest.actions.get_virginal_state(wc_dir, 2)
+
+  svntest.actions.run_and_verify_update(wc_dir,
+                                        None, None, expected_status,
+                                        None,
+                                        None, None, None, None,
+                                        False,
+                                        wc_dir, '-r', '2')
+
+  # Update to r3. this should retranslate iota, dropping the keyword expansion
+  expected_disk = svntest.main.greek_state.copy()
+  expected_disk.tweak('iota', contents=''.join(content))
+
+  expected_status = svntest.actions.get_virginal_state(wc_dir, 3)
+
+  svntest.actions.run_and_verify_update(wc_dir,
+                                        None, expected_disk, expected_status,
+                                        None,
+                                        None, None, None, None,
+                                        False,
+                                        wc_dir)
+
+  # We used to leave some temporary files around. Make sure that we don't.
+  temps = os.listdir(os.path.join(wc_dir, '.svn', 'tmp'))
+  temps.remove('prop-base')
+  temps.remove('props')
+  temps.remove('text-base')
+  if temps:
+    print('Temporary files leftover: %s' % (', '.join(temps),))
+    raise svntest.Failure
+
+
 ########################################################################
 # Run the tests
 
@@ -807,6 +893,7 @@ test_list = [ None,
               copy_propset_commit,
               propset_commit_checkout_nocrash,
               propset_revert_noerror,
+              props_only_file_update,
              ]
 
 if __name__ == '__main__':