> -----Original Message-----
> From: Daniel Rall [mailto:dlr@collab.net]
> Sent: Thursday, May 31, 2007 6:20 PM
> To: Paul Burba
> Cc: dev@subversion.tigris.org
> Subject: WC merge info elision and paths with empty revision ranges
>
> On Tue, 29 May 2007, Paul Burba wrote:
> ...
> > > On Tue, 22 May 2007, Paul Burba wrote:
> ...
<snip>
> > The attached patch allows empty revision range merge info to be set
> > and handles almost all of the elision cases for such merge info.
> > Unlike my previous patch it uses the existing elision code
> > to prevent
> > spurious empty range merge info from being set on a path. There is
> > still a problem when there is no WC ancestor with merge
> > info to elide
> > to however
> > - see REMAINING PROBLEM below. Before I go further in solving that
> > problem though, I want to make sure no one objects to the idea of
> > 'partial' elision that this patch allows, as well as an
> > expansion of
> > what 'full' elision up till now has meant:
> >
> > ----------------------------------------
> >
> > NEW TYPE OF 'FULL' ELISION:
> >
> > If the merge info on PATH_CHILD consists *only* of paths
> > that map to
> > empty revision ranges, and *none* of these paths exist in
> > PATH_PARENT's merge info, then PATH_CHILD's merge info
> > elides to PATH_PARENT.
>
> I agree.
>
> Furthermore, if PATH_PARENT has merge info which
> contains a path with an empty revision range, CHILD_PATH
> should still elide to PATH_PARENT. (You probably aren't
> considering this to be new type of full elision.)
Agreed, well if what you are saying is the following:
If the merge info on PATH_CHILD is equivalent to the merge info on
PATH_PARENT, *except* for paths which exist *only* in PATH_PARENT and
map to empty rev ranges, then PATH_CHILD's merge info elides fully.
And I'm pretty sure that is what you are saying!
Though to be honest I can't quite see how we would ever end up with such
a situation...regardless I added a test of this (and every other case
mentioned here) in r25318.
> > e.g.
> > Properties on 'A/D':
> > svn:mergeinfo : /A_COPY/D:3-4
> > Properties on 'A/D/H':
> > svn:mergeinfo : /A_COPY_2/D/H:
> > svn:mergeinfo : /A_COPY_3/D/H:
> >
> > The merge info on 'merge_tests-1/A/D/H' would fully elide,
> > leaving no
> > merge info on 'merge_tests-1/A/D/H'.
> >
> > ----------------------------------------
> >
> > ANOTHER NEW TYPE OF 'FULL' ELISION:
> >
> > If the merge info on PATH_CHILD contains *some* paths that
> > don't exist
> > in PATH_PARENT's merge info and map to empty revision ranges, then
> > PATH_CHILD's merge info still elides if it is otherwise
> > equivalent to
> > PATH_PARNET's merge info.
>
> I agree. This is the converse of what I suggested above.
<snip>
> > ----------------------------------------
> >
> > 'PARTIAL' ELISION:
> >
> > If the merge info on PATH_CHILD contains *some* paths that
> > don't exist
> > in PATH_PARENT's merge info and map to empty revision
> > ranges, but the
> > other paths in PATH_CHILD's merge are *not* equivalent to
> > PATH_PARENT's merge info, then only the empty rev range
> > paths unique
> > to PATH_CHILD's merge info elide.
>
> Agreed!
<snip>
> > ----------------------------------------
> >
> > REMAINING PROBLEM:
> >
> > Currently the elision code only tries to elide merge info
> > the nearest
> > *working copy* ancestor with merge info. This causes a
> > problem with
> > this patch when there is no WC ancestor with merge info.
> >
> > e.g. A greek WC with the following changes:
> > r2 - Copy A to A_COPY
> > r3 - Text mod to A/D/H/psi
> >
> > >svn st merge_tests-1
> >
> > >svn pl -vR merge_tests-1
> > Properties on 'merge_tests-1\A_COPY':
> > svn:mergeinfo : /A:1
> >
> > # Merge to a target with no WC
> > # ancestors with merge info
> > >svn merge -c3 %URL%/A_COPY/D/H merge_tests-1\A\D\H
> > U merge_tests-1\A\D\H\psi
> >
> > >svn pl -vR merge_tests-1
> > Properties on 'merge_tests-1\A\D\H':
> > svn:mergeinfo : /A_COPY/D/H:3
> > Properties on 'merge_tests-1\A_COPY':
> > svn:mergeinfo : /A:1
> >
> > # Reverse the previous merge...
> > >svn merge -c-3 %URL%/A_COPY/D/H merge_tests-1\A\D\H
> > G merge_tests-1\A\D\H\psi
> >
> > # ...which leaves spurious empty range # merge info on the target.
> > >svn pl -vR merge_tests-1
> > Properties on 'merge_tests-1\A\D\H':
> > svn:mergeinfo : /A_COPY/D/H:
> > Properties on 'merge_tests-1\A_COPY':
> > svn:mergeinfo : /A:1
> >
> > >svn st merge_tests-1
> > M merge_tests-1\A\D\H
> >
> > We can't simply say, "don't set empty range merge info on a
> > target if
> > no WC ancestor with merge info is found". In this example
> > it would be
> > ok, but if /A/D/H was a disconnected working copy then we wouldn't
> > know if we were overriding the merge info of some ancestor
> > path in the
> > repository. This example could also be set up as a partial elision
> > sceario.
> >
> > The solution to this problem seems straightforward: check the
> > repository for the nearest ancestor with merge info if one
> > in the WC
> > cannot be found. svn_ra_get_mergeinfo() would need to be
> > changed to
> > indicate if the merge info it obtained was set explicitly
> > on a path or
> > was inherited, analogous to what merge.c:get_wc_mergeinfo() does.
What I ended up doing instead was adding a boolean arg to
svn_ra_get_mergeinfo() allowing us to specifically request inherited
mergeinfo only. This actually seemed a lot simpler. Simpler to use
anyhow, never having added a arg to svn_ra_* function I had no idea the
horrific amount of trickle down it causes :-\
> > svn_client__get_repos_mergeinfo() would be similarly
> > changed and then
> > the elision code could use it to decide if elision occurs or not.
> > Question is, do we want to elide merge info to the *repos*?
> > I don't
> > see any problems with this. Sound ok to you?
>
> Your suggested solution sounds reasonable. For operations
> which affect only the WC, I certainly don't want to elide
> merge info IN the repository, but considering the
> repository's merge info when performing eliding does sound
> like the way to go.
Agreed, never "IN" the repos, only TO it. The new patch attached
supports elision to the repos.
> > [[[
<snip>
> > + {
> > + /* "Move" paths with empty revision ranges
> which don't
> > + exist in PARENT_MERGEINFO from CHILD_MERGEINFO to
> > + *EMPTY_RANGE_MERGEINFO. */
> > + if (parent_mergeinfo == NULL
> > + || !apr_hash_get(parent_mergeinfo, child_key,
> > + APR_HASH_KEY_STRING))
> ^
> "== NULL" would be more explicit.
Made that change. You've mentioned this before, I'll try to be more
diligent about it in the future.
> > + {
> > + apr_hash_set(*empty_range_mergeinfo, child_key,
> > + APR_HASH_KEY_STRING, child_val);
>
> I guess we're not worried about APR pool lifetime issues? If
> we were, we'd duplicate "child_key" and "child_val" for
> *empty_range_mergeinfo using its pool. If we're not, we
> should indicate as much in the function's doc string.
I hadn't thought about this. Looking at similar functions like
svn_mergeinfo_diff(), which do copy the key/val pairs, I changed this to
follow suit.
> > + apr_hash_set(child_mergeinfo, child_key,
> > + APR_HASH_KEY_STRING, NULL);
> > + }
> > + }
> > + }
> > + }
> > + return SVN_NO_ERROR;
> > +}
> > +
> > +/* A tri-state value returned by mergeinfo_elides(). */
>
> Hmmm. While we don't really use elision_type_unknown, I
> wouldn't call this a tri-state enum. ;-)
elision_type_unknown wasn't ultimately needed so this is back to three
states now and completely within elide_mergeinfo().
> I would also indicate in the doc string and/or enum name that
> this has to do with eliding of WC merge info.
Done.
> > +enum elision_type
> > +{
> > + elision_type_unknown, /* Elision status not determined -
> > never returned by
> > + mergeinfo_elides(), used
> > internally only. */
> > + elision_type_none, /* No elision occurs. */
> > + elision_type_partial, /* Paths that exist only in
> > CHILD_MERGEINFO and
> > + map to empty revision ranges elide. */
> > + elision_type_full /* All merge info in
> > CHILD_MERGEINFO elides. */
> > +};
> > +
> > +/* Helper for svn_client__elide_mergeinfo() and elide_children().
> > +
> > + Compare PARENT_MERGEINFO and CHILD_MERGEINFO to see if
> > they are identical.
> > + If CHILD_MERGEINFO is NULL, ELISION_TYPE is set to
> > ELISION_TYPE_NONE. If
> > + PATH_SUFFIX and PARENT_MERGEINFO are not NULL append
> > PATH_SUFFIX to each
> > + path in PARENT_MERGEINFO before performing the comparison.
> > +
> > Set *ELIDES to whether PARENT_MERGEINFO and CHILD_MERGEINFO are
> > - identical (TRUE or FALSE). */
> > + identical (TRUE or FALSE).
>
> We no longer set *ELIDES -- it's been replaced by ELISION_TYPE.
N/A now that mergeinfo_elides() is now elide_mergeinfo().
> > +
> > + If CHILD_MERGEINFO consists only of paths mapped to
> > empty revision ranges,
> > + and these paths do not exist in PARENT_MERGEINFO ????. */
> > static svn_error_t *
> > -mergeinfo_elides(svn_boolean_t *elides,
> > +mergeinfo_elides(enum elision_type *elision_type,
> > apr_hash_t *parent_mergeinfo,
> > apr_hash_t *child_mergeinfo,
> > const char *path_suffix,
> > apr_pool_t *pool)
> > {
> > - apr_pool_t *subpool = svn_pool_create(pool);
> > - apr_hash_t *mergeinfo;
> > -
> > - if (parent_mergeinfo == NULL || child_mergeinfo == NULL ||
> > - apr_hash_count(parent_mergeinfo) !=
> > apr_hash_count(child_mergeinfo))
> > + apr_pool_t *subpool;
> > + apr_hash_t *mergeinfo, *child_empty_mergeinfo; svn_boolean_t
> > + equal_mergeinfo;
>
> We can defer declaration of "equal_mergeinfo" until later on.
Moved to appropriate block.
> > +
> > + /* Easy out: No child merge info to elide or no parent
> > to elide to.
> > + */ if (parent_mergeinfo == NULL || child_mergeinfo == NULL)
> > {
> > - *elides = FALSE;
> > + *elision_type = elision_type_none;
> > return SVN_NO_ERROR;
> > }
> >
> > + subpool = svn_pool_create(pool);
> > if (path_suffix)
> > {
> > apr_hash_index_t *hi;
> > @@ -1186,9 +1250,50 @@
> > mergeinfo = parent_mergeinfo;
> > }
> >
> > - SVN_ERR(svn_mergeinfo__equals(elides, child_mergeinfo, mergeinfo,
> > - subpool));
> > + /* Separate any merge info with empty rev ranges for
> > paths that exist only
> > + in CHILD_MERGEINFO and store these in
> > CHILD_EMPTY_MERGEINFO. */
> > + SVN_ERR(get_child_only_empty_revs(&child_empty_mergeinfo,
> > child_mergeinfo,
> > + mergeinfo, pool));
>
> IMO, it's not typically great practice to modify your inputs (as
> get_child_only_empty_revs() does). While
> get_child_only_empty_revs() documents that it's modifying its
> input, mergeinfo_elides() is not, but should if it's going to
> continue to do so.
I see how that could be a bad idea in light of the fact that we don't
typically do it in our code so its not expected, even when it is
documented. I was getting a bit greedy and tried to make a function
that tells us something about mergeinfo, mergeinfo_elides(), try to *do*
something about it too.
I was going to change mergeinfo_elides()/get_child_only_empty_revs() to
stop modifying their input hashes and instead return another hash with
the non-empty range merge info -- similar to what svn_mergeinfo_diff()
does -- but the simplicity of making mergeinfo_elides() actually *do*
the elision struck me and I changed its name to elide_mergeinfo(), added
wcpath and adm_access args, and now everything is a lot cleaner.
> > +
> > + *elision_type = elision_type_unknown;
>
> This can be set in an "else" block just below...
N/A now.
> >
> > + /* If *all* paths in CHILD_MERGEINFO map to empty
> > revision ranges and none
> > + of these paths exist in PARENT_MERGEINFO full elision
> > occurs; if only
> > + *some* of the paths in CHILD_MERGEINFO meet this
> > criteria we know, at a
> > + minimum, partial elision will occur. */ if
> > + (apr_hash_count(child_empty_mergeinfo) > 0)
> > + {
> > + *elision_type = apr_hash_count(child_mergeinfo) == 0
> > + ? elision_type_full : elision_type_partial;
> > + }
>
> else
> *elision_type = elision_type_unknown;
>
> ...and we could close the curlies from the "if" block, too.
^^^^^
lose I assume.
> > +
> > + if (*elision_type == elision_type_partial
> > + || *elision_type == elision_type_unknown)
> > + {
> > + /*If no determination of elision status has been
> > made yet or we
> > + know
> ^
> Missing space.
Fixed.
> > + only that partial elision occurs, compare
> > CHILD_MERGEINFO with the
> > + PATH_SUFFIX tweaked version of PARENT_MERGEINFO
> for equality.
> > +
> > + If we determined that at least partial elision
> > occurs, full elision
> > + may still yet occur if CHILD_MERGEINFO, which no
> > longer contains any
> > + paths unique to it that map to empty revision
> > ranges, is equivalent to
> > + PARENT_MERGEINFO. */
>
> Declare "equal_mergeinfo" here.
Things have changed bit since this first patch, but I moved it to an
appropriate block.
> > + SVN_ERR(svn_mergeinfo__equals(&equal_mergeinfo,
> > child_mergeinfo,
> > + mergeinfo, subpool));
> > + if (*elision_type == elision_type_partial)
> > + {
> > + if (equal_mergeinfo)
> > + *elision_type = elision_type_full;
> > + }
> > + else
> > + {
> > + *elision_type = equal_mergeinfo ? elision_type_full
> > + : elision_type_none;
> > + }
>
> This might be more clear if the "if (equal_mergeinfo)" check
> was the outermost.
Cleaned this up.
> > + }
> > +
> > +/* DELETE BEFORE COMMIT !!!!!!!!!!!! */ assert(*elision_type !=
> > +elision_type_unknown); /* DELETE BEFORE COMMIT !!!!!!!!!!!! */
> > +
>
> Hehe! <voice personality="borat">very nice</voice>
Nothing to see here anymore...move along :-)
> > svn_pool_destroy(subpool);
> > return SVN_NO_ERROR;
> > }
> > @@ -1228,7 +1333,8 @@
> > {
> > apr_hash_t *child_mergeinfo;
> > const char *child_wcpath;
> > - svn_boolean_t elides, switched;
> > + svn_boolean_t switched;
> > + enum elision_type elision_type;
> > const svn_wc_entry_t *child_entry;
> > svn_sort__item_t *item =
> > &APR_ARRAY_IDX(children_with_mergeinfo, i,
> >
> > svn_sort__item_t);
> > @@ -1285,13 +1391,18 @@
> > path_prefix =
> > svn_path_dirname(path_prefix, iterpool);
> > }
> >
> > - SVN_ERR(mergeinfo_elides(&elides, target_mergeinfo,
> > + SVN_ERR(mergeinfo_elides(&elision_type,
> > + target_mergeinfo,
> > child_mergeinfo,
> > path_suffix,
> > iterpool));
> > - if (elides)
> > + if (elision_type == elision_type_full)
> > SVN_ERR(svn_wc_prop_set2(SVN_PROP_MERGE_INFO, NULL,
> > child_wcpath,
> > adm_access, TRUE,
> > iterpool));
> > + else if (elision_type == elision_type_partial)
> > +
> > SVN_ERR(svn_client__record_wc_mergeinfo(child_wcpath,
> > +
> > child_mergeinfo,
> > + adm_access,
> > + iterpool));
>
> I would definitely use a "switch" statement here.
This logic is now in elide_mergeinfo() where it does use a switch.
> ...
> > @@ -1344,11 +1456,15 @@
> > if (mergeinfo == NULL)
> > return SVN_NO_ERROR;
> >
> > - SVN_ERR(mergeinfo_elides(&elides, mergeinfo,
> > target_mergeinfo,
> > + SVN_ERR(mergeinfo_elides(&elision_type, mergeinfo,
> > + target_mergeinfo,
> > NULL, pool));
> > - if (elides)
> > + if (elision_type == elision_type_full)
> > SVN_ERR(svn_wc_prop_set2(SVN_PROP_MERGE_INFO, NULL,
> > target_wcpath,
> > adm_access, TRUE,
> > pool));
> > + else if (elision_type == elision_type_partial)
> > + SVN_ERR(svn_client__record_wc_mergeinfo(target_wcpath,
> > +
> > target_mergeinfo,
> > + adm_access,
> > + pool));
> ...
>
> Ditto. These above two blocks look quite similar -- would
> refactoring into another small help function make sense?
Yes it would, consolidated in elide_mergeinfo()
-----
Ok, new patch and log message attached.
Thanks for taking the time to review the first patch, if you have the
time to take another look at this before I commit it would be much
appreciated.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 7 22:23:59 2007