You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2013/03/13 21:56:30 UTC
Re: 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.
[...]
> 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
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