Hi Daniel. Thanks for reviewing...
(Dropping commits@ from the CC.)
Daniel Shahaf wrote:
> julianfoad_at_apache.org wrote:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_client_private.h?rev=1303016&r1=1303015&r2=1303016&view=diff
> ==============================================================================
>> --- subversion/trunk/subversion/include/private/svn_client_private.h
>> +++ subversion/trunk/subversion/include/private/svn_client_private.h
>> +/* Perform a symmetric merge.
>> + *
>> + * Merge according to MERGE into the WC at TARGET_WCPATH.
>> + */
>> +svn_error_t *
>> +svn_client__do_symmetric_merge(const svn_client__symmetric_merge_t *merge,
>> + const char *target_wcpath,
>> + svn_depth_t depth,
>> + svn_boolean_t ignore_ancestry,
>
> What does IGNORE_ANCESTRY mean in the context of symmetric merge? In
> particular, is it meaningful for the second merge in a 'sync A->B,
> sync A->B' scenario?
Clearly I need to fill in the doc strings.
IGNORE_ANCESTRY doesn't affect the high level operation of the merge, it only affects how file diffs are shown -- even if the source and
target file are not historically related it will show a diff rather than
a delete and an add of the file -- or something similar to that. From svn_client_merge4():
* Use @a ignore_ancestry to control whether or not items being
* diffed will be checked for relatedness first. Unrelated items
* are typically transmitted to the editor as a deletion of one thing
* and the addition of another, but if this flag is TRUE, unrelated
* items will be diffed as if they were related.
>> + svn_boolean_t force,
>> + svn_boolean_t record_only,
>> + svn_boolean_t dry_run,
>> + const apr_array_header_t *merge_options,
>> + svn_client_ctx_t *ctx,
>> + apr_pool_t *scratch_pool);
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1303016&r1=1303015&r2=1303016&view=diff
> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_client/merge.c
>> +++ subversion/trunk/subversion/libsvn_client/merge.c
>> @@ -10864,3 +10864,409 @@
>> +/* */
>> +static svn_error_t *
>> +find_symmetric_merge(repo_location_t **yca_p,
>> + repo_location_t **base_p,
>> + repo_location_t **mid_p,
>> + source_and_target_t *s_t,
>> + svn_client_ctx_t *ctx,
>> + apr_pool_t *result_pool,
>> + apr_pool_t *scratch_pool)
>> +{
>> + repo_location_t *yca, *base_on_source, *base_on_target, *mid;
>> +
>> + yca = apr_palloc(result_pool, sizeof(*yca));
>> + SVN_ERR(svn_client__get_youngest_common_ancestor(
>> + NULL, &yca->url, &yca->rev,
>> + s_t->source->url, s_t->source->rev,
>> + s_t->target->loc.url, s_t->target->loc.rev,
>> + ctx, result_pool));
>> + *yca_p = yca;
>> +
>> + /* Find the latest revision of A synced to B and the latest
>> + * revision of B synced to A.
>> + *
>> + * base_on_source = youngest_complete_synced_point(source, target)
>> + * base_on_target = youngest_complete_synced_point(target, source)
>> + */
>> + SVN_ERR(find_base_on_source(&base_on_source, s_t,
>> + ctx, scratch_pool, scratch_pool));
>> + SVN_ERR(find_base_on_target(&base_on_target, &mid, s_t,
>> + ctx, scratch_pool, scratch_pool));
[...]
>> + /* Choose a base. */
>> + if (base_on_source
>> + && (! base_on_target || (base_on_source->rev > base_on_target->rev)))
>> + {
>
> The last part of this condition seems arbitrary: in the criss-cross
> scenario, the order in which the 'criss' and the 'cross' are
> committed shouldn't affect the base the algorithm chooses.
Yes, that's true for a criss-cross. However, it's not a problem for normal cases; criss-cross is a rare case. As I wrote in the criss-cross merge section of <http://wiki.apache.org/subversion/SymmetricMerge>, in that case we probably should consider the relative ages of A1, B1, A3, B3, and A2, but I haven't yet thought about what's the best way to compare them.
>> + *base_p = base_on_source;
>> + *mid_p = NULL;
>> + }
>> + else if (base_on_target)
>> + {
>> + *base_p = base_on_target;
>> + *mid_p = mid;
>> + }
>> + else
>> + {
>> + /* No previous merge was found, so this is the simple case where
>> + * the base is the youngest common ancestor of the branches. We'll
>> + * set MID=NULL; in theory the end result should be the same if we
>> + * set MID=YCA instead. */
>> + *base_p = yca;
>> + *mid_p = NULL;
>> + }
>> +
>> + return SVN_NO_ERROR;
>> +}
Thanks.
- Julian
Received on 2012-03-22 09:54:47 CET