Re: svn commit: r37491 - trunk/subversion/libsvn_client

On Tue, Apr 28, 2009 at 01:16, Paul T. Burba <pburba_at_collab.net> wrote:
>...
> +++ trunk/subversion/libsvn_client/merge.c      Mon Apr 27 16:16:50 2009        (r37491)
>...
> @@ -2847,7 +2902,8 @@ get_full_mergeinfo(svn_mergeinfo_t *reco
>    function must have previously been called for PARENT.
>  */
>  static svn_error_t *
> -filter_merged_revisions(svn_client__merge_path_t *child,
> +filter_merged_revisions(svn_client__merge_path_t *parent,
> +                        svn_client__merge_path_t *child,
>                         const svn_wc_entry_t *entry,
>                         const char *mergeinfo_path,
>                         svn_mergeinfo_t target_mergeinfo,
> @@ -2942,12 +2998,33 @@ filter_merged_revisions(svn_client__merg
>           /* If we haven't already found CHILD->IMPLICIT_MERGEINFO then
>              contact the server to get it. */
>           if (! (child->implicit_mergeinfo))
> -            SVN_ERR(get_full_mergeinfo(NULL, &(child->implicit_mergeinfo),
> -                                       entry, NULL, svn_mergeinfo_inherited,
> -                                       ra_session, child->path,
> -                                       MAX(revision1, revision2),
> -                                       MIN(revision1, revision2),
> -                                       adm_access, ctx, subpool));
> +            {
> +              /* If CHILD has explicit mergeinfo only because its parent has
> +                 non-inheritable mergeinfo (see criteria 3 in
> +                 get_mergeinfo_paths() then CHILD can inherit PARENT's
> +                 implicit mergeinfo and we can avoid contacting the server.
> +
> +                 If child->child_of_noninheritable is true, it implies that
> +                 PARENT must exist per the rules of get_mergeinfo_paths(). */
> +              if (child->child_of_noninheritable)
> +                SVN_ERR(inherit_implicit_mereginfo_form_parent(parent,
> +                                                               child,
> +                                                               revision1,
> +                                                               revision2,
> +                                                               ra_session,
> +                                                               adm_access,
> +                                                               ctx,
> +                                                               pool));
> +              else
> +                SVN_ERR(get_full_mergeinfo(NULL,
> +                                           &(child->implicit_mergeinfo),
> +                                           entry, NULL,
> +                                           svn_mergeinfo_inherited,
> +                                           ra_session, child->path,
> +                                           MAX(revision1, revision2),
> +                                           MIN(revision1, revision2),
> +                                           adm_access, ctx, subpool));
> +            }
>
>           target_implicit_rangelist = apr_hash_get(child->implicit_mergeinfo,
>                                                    mergeinfo_path,
> @@ -3029,12 +3106,33 @@ filter_merged_revisions(svn_client__merg
>              ranges are represented there.  If we don't already have
>              CHILD->IMPLICIT_MERGEINFO then get it now. */
>           if (! (child->implicit_mergeinfo))
> -            SVN_ERR(get_full_mergeinfo(NULL, &(child->implicit_mergeinfo),
> -                                       entry, NULL, svn_mergeinfo_inherited,
> -                                       ra_session, child->path,
> -                                       MAX(revision1, revision2),
> -                                       MIN(revision1, revision2),
> -                                       adm_access, ctx, pool));
> +            {
> +              /* If CHILD has explicit mergeinfo only because its parent has
> +                 non-inheritable mergeinfo (see criteria 3 in
> +                 get_mergeinfo_paths() then CHILD can inherit PARENT's
> +                 implicit mergeinfo and we can avoid contacting the server.
> +
> +                 If child->child_of_noninheritable is true, it implies that
> +                 PARENT must exist per the rules of get_mergeinfo_paths(). */
> +              if (child->child_of_noninheritable)
> +                SVN_ERR(inherit_implicit_mereginfo_form_parent(parent,
> +                                                               child,
> +                                                               revision1,
> +                                                               revision2,
> +                                                               ra_session,
> +                                                               adm_access,
> +                                                               ctx,
> +                                                               pool));
> +              else
> +                SVN_ERR(get_full_mergeinfo(NULL,
> +                                           &(child->implicit_mergeinfo),
> +                                           entry, NULL,
> +                                           svn_mergeinfo_inherited,
> +                                           ra_session, child->path,
> +                                           MAX(revision1, revision2),
> +                                           MIN(revision1, revision2),
> +                                           adm_access, ctx, pool));
> +            }

The above couple blocks of code are *exactly* the same.

I see no rationale for condoning this for a backport. I don't know if
this is a single instance of the merge code bloat, or if it is
indicative. But we shouldn't see duplicative code like this.

Stop the bloat.

-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2073630
Received on 2009-05-06 01:06:04 CEST