Re: [PATCH] Split up the reintegrate merge API: first find what to do, then do it - v2

DanielShahaf wrote:

> [ Caveat: I'm not at all familiar with merge.c ]

Thanks for casting your eyes over it anyway.

> Julian Foad wrote on Sun, Dec 11, 2011 at 19:35:04 +0000:
>>   The svn client reintegrate merge code calls:
>>
>>     svn_client_find_reintegrate_merge(&url1, &rev1, &url2,
> &rev2,
>> ...);     svn_client_merge4(url1, rev1, url2, rev2, ...);
> ...

> Looks good.

Thanks.

Look at the following patch hunk, and ideally look also at the earlier hunk where we see these changes are inside the function that was called 'reintegrate_merge_locked' but is now a cut-down function called 'find_reintegrate_merge'.  So ...

>> @@ -10628,31 +10631,76 @@ merge_reintegrate_locked(const char *sou
>>  
>>     /* Left side: trunk_at_youngest-trunk-rev-merged-to-branch-at-specified-peg-rev
>>     * Right side: branch_at_specified-peg-revision */
>> +  *source_p = apr_pmemdup(result_pool, &source, sizeof(source));
>> +  return SVN_NO_ERROR;
>> +}
>>  
>> -  /* Do the real merge! */
>> -  /* ### TODO(reint): Make sure that one isn't the same line ancestor
>> -    ### of the other (what's erroneously referred to as "ancestrally
>> -    ### related" in this source file).  We can merge to trunk without
>> -    ### implementing this. */
>> -  err = merge_cousins_and_supplement_mergeinfo(target_abspath,
>> -                                         target_ra_session,
>> -                                         source_ra_session,
>> -                                         &source, yc_ancestor_rev,
>> -                                         TRUE /* same_repos */,
>> -                                         svn_depth_infinity,
>> -                                         FALSE /* ignore_ancestry */,
>> -                                         FALSE /* force */,
>> -                                         FALSE /* record_only */,
>> -                                         dry_run,
>
> Could you clarify why this is removed?  I don't see it added elsewhere
> in the patch.  Is it a functional change?  Or do the diff hunks form an
> optical illusion here?

Instead of performing the merge, the (renamed) function now only finds the URLs and returns them.  Then, later on (in merge_reintegrate_locked), instead of calling this 'merge_cousins' function directly, we instead call 'merge_locked' (which is the guts of svn_client_merge4()) which calls 'merge_cousins'.

>
>> +merge_reintegrate_locked(const char *source_path_or_url,
>> +                        const svn_opt_revision_t *source_peg_revision,
>> +                        const char *target_abspath,
>> +                        svn_boolean_t dry_run,
>> +                        const apr_array_header_t *merge_options,
>> +                        svn_client_ctx_t *ctx,
>> +                        apr_pool_t *scratch_pool)
>> +{
>> +  if (source->url1)
>> +    {
>> +      svn_opt_revision_t revision1
>> +        = { svn_opt_revision_number, { source->rev1 } };
>> +      svn_opt_revision_t revision2
>> +        = { svn_opt_revision_number, { source->rev2 } };
>
> ISTR we had trouble in the past with some compilers not allowing these
> non-constant initializers.  (Fix would be to unroll the initialization
> into separate lines of code.)

AFAIK we've had this kind of initialization in the Subversion source for a long time now, so I'm treating it as de-facto acceptable even though not C'89.  I've been writing quite a few of these recently.  I can change them all to the long-winded alternative if proven necessary, but I hope it's not necessary because I really like the brevity.

- Julian
Received on 2011-12-11 21:54:04 CET