You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2013/03/13 13:03:41 UTC
svn commit: r1455898 - in /subversion/trunk/subversion:
libsvn_client/resolved.c libsvn_wc/wc_db_update_move.c
tests/cmdline/stat_tests.py
Author: philip
Date: Wed Mar 13 12:03:41 2013
New Revision: 1455898
URL: http://svn.apache.org/r1455898
Log:
Detect unmodified files being updated during move-update.
* subversion/libsvn_client/resolved.c
(svn_client_resolve): Add sleep for timestamps.
* subversion/libsvn_wc/wc_db_update_move.c
(update_working_file): Detect unmodified local file and install
pristine rather than merging.
* subversion/tests/cmdline/stat_tests.py
(get_text_timestamp): Add string to exception.
(no_text_timestamp): New helper.
(move_update_timestamps): New test.
(test_list): Add new test.
Modified:
subversion/trunk/subversion/libsvn_client/resolved.c
subversion/trunk/subversion/libsvn_wc/wc_db_update_move.c
subversion/trunk/subversion/tests/cmdline/stat_tests.py
Modified: subversion/trunk/subversion/libsvn_client/resolved.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/resolved.c?rev=1455898&r1=1455897&r2=1455898&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/resolved.c (original)
+++ subversion/trunk/subversion/libsvn_client/resolved.c Wed Mar 13 12:03:41 2013
@@ -77,5 +77,7 @@ svn_client_resolve(const char *path,
err = svn_error_compose_create(err, svn_wc__release_write_lock(ctx->wc_ctx,
lock_abspath,
pool));
+ svn_io_sleep_for_timestamps(path, pool);
+
return svn_error_trace(err);
}
Modified: subversion/trunk/subversion/libsvn_wc/wc_db_update_move.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db_update_move.c?rev=1455898&r1=1455897&r2=1455898&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db_update_move.c (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db_update_move.c Wed Mar 13 12:03:41 2013
@@ -900,53 +900,73 @@ update_working_file(const char *local_re
if (!svn_checksum_match(new_version->checksum, old_version->checksum))
{
- /*
- * Run a 3-way merge to update the file, using the pre-update
- * pristine text as the merge base, the post-update pristine
- * text as the merge-left version, and the current content of the
- * moved-here working file as the merge-right version.
- */
- SVN_ERR(svn_wc__db_pristine_get_path(&old_pristine_abspath,
- db, wcroot->abspath,
- old_version->checksum,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_wc__db_pristine_get_path(&new_pristine_abspath,
- db, wcroot->abspath,
- new_version->checksum,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_wc__internal_merge(&work_item, &conflict_skel,
- &merge_outcome, db,
- old_pristine_abspath,
- new_pristine_abspath,
- local_abspath,
- local_abspath,
- NULL, NULL, NULL, /* diff labels */
- actual_props,
- FALSE, /* dry-run */
- NULL, /* diff3-cmd */
- NULL, /* merge options */
- propchanges,
- NULL, NULL, /* cancel_func + baton */
- scratch_pool, scratch_pool));
-
- work_items = svn_wc__wq_merge(work_items, work_item, scratch_pool);
+ svn_boolean_t is_locally_modified;
- if (merge_outcome == svn_wc_merge_conflict)
+ SVN_ERR(svn_wc__internal_file_modified_p(&is_locally_modified,
+ db, local_abspath,
+ FALSE /* exact_comparison */,
+ scratch_pool));
+ if (!is_locally_modified)
{
- content_state = svn_wc_notify_state_conflicted;
+ SVN_ERR(svn_wc__wq_build_file_install(&work_item, db,
+ local_abspath,
+ NULL,
+ FALSE /* FIXME: use_commit_times? */,
+ TRUE /* record_file_info */,
+ scratch_pool, scratch_pool));
+
+ work_items = svn_wc__wq_merge(work_items, work_item, scratch_pool);
+
+ content_state = svn_wc_notify_state_changed;
}
else
{
- svn_boolean_t is_locally_modified;
-
- SVN_ERR(svn_wc__internal_file_modified_p(&is_locally_modified,
- db, local_abspath,
- FALSE /* exact_comparison */,
- scratch_pool));
- if (is_locally_modified)
- content_state = svn_wc_notify_state_merged;
+ /*
+ * Run a 3-way merge to update the file, using the pre-update
+ * pristine text as the merge base, the post-update pristine
+ * text as the merge-left version, and the current content of the
+ * moved-here working file as the merge-right version.
+ */
+ SVN_ERR(svn_wc__db_pristine_get_path(&old_pristine_abspath,
+ db, wcroot->abspath,
+ old_version->checksum,
+ scratch_pool, scratch_pool));
+ SVN_ERR(svn_wc__db_pristine_get_path(&new_pristine_abspath,
+ db, wcroot->abspath,
+ new_version->checksum,
+ scratch_pool, scratch_pool));
+ SVN_ERR(svn_wc__internal_merge(&work_item, &conflict_skel,
+ &merge_outcome, db,
+ old_pristine_abspath,
+ new_pristine_abspath,
+ local_abspath,
+ local_abspath,
+ NULL, NULL, NULL, /* diff labels */
+ actual_props,
+ FALSE, /* dry-run */
+ NULL, /* diff3-cmd */
+ NULL, /* merge options */
+ propchanges,
+ NULL, NULL, /* cancel_func + baton */
+ scratch_pool, scratch_pool));
+
+ work_items = svn_wc__wq_merge(work_items, work_item, scratch_pool);
+
+ if (merge_outcome == svn_wc_merge_conflict)
+ {
+ content_state = svn_wc_notify_state_conflicted;
+ }
else
- content_state = svn_wc_notify_state_changed;
+ {
+ SVN_ERR(svn_wc__internal_file_modified_p(&is_locally_modified,
+ db, local_abspath,
+ FALSE /* exact_comparison */,
+ scratch_pool));
+ if (is_locally_modified)
+ content_state = svn_wc_notify_state_merged;
+ else
+ content_state = svn_wc_notify_state_changed;
+ }
}
}
else
Modified: subversion/trunk/subversion/tests/cmdline/stat_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/stat_tests.py?rev=1455898&r1=1455897&r2=1455898&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/stat_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/stat_tests.py Wed Mar 13 12:03:41 2013
@@ -630,7 +630,16 @@ def get_text_timestamp(path):
if re.match("^Text Last Updated", line):
return line
logger.warn("Didn't find text-time for %s", path)
- raise svntest.Failure
+ raise svntest.Failure("didn't find text-time")
+
+def no_text_timestamp(path):
+ "ensure no text-time for path using svn info"
+ exit_code, out, err = svntest.actions.run_and_verify_svn(None, None, [],
+ 'info', path)
+ for line in out:
+ if re.match("^Text Last Updated", line):
+ logger.warn("Found text-time for %s", path)
+ raise svntest.Failure("found text-time")
# Helper for timestamp_behaviour test
def text_time_behaviour(wc_dir, wc_path, status_path, expected_status, cmd):
@@ -2015,6 +2024,74 @@ def status_case_changed(sbox):
expected_status)
+def move_update_timestamps(sbox):
+ "timestamp behaviour for move-update"
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ sbox.simple_append('A/B/E/beta', 'X\nY\nZ\n', truncate=True)
+ sbox.simple_commit()
+ sbox.simple_append('A/B/E/alpha', 'modified alpha')
+ sbox.simple_append('A/B/E/beta', 'XX\nY\nZ\n', truncate=True)
+ sbox.simple_commit()
+ sbox.simple_update('', 2)
+
+ sbox.simple_append('A/B/E/beta', 'local beta')
+ src_time = get_text_timestamp(sbox.ospath('A/B/E/alpha'))
+ sbox.simple_move("A/B/E", "A/B/E2")
+ alpha_dst_time = get_text_timestamp(sbox.ospath('A/B/E2/alpha'))
+ beta_dst_time = get_text_timestamp(sbox.ospath('A/B/E2/beta'))
+ if src_time != alpha_dst_time:
+ raise svntest.Failure("move failed to copy timestamp")
+
+ expected_output = svntest.wc.State(wc_dir, {
+ 'A/B/E' : Item(status=' ', treeconflict='C'),
+ 'A/B/E/alpha' : Item(status=' ', treeconflict='U'),
+ 'A/B/E/beta' : Item(status=' ', treeconflict='U'),
+ })
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 3)
+ expected_status.tweak('A/B/E',
+ status='D ', treeconflict='C', moved_to='A/B/E2')
+ expected_status.tweak('A/B/E/alpha', 'A/B/E/beta', status='D ')
+ expected_status.add({
+ 'A/B/E2' : Item(status='A ', wc_rev='-', copied='+',
+ moved_from='A/B/E'),
+ 'A/B/E2/alpha' : Item(status=' ', wc_rev='-', copied='+'),
+ 'A/B/E2/beta' : Item(status='M ', wc_rev='-', copied='+'),
+ })
+ expected_disk = svntest.main.greek_state.copy()
+ expected_disk.remove('A/B/E', 'A/B/E/alpha', 'A/B/E/beta')
+ expected_disk.add({
+ 'A/B/E2' : Item(),
+ 'A/B/E2/alpha' : Item("This is the file 'alpha'.\n"),
+ 'A/B/E2/beta' : Item("X\nY\nZ\nlocal beta"),
+ })
+ svntest.actions.run_and_verify_update(wc_dir,
+ expected_output,
+ expected_disk,
+ expected_status)
+
+ time.sleep(1.1)
+ svntest.actions.run_and_verify_svn("resolve failed", None, [],
+ 'resolve',
+ '--accept=mine-conflict',
+ sbox.ospath('A/B/E'))
+ expected_status.tweak('A/B/E', treeconflict=None)
+ expected_status.tweak('A/B/E2/beta', status='M ')
+ svntest.actions.run_and_verify_status(wc_dir, expected_status)
+ expected_disk.tweak('A/B/E2/beta', contents="XX\nY\nZ\nlocal beta")
+ expected_disk.tweak('A/B/E2/alpha', contents="This is the file 'alpha'.\nmodified alpha")
+ svntest.actions.verify_disk(wc_dir, expected_disk)
+
+ # alpha is pristine so gets a new timestamp
+ new_time = get_text_timestamp(sbox.ospath('A/B/E2/alpha'))
+ if new_time == alpha_dst_time:
+ raise svntest.Failure("move failed to update timestamp")
+
+ # beta is modified so timestamp is removed
+ no_text_timestamp(sbox.ospath('A/B/E2/beta'))
+
########################################################################
# Run the tests
@@ -2061,6 +2138,7 @@ test_list = [ None,
status_not_present,
status_unversioned_dir,
status_case_changed,
+ move_update_timestamps,
]
if __name__ == '__main__':
Re: svn commit: r1455898 - in /subversion/trunk/subversion: libsvn_client/resolved.c libsvn_wc/wc_db_update_move.c tests/cmdline/stat_tests.py
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>>> +
>>> + if (merge_outcome == svn_wc_merge_conflict)
>>> + {
>>> + content_state = svn_wc_notify_state_conflicted;
>>> + }
>>> + else
>>> + {
>>> + SVN_ERR(svn_wc__internal_file_modified_p(&is_locally_modified,
>>> + db, local_abspath,
>>> + FALSE,
>>> + scratch_pool));
>>> + if (is_locally_modified)
>>> + content_state = svn_wc_notify_state_merged;
>>> + else
>>> + content_state = svn_wc_notify_state_changed;
>>> + }
>>
>> I don't understand that last 'else' block.
>>
>> We're asking 'Is it locally modified?' but I don't think we have a
>> consistent notion of what 'it' is, since the above svn_wc__internal_merge()
>> might or might not have updated the WC file at local_abspath -- it might
>> instead have created a temp file and set up some work items which will
>> install that file later [1].
>>
>> Do we actually want to ask here, 'Was the file locally modified before we
>> started merging?' If so, this whole block should be replaced with just
>> "content_state = svn_wc_notify_state_merged;".
>
> I'm a bit confused by that code as well but I didn't add that code in
> this commit I just changed the identation. It dates back to r1406631.
You didn't add this code but you repeated the 'svn_wc__internal_file_modified_p' call earlier, and moved all the existing code inside the 'else' of that earlier test, thus changing the conditions under which this can be executed -- it can no longer be executed if the file wasn't already locally modified.
I think the intent is to print 'U' if we're updating the file content
when it was not already locally modified, or 'G' if we are merging into a
file that was already locally modified. Or something close to that.
I'm not convinced that we're very consistent about this aspect of
updates and merges.
- Julian
Re: svn commit: r1455898 - in /subversion/trunk/subversion: libsvn_client/resolved.c libsvn_wc/wc_db_update_move.c tests/cmdline/stat_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
>> +
>> + if (merge_outcome == svn_wc_merge_conflict)
>> + {
>> + content_state = svn_wc_notify_state_conflicted;
>> + }
>> + else
>> + {
>
>> + SVN_ERR(svn_wc__internal_file_modified_p(&is_locally_modified,
>> + db, local_abspath,
>> + FALSE /* exact_comparison */,
>> + scratch_pool));
>> + if (is_locally_modified)
>> + content_state = svn_wc_notify_state_merged;
>> + else
>> + content_state = svn_wc_notify_state_changed;
>> + }
>
> I don't understand that last 'else' block.
>
> We're asking 'Is it
> locally modified?' but I don't think we have a consistent notion of what
> 'it' is, since the above svn_wc__internal_merge() might or might not
> have updated the WC file at local_abspath -- it might instead have created a temp file and set up
> some work items which will install that file later [1].
>
> Do
> we actually want to ask here, 'Was the file locally modified before we
> started merging?' If so, this whole block should be replaced with just
> "content_state = svn_wc_notify_state_merged;".
I'm a bit confused by that code as well but I didn't add that code in
this commit I just changed the identation. It dates back to r1406631.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1455898 - in /subversion/trunk/subversion: libsvn_client/resolved.c libsvn_wc/wc_db_update_move.c tests/cmdline/stat_tests.py
Posted by Julian Foad <ju...@btopenworld.com>.
> Author: philip
> Date: Wed Mar 13 12:03:41 2013
> New Revision: 1455898
>
> URL: http://svn.apache.org/r1455898
> Log:
> Detect unmodified files being updated during move-update.
>
> * subversion/libsvn_client/resolved.c
> (svn_client_resolve): Add sleep for timestamps.
>
> * subversion/libsvn_wc/wc_db_update_move.c
> (update_working_file): Detect unmodified local file and install
> pristine rather than merging.
[...]
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db_update_move.c
> + /*
> + * Run a 3-way merge to update the file, using the pre-update
> + * pristine text as the merge base, the post-update pristine
> + * text as the merge-left version, and the current content of the
> + * moved-here working file as the merge-right version.
> + */
> + SVN_ERR(svn_wc__db_pristine_get_path(&old_pristine_abspath,
> + db, wcroot->abspath,
> + old_version->checksum,
> + scratch_pool, scratch_pool));
> + SVN_ERR(svn_wc__db_pristine_get_path(&new_pristine_abspath,
> + db, wcroot->abspath,
> + new_version->checksum,
> + scratch_pool, scratch_pool));
> + SVN_ERR(svn_wc__internal_merge(&work_item, &conflict_skel,
> + &merge_outcome, db,
> + old_pristine_abspath,
> + new_pristine_abspath,
> + local_abspath,
> + local_abspath,
> + NULL, NULL, NULL, /* diff labels */
> + actual_props,
> + FALSE, /* dry-run */
> + NULL, /* diff3-cmd */
> + NULL, /* merge options */
> + propchanges,
> + NULL, NULL, /* cancel_func + baton */
> + scratch_pool, scratch_pool));
> +
> + work_items = svn_wc__wq_merge(work_items, work_item, scratch_pool);
> +
> + if (merge_outcome == svn_wc_merge_conflict)
> + {
> + content_state = svn_wc_notify_state_conflicted;
> + }
> + else
> + {
> + SVN_ERR(svn_wc__internal_file_modified_p(&is_locally_modified,
> + db, local_abspath,
> + FALSE /* exact_comparison */,
> + scratch_pool));
> + if (is_locally_modified)
> + content_state = svn_wc_notify_state_merged;
> + else
> + content_state = svn_wc_notify_state_changed;
> + }
I don't understand that last 'else' block.
We're asking 'Is it
locally modified?' but I don't think we have a consistent notion of what
'it' is, since the above svn_wc__internal_merge() might or might not
have updated the WC file at local_abspath -- it might instead have created a temp file and set up
some work items which will install that file later [1].
Do
we actually want to ask here, 'Was the file locally modified before we
started merging?' If so, this whole block should be replaced with just
"content_state = svn_wc_notify_state_merged;".
> }
> }
> else
[1] Actually in
r1432216 I edited the doc string of svn_wc__internal_merge() to say it
does not update the WC file. It looks like I was over-zealous there and should change it as follows:
[[[
Index: subversion/libsvn_wc/wc.h
===================================================================
--- subversion/libsvn_wc/wc.h (revision 1456105)
+++ subversion/libsvn_wc/wc.h (working copy)
@@ -391,6 +391,7 @@
/* Prepare to merge a file content change into the working copy. This
does not merge properties; see svn_wc__merge_props() for that. This
- does not change the working file on disk; it returns work items that
- will replace the working file on disk when they are run.
+ does not necessarily change the file TARGET_ABSPATH on disk; it may
+ instead return work items that will replace the file on disk when
+ they are run.
Merge the difference between LEFT_ABSPATH and RIGHT_ABSPATH into
]]]
- Julian