Fixed in r930175. grrr......
On Fri, Apr 2, 2010 at 01:20, Greg Stein <gst...@gmail.com> wrote: > This breaks: lock_tests 10, and switch_tests 17. > > Investigating... > > On Thu, Apr 1, 2010 at 18:09, <gst...@apache.org> wrote: >> 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__': >> >> >> >
