You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by DanielShahaf <da...@elego.de> on 2011/12/11 21:29:43 UTC
Re: [PATCH] Split up the reintegrate merge API: first find what to
do, then do it - v2
[ Caveat: I'm not at all familiar with merge.c ]
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, ...);
...
> If no objections I plan to commit this, without the extra messages
> about two-url-command (which I'll come back to later).
Looks good.
> @@ -10628,31 +10631,76 @@ merge_reintegrate_locked(const char *sou
>
> /* Left side: trunk@youngest-trunk-rev-merged-to-branch-at-specified-peg-rev
> * Right side: branch@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?
> +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.)
>
Thanks!
Daniel
Re: [PATCH] Split up the reintegrate merge API: first find what to
do, then do it - v2
Posted by DanielShahaf <da...@elego.de>.
Julian Foad wrote on Sun, Dec 11, 2011 at 20:53:26 +0000:
> DanielShahaf wrote:
> > Julian Foad wrote on Sun, Dec 11, 2011 at 19:35:04 +0000:
> >> + 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.
I don't think it's necessary to change from the style used in the patch
if it works in practice.
Re: [PATCH] Split up the reintegrate merge API: first find what to
do, then do it - v2
Posted by DanielShahaf <da...@elego.de>.
Julian Foad wrote on Sun, Dec 11, 2011 at 20:53:26 +0000:
> 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@youngest-trunk-rev-merged-to-branch-at-specified-peg-rev
> >> * Right side: branch@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'.
>
OK. I can't find reintegrate_merge_locked(), but overall it sounds like
an intentional side-effect of gutting the actual merge out of
svn_client_find_reintegrate_merge(). Thanks for the explanation.
Re: [PATCH] Split up the reintegrate merge API: first find what to do, then do it - v2
Posted by Julian Foad <ju...@btopenworld.com>.
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@youngest-trunk-rev-merged-to-branch-at-specified-peg-rev
>> * Right side: branch@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