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.]
> 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?
> * 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.
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"?
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.
> @@ -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 :-) ! 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. (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()? 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),
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:
[[[
* 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
---------------------------------------------------------------------
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 19:05:38 CEST