Re: [PATCH] Mergeinfo Elision

On Wed, 18 Apr 2007, Paul Burba wrote:
...
> > > }
> > >
> > > if (apr_hash_count(wc_mergeinfo) == 0 && @@ -897,6 +923,10 @@
> > > {
> > > svn_error_t *err;
> > >
> > > + /* Don't look any higher than the limit path. */
> > > + if (limit_path && strcmp(limit_path, wcpath) == 0)
> > > + break;
> > > +
> > > /* No explicit merge info on this path. Look
> > higher up the
> > > directory tree while keeping track of what
> > we've walked. */
> > > walk_path =
> > svn_path_join(svn_path_basename(wcpath, pool),
> > > @@ -956,6 +986,9 @@
> > > }
> > > }
> > >
> > > + if (walked_path)
> > > + *walked_path = walk_path;
> > > +
> > > return SVN_NO_ERROR;
> > > }
> > ...
> > >
> > >
> > /*--------------------------------------------------------------------
> > > ---*/
> > > >
> > > +/*** Eliding merge info. ***/
> >
> > I'm beginning to think that we should have a separate
> > mergeinfo.c in libsvn_client.
>
> Speaking of separation:
>
> I created private/svn_mergeinfo_private and moved
> svn_mergeinfo__to_string() there in r24648. As part of this patch I
> also added svn_mergeinfo__equals(). Both are defined in
> libsvn_subr/mergeinfo.c.
>
> Also in this patch I created private/svn_client_mergeinfo_private.h and
> moved svn_client__elide_mergeinfo() into it.

As previously mentioned in another thread, I moved
include/private/svn_client_mergeinfo_private.h to
libsvn_client/mergeinfo.h.
 
> Now onto your comment about mergeinfo.c: Did you want me to split
> libsvn_client/merge.c into merge.c and mergeinfo.c, similar to what
> lundblad did in r24045?

That's what I was thinking, yeah.

> svn_client__elide_mergeinfo(), svn_client_get_mergeinfo(), and the
> following file-private functions in merge.c could move to mergeinfo.c:
>
> get_wc_merge_info()
> *get_wc_or_repos_merge_info()
> mergeinfo_elides()
> elide_children()
> *calculate_merge_ranges()
> *determine_merges_performed()
> *update_wc_merge_info()
> *grok_range_info_from_opt_revisions()

I wouldn't classify grok_...() as a merge info-specific routine. It's
a merge meta data collection routine used by merge info-aware code.

> *discover_and_merge_children()
>
> * These functions are called from merge.c so would have to be declared
> in libsvn_client/client.h and get new "__" library internal names.

All of these should be marked with *, right?

It doesn't make sense to expand the scope of the file-private
routines -- those should stay put.

...
> > > + SVN_ERR(svn_mergeinfo_diff(&deleted, &added, mergeinfo,
> > child_mergeinfo,
> > > + subpool)); *elides =
> > > + (apr_hash_count(deleted) || apr_hash_count(added))
> > > + ? FALSE : TRUE;
> >
> > We should do a quick check here before diving down into the
> > relatively expensive svn_mergeinfo_diff() call.
> >
> > if (apr_hash_count(mergeinfo) != apr_hash_count(child_mergeinfo))
> > {
> > SVN_ERR(svn_mergeinfo_diff(&deleted, &added,
> > mergeinfo, child_mergeinfo,
> > subpool));
> > *elides = (apr_hash_count(deleted) == 0 &&
> > apr_hash_count(added) == 0);
> > }
> > else
> > {
> > *elides = FALSE;
> > }
>
> Even better? -- Check the count of PARENT_MERGEINFO and CHILD_MERGEINFO
> right at the start of the function.

Yeah, that looks great.

...
> > > +
> > > + /* Get mergeinfo for the target of the merge. */
> > > +
> > SVN_ERR(svn_client__parse_merge_info(&target_mergeinfo, entry,
> > > + target_wcpath,
> > adm_access,
> > > + ctx, pool));
> >
> > We could allocate target_mergeinfo out of a true subpool, but
> > since we've got an iterpool, we can't.
>
> Which is why I didn't pass subpool...I could create a true subpool, use
> it for target_mergefinfo, create iterpool out of subpool,and destroyed
> subpool at the end. Or is that overkill? I did a quick search and
> don't see any example of subpool/iterpool use in the same function
> anywhere eles.

I wouldn't bother using a sub-pool here.

...
> > > + }
> > > + else
> > > + {
> > > + int i;
> > > + apr_array_header_t *children_with_mergeinfo =
> > > + svn_sort__hash(children_with_mergeinfo_hash,
> > > + svn_sort_compare_items_as_paths, pool);
> > > +
> > > + /* children_with_mergeinfo is sorted in depth
> > first order.
> > > + To minimize svn_client__elide_mergeinfo()'s
> > crawls up the
> > > + working copy from each child, run through the
> > array backwards,
> > > + effectively doing a right-left post-order
> > traversal. */
> > > + for (i = children_with_mergeinfo->nelts -1; i >= 0; i--)
> > > + {
> > > + const char *child_wcpath;
> > > + svn_sort__item_t *item =
> > > + &APR_ARRAY_IDX(children_with_mergeinfo, i,
> > > + svn_sort__item_t);
> > > + child_wcpath = item->key;
> > > + /* Elide to target yes, but also elide target? */
> >
> > On one hand, we do want to elide the target, to avoid
> > spurious differences with the repository. On the other hand,
> > I think that eliding past our target is inconsistent with the
> > behavior of other Subversion operations. Thoughts?
>
> For switches we don't want to elide the target, I've seen the light on
> that, http://svn.haxx.se/dev/archive-2007-04/0678.shtml. I'll fix that
> tomorrow, when I tackle all the switch/mergeinfo problems.

Okay.

> Updates and merges are a different story though, I feel we should
> attempt to elide the target of a merge or update all the way to the root
> of the working copy. It may be inconsistent with other svn operations,
> but not with merge, at least not since merge-tracking. Here's what I
> mean: When we do 'svn merge URL WCPATH -cX', where WCPATH is not the
> root of the WC, we check the mergeinfo of WCPATH's parents to prevent
> repeated merges. So we already look up the WC above the merge target,
> it's fundamental to the merge-tracking implementation yes? The other
> thing to note is that we aren't making any local changes above the
> target, at most we are clearing the mergeinfo on the target because it
> elides. I don't see anything special about updates either that should
> exempt it from this.

Agreed.

> Now possibly users will find the behavior counter-intuitive or at least
> non-obvious, but other than that I don't see a problem.
...

IMHO, it's fine.