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

> -----Original Message-----
> From: Greg Stein [mailto:gstein_at_gmail.com]
> Sent: woensdag 6 mei 2009 1:06
> To: dev_at_subversion.tigris.org
> Subject: 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.

Working on an initial patch to address these concerns (to hopefully get the fix in 1.6.2) I found the 2 calls are /not/ identical.

The first call stores the result of get_full_mergeinfo in subpool, the second in pool.

As child lives longer than subpool, I think both should be stored in pool.

Paul, could you take a look at this patch? (and this pool lifetime issue).

        Bert

[[[
Stop the bloat.

* subversion/libsvn_client/merge.c
  (ensure_implicit_mergeinfo): New helper function.
  (filter_merged_revisions): Use ensure_implicit_mergeinfo instead of duplicating code.

Patch by: rhuijben
]]]

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2079191