On Fri, May 2, 2008 at 1:05 PM, Karl Fogel <kfogel_at_red-bean.com> wrote:
> pburba_at_tigris.org writes:
> > Log:
> > Partial fix for issue #3187. Properly calculate ranges to reverse merge
> > on paths with non-inheritable revision ranges.
>
> [I saw your later propchange removing the word "partial" above.]
Yes, initially I thought there was more to do, but I was thinking of
issue #3188 which I spun off of #3187.
> > The bulk of this fix is a change to svn_rangelist_intersect API allowing
> > the caller to decide if inheritance is considered rather than considering
> > it by default. This makes svn_rangelist_intersect() more like
> > svn_rangelist_remove() which shares a common implementation.
> >
> > Note: If this is not backported before the final 1.5 RC it will have to be
> > reverted and svn_rangelist_intersect revved.
>
> *nod* Nice summary, thank you.
>
>
> > * subversion/include/svn_mergeinfo.h
> > (svn_rangelist_intersect): Add boolean consider_inheritance arg, tweak
> > doc string to reflect this.
> >
> > * subversion/libsvn_client/merge.c
> > (filter_merged_revisions): Update call to svn_rangelist_intersect() so as
> > to *not* consider inheritance. This is safe to do because paths whose
> > parent has non-inheritable ranges are always handled separately even if
> > that path has no explicit mergeinfo prior to the merge -- See condition
> > 3 in the doc string for merge.c:get_mergeinfo_paths().
>
> Everything from "This is safe..." to the end maybe should be a comment
> in the code, rather than going in the log message?
You are quite right, in r30961 I made to simplify
filter_merged_revisions() a little bit and improve its documentation a
lot.
> > * subversion/libsvn_client/mergeinfo.c
> > (filter_log_entry_with_rangelist):
> > * subversion/libsvn_client/ra.c
> > (svn_client__get_youngest_common_ancestor):
> > * subversion/tests/libsvn_subr/mergeinfo-test.c
> > (test_rangelist_intersect, test_rangelist_intersect_randomly):
> > Update calls to svn_rangelist_intersect(), always considering inheritance.
> > This is equivalent to the previous default behavior. The only call
> > changed from the previous default in this commit is in merge.c above.
>
> Gotcha.
>
>
> > * subversion/libsvn_subr/mergeinfo.c
> > (svn_rangelist_intersect): Add new consider_inheritance arg and pass it
> > to rangelist_intersect_or_remove() which does the real work for this
> > function and svn_rangelist_remove. If you were wondering, the latter
> > already has a consider_inheritance arg and it is what is used when
> > calculating forward merges, that is why issue #3187 only applies to
> > reverse merges.
>
> Might be best to put this entry right after the svn_mergeinfo.h change,
> since it's simply the .c followup to what the .h started.
Done.
> Good clarification about what the reader might have been wondering (I
> was, briefly, until I looked).
>
> > --- trunk/subversion/include/svn_mergeinfo.h (r30882)
> > +++ trunk/subversion/include/svn_mergeinfo.h (r30883)
>
> > @@ -267,6 +267,10 @@ svn_mergeinfo_intersect(svn_mergeinfo_t
> > * svn_merge_range_t * elements, @a rangelist1 and @a rangelist2, and
> > * place the result in @a *rangelist (which is never @c NULL).
> > *
> > + * @a consider_inheritance determines how to account for the
> > + * @c svn_merge_range_t inheritable field when comparing @a whiteboard's
> > + * and @a *eraser's rangelists for equality. @See svn_mergeinfo_diff().
> > + *
> > * Note: @a rangelist1 and @a rangelist2 must be sorted as said by @c
> > * svn_sort_compare_ranges(). @a *rangelist is guaranteed to be in sorted
> > * order.
> > @@ -276,6 +280,7 @@ svn_error_t *
> > svn_rangelist_intersect(apr_array_header_t **rangelist,
> > apr_array_header_t *rangelist1,
> > apr_array_header_t *rangelist2,
> > + svn_boolean_t consider_inheritance,
> > apr_pool_t *pool);
>
> You mention "whiteboard" and "eraser", which aren't parameters to this
> function, nor are they paramaters to the cited svn_mergeinfo_diff().
>
> Are you sure you didn't mean "mergeto" and "mergefrom"?
Cut and paste stupidity on my part, also fixed in r30959.
> Other than that, the meaning is clear.
>
> > --- trunk/subversion/libsvn_client/merge.c (r30882)
> > +++ trunk/subversion/libsvn_client/merge.c (r30883)
>
> > @@ -1529,7 +1529,7 @@ filter_merged_revisions(apr_array_header
> > SVN_ERR(svn_rangelist_reverse(requested_merge, pool));
> > SVN_ERR(svn_rangelist_intersect(remaining_ranges,
> > target_rangelist,
> > - requested_merge, pool));
> > + requested_merge, FALSE, pool));
> > SVN_ERR(svn_rangelist_reverse(*remaining_ranges, pool));
> > }
> > else
>
> Here is where the comment explaining "FALSE" might be good.
As noted above there is a detailed comment in r30961.
> > @@ -3445,10 +3445,10 @@ get_mergeinfo_paths(apr_array_header_t *
> > svn_mergeinfo_t mergeinfo;
> > SVN_ERR(svn_client__get_wc_mergeinfo
> > (&mergeinfo, &inherited, FALSE,
> > - svn_mergeinfo_nearest_ancestor,
> > - entry, child_of_noninheritable->path,
> > - merge_cmd_baton->target, NULL, adm_access,
> > - merge_cmd_baton->ctx, iterpool));
> > + svn_mergeinfo_nearest_ancestor,
> > + entry, child_of_noninheritable->path,
> > + merge_cmd_baton->target, NULL, adm_access,
> > + merge_cmd_baton->ctx, iterpool));
> >
> > SVN_ERR(svn_client__record_wc_mergeinfo(
> > child_of_noninheritable->path, mergeinfo, adm_access,
>
> I saw your later commit correcting this and other whitespace-only
> changes, so I'll ignore them here, along with the trivial parts of this
> commit: the addition of "TRUE" args, and the one-line change to pass
> along the consider_inheritance flag to rangelist_intersect_or_remove().
>
> So now the thing to do is evaluate the main claim of this commit:
>
>
> "This is safe to do because paths whose parent has non-inheritable
> ranges are always handled separately even if that path has no
> explicit mergeinfo prior to the merge -- See condition 3 in the doc
> string for merge.c:get_mergeinfo_paths()."
>
> It would be easier if filter_merged_revisions() actually documented its
> TARGET_MERGEINFO parameter, instead of merely saying it may be NULL, and
> documented its IMPLICIT_MERGEINFO, IS_ROLLBACK, and ENTRY parameters at
> all :-) !
Done in r30961.
> Especially given that the svn_rangelist_intersect() call in
> question only happens if IS_ROLLBACK is true, and depends ultimately on
> the value of IMPLICIT_MERGEINFO.
The svn_rangelist_intersect() depends on the value of
IMPLICIT_MERGEINFO? We combine IMPLICIT_MERGEINFO *and*
TARGET_MERGEINFO and then look if there are any ranges for
MERGEINFO_PATH in the combined result. If so then
svn_rangelist_intersect will be called to find out what has already
been merged. But...uh, I'm not sure why you are especially interested
in this. Do the comments in r30961 clarify this at all?
> (Luckily, there's only one call to
> filter_merged_revisions() in the entire codebase, so some can be figured
> out by looking at that call site in calculate_remaining_ranges() ).
>
> I'm assuming that you've traced all the code paths and know that we
> can't get to calculate_remaining_ranges() except via
> get_mergeinfo_paths()?
Yes...sort of :-)
do_directory_merge() always calls get_mergeinfo_paths() unless the
merge sources aren't related to each other or this is a foreign repos
merge (in that case the function returns).
Then do_directory_merge() calls populate_remaining_ranges(). That
function calls calculate_remaining_ranges() for each subtree found by
get_mergeinfo_paths(). Recall that case 3 in get_mergeinfo_paths()
assures us that any subtree with a parent with non-inheritable
mergeinfo is in this array of subtrees...
...Which is why I mentioned get_mergeinfo_paths() in the first place.
In case someone looked at my change and said, "Hey you are merging to
a directory with non-inheritable mergeinfo, how can you ignore this?
Won't the directories children not get a merge they need or get a
reverse merge they do need?" In hindsight perhaps I was answering a
question only I had :-(
As to the "sort of": do_file_merge() also calls
calculate_remaining_ranges() but if we are merging to a file we
obviously don't need to worry about the target's subtrees.
> Or is there some other reason why condition 3 in
> the doc string for get_mergeinfo_paths() governs here?
>
> If I can attempt to summarize the thinking: if we're rolling back, *and*
> a target_rangelist was found (ultimately via the IMPLICIT_MERGEINFO),
See my comment above.
> then we can and should ignore inheritance when getting the rangelist
> intersection, because... and here's where I break down. I want to fully
> understand this change, but I'm still not quite getting it. When you
> say that paths whose parent has non-inheritable ranges are "handled
> separately", how exactly are they are handled? I think if I understood
> that, everything else would fall into place. And then I could vote for
> this in STATUS with a clear conscience.
>
> Btw, I spotted what seemed to be some premature reallocations in
> filter_merged_revisions(), and would appreciate your review on the
> patch, just to make sure I'm not missing some lifetime issues:
The patch looks perfectly fine. I suspect when this code was first
written the author assumed we were only processing
children_WITH_mergeinfo, but some of the paths that come through here
don't have explicit mergeinfo.
> [[[
>
> * subversion/libsvn_client/merge.c
> (filter_merged_revisions): Avoid some premature re-allocations.
> ]]]
>
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 30947)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -1509,13 +1509,16 @@
> apr_pool_t *pool)
> {
> apr_array_header_t *target_rangelist = NULL;
> - svn_mergeinfo_t mergeinfo;
> + svn_mergeinfo_t mergeinfo = implicit_mergeinfo;
>
> if (is_rollback)
> {
> - mergeinfo = svn_mergeinfo_dup(implicit_mergeinfo, pool);
> if (target_mergeinfo)
> - SVN_ERR(svn_mergeinfo_merge(mergeinfo, target_mergeinfo, pool));
> + {
> + mergeinfo = svn_mergeinfo_dup(implicit_mergeinfo, pool);
> + SVN_ERR(svn_mergeinfo_merge(mergeinfo, target_mergeinfo, pool));
> + }
> +
> target_rangelist = apr_hash_get(mergeinfo,
> mergeinfo_path, APR_HASH_KEY_STRING);
> if (target_rangelist)
> @@ -1566,9 +1569,12 @@
> target_rangelist = apr_hash_get(target_mergeinfo,
> mergeinfo_path, APR_HASH_KEY_STRING);
> #else
> - mergeinfo = svn_mergeinfo_dup(implicit_mergeinfo, pool);
> if (target_mergeinfo)
> - SVN_ERR(svn_mergeinfo_merge(mergeinfo, target_mergeinfo, pool));
> + {
> + mergeinfo = svn_mergeinfo_dup(implicit_mergeinfo, pool);
> + SVN_ERR(svn_mergeinfo_merge(mergeinfo, target_mergeinfo, pool));
> + }
> +
> target_rangelist = apr_hash_get(mergeinfo,
> mergeinfo_path, APR_HASH_KEY_STRING);
> #endif
As always a thanks for the thorough review Karl,
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-02 23:01:37 CEST