Re: WC merge info elision and paths with empty revision ranges

On Mon, 11 Jun 2007, Paul Burba wrote:
...
> When I posted the previous patch I hadn't run the full 6-way tests to
> completion. When they finished there were two failures over http:
>
> FAIL: update_tests.py 22: update a schedule-add directory
> FAIL: copy_tests.py 21: copy a deleted directory back from the repos
>
> These were attributable to merge.c:get_wc_or_repos_mergeinfo() trying to
> get reposisory mergeinfo for WC scheduled add paths. Fixed that in
> attached patch, everything else is the same.
>
> Paul

> Support setting and elision of empty rev range mergeinfo and elision to repos.
>
> Prior to this patch we supported empty revision range merge info,
> e.g. svn:mergeinfo '/A/B:', in the sqlite db but we didn't allow it to be set
> on a working copy path. This patch allows that and also makes the mergeinfo
> elision code DTRT with empty revision range merge info, basically this means
                                                        ^^^
I'd start a new sentence here.

> that if empty revision range merge info has no meaning it elides. This patch
> also introduces the new concept of 'partial' elision, where only paths mapped
> to empty rev ranges elide, leaving other path mappings behind. Again, this is
> only done where the elision of the empty ranges has no semantic impact on the
> total merg einfo.
           ^^
Typo.

>
> Lastly this patch allows elision not only to a path's nearest WC ancestor but
> also it's nearest REPOS ancestor if a WC ancestor doesn't exist.
         ^
its

>
> * subversion/include/private/svn_fs_mergeinfo.h
> (svn_fs_mergeinfo__get_mergeinfo):
> * subversion/include/svn_fs.h
> (svn_fs_get_mergeinfo):
> * subversion/include/svn_ra.h
> (svn_ra_get_mergeinfo):
> * subversion/include/svn_repos.h
> (svn_repos_fs_get_mergeinfo):
> Add argument specifying retrieval of inherited merge info only.

You are going to hate -- well, perhaps only be irrated with -- me for
suggesting this, but I think the include_parents and parents_only
arguments should be collapsed into an enumeration (marshalled over the
wire as an integer). parents_only is only going to matter when
include_parents is true (looking below, the code ignores
include_parents when parents_only is set, but not a big difference
there), but even then has slightly different semantics which are best
expressed by a single parameter.

> * subversion/libsvn_client/copy.c
> (calculate_target_mergeinfo): Update call to
> svn_client__get_repos_mergeinfo().
>
> * subversion/libsvn_client/merge.c
> (get_wc_or_repos_mergeinfo): Add two new boolean arguments, one signaling
> retrieval of inherited merge info only, one signaling retrieval of merge
> info only from the repository. Update call to
> svn_client__get_repos_mergeinfo().
> (get_child_only_empty_revs): New helper for mergeinfo_elides(), finds merge
> info for paths in a child hash that map to empty revision ranges and don't
> exist in the parent hash.
> (mergeinfo_elides): Renamed to elide_mergeinfo()
> (elide_mergeinfo): New, replacement for mergeinfo_elides, now not only
> determines if elision occurs, but also performs the elision.
> (elide_children): Replace call to mergeinfo_elides() with elide_mergeinfo().
> (svn_client__elide_mergeinfo): Let helper elide_mergeinfo() test for *and*
> perform elision. If no working copy ancestor with mergeinfo is found permit
> possible check of the repository for ancestor mergeinfo. Rename
> elision_limit_path argument to more accurate wc_elision_limit_path and use
> it as a flag indicating whether to check the repos for ancestor merge info.
> (update_wc_mergeinfo): Allow empty revision range merge info to be set.
> (do_merge, do_single_file_merge): Update callers of
> get_wc_or_repos_mergeinfo().
> (svn_client_get_mergeinfo): Update calls to
> svn_client__get_repos_mergeinfo() and get_wc_or_repos_mergeinfo().
>
> * subversion/libsvn_client/mergeinfo.h
> (svn_client__get_repos_mergeinfo): Add boolean flag indicating only
> inherited mergeinfo should be accquired.
> (svn_client__elide_mergeinfo): Rename elision_limit_path argument to more
> accurate wc_elision_limit_path. Tweak docstring to reflect new empty rev
> range elision and repos elision behaviors.
>
> * subversion/libsvn_client/mergeinfo.c
> (svn_client__get_repos_mergeinfo): Add inherited_only arg, update call to
> svn_ra_get_mergeinfo().
>
> * subversion/libsvn_fs/fs-loader.h
> (struct root_vtable_t): Add parents_only arg to 'get_mergeinfo' hook.
>
> * subversion/libsvn_fs/fs-loader.c
> (svn_fs_get_mergeinfo): Add parents_only argument, update call to
> get_mergeinfo().
>
> * subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
> (get_mergeinfo_for_path): Add parents_only argument, update recursive call.
> (index_path_mergeinfo): Update call to get_mergeinfo_for_path().
> (get_mergeinfo): Add parents_only arg, update call to
> get_mergeinfo_for_path().
> (svn_fs_mergeinfo__get_mergeinfo): Add parents_only argument, update call
> to get_mergeinfo().
> (svn_fs_mergeinfo__get_mergeinfo_for_tree): Update call to get_mergeinfo().
>
> * subversion/libsvn_ra/ra_loader.h
> (struct svn_ra__vtable_t): Add parents_only arg to 'get_mergeinfo' hook.
>
> * subversion/libsvn_ra/ra_loader.c (svn_ra_dav__get_mergeinfo):
> * subversion/libsvn_ra_dav/mergeinfo.c (svn_ra_dav__get_mergeinfo):
> * subversion/libsvn_ra_dav/ra_dav.h (svn_ra_dav__get_mergeinfo):
> * subversion/libsvn_ra_serf/mergeinfo.c (svn_ra_serf__get_mergeinfo):
> * subversion/libsvn_ra_svn/client.c (ra_svn_get_merge_info):
> Add parents_only argument.

You missed update of the subversion/libsvn_ra_svn/protocol document.
It needs to be updated whenever we change the "svn" protocol.

There's a similar document at notes/webdav-protocol, but I'm not sure
it has the same level of detail. I don't recall ever having updated
it for a protocol tweak -- oops! In fact, it looks like it should
list the "merge-info-report" REPORT.

On a related note, this new REPORT also appears to be missing (?) from
dav_svn__reports_list in subversion/mod_dav_svn/dav_svn.h -- I'm
working up a patch to fix that.

...
> --- subversion/libsvn_client/merge.c (revision 25357)
> +++ subversion/libsvn_client/merge.c (working copy)
...
> + If PARENT_ONLY is TRUE only return inherited merge info.

We call this parents_only in a lot of other places.

> +
> + If TARGET_WCPATH inherited its merge info from a working copy ancestor
> + or if it was obtained from the repository, set *INDIRECT to TRUE, set it
> + to FALSE *otherwise. */
> static svn_error_t *
> get_wc_or_repos_mergeinfo(apr_hash_t **target_mergeinfo,
> const svn_wc_entry_t **entry,
> svn_boolean_t *indirect,
> + svn_boolean_t repos_only,
> + svn_boolean_t parent_only,
> svn_ra_session_t *ra_session,
> const char *target_wcpath,
> svn_wc_adm_access_t *adm_access,
> @@ -1103,27 +1110,34 @@
> ### differs from that of TARGET_WCPATH, and if those paths are
> ### directories, their children as well. */
>
> - SVN_ERR(get_wc_mergeinfo(target_mergeinfo, indirect, FALSE, *entry,
> - target_wcpath, NULL, NULL, adm_access, ctx,
> - pool));
> + if (repos_only)
> + *target_mergeinfo = NULL;
> + else
> + SVN_ERR(get_wc_mergeinfo(target_mergeinfo, indirect, parent_only, *entry,
> + target_wcpath, NULL, NULL, adm_access, ctx,
> + pool));
>
> /* If there in no WC merge info check the repository. */
> if (*target_mergeinfo == NULL)
> {
> apr_hash_t *repos_mergeinfo;
...
> + /* No need to check the repos is this is a local addition. */
> + if ((*entry)->schedule != svn_wc_schedule_add)

Great! This was a pre-existing problem then, yes?

> {
...
> + if (ra_session == NULL)
> + SVN_ERR(svn_client__open_ra_session_internal(&ra_session, url,
> + NULL, NULL, NULL, FALSE,
> + TRUE, ctx, pool));
> + SVN_ERR(svn_client__get_repos_mergeinfo(ra_session,
> + &repos_mergeinfo,
> + repos_rel_path, target_rev,
> + parent_only, pool));
> + if (repos_mergeinfo)
> + {
> + *target_mergeinfo = repos_mergeinfo;
> + *indirect = TRUE;
> + }
> }
> }
> return SVN_NO_ERROR;
...
> +/* Helper for elide_mergeinfo().
...
> + Find all paths in CHILD_MERGEINFO which map to empty revision ranges
> + and copy these from CHILD_MERGEINFO to *EMPTY_RANGE_MERGEINFO iff
> + PARENT_MERGEINFO is NULL or does not have merge info for the path in
> + question.

"parent_" prefix again. I rather prefer "parent_" to "parents_", come
to think of it.

> +
> + All mergeinfo in CHILD_MERGEINFO not copied to *EMPTY_RANGE_MERGEINFO
> + is copied to *NONEMPTY_RANGE_MERGEINFO.
> +
> + *EMPTY_RANGE_MERGEINFO and *NONEMPTY_RANGE_MERGEINFO are set to empty
> + hashes if nothing is copied into them.
> +
> + All copied hashes are deep copies allocated in POOL. */
> static svn_error_t *
...
> +get_child_only_empty_revs(apr_hash_t **empty_range_mergeinfo,

Function name is somewhat awkward. Maybe get_empty_rangelists_for_child()
or something similar?

...
> +/* Helper for svn_client__elide_mergeinfo() and elide_children().
> +
> + Given a working copy PATH, its mergeinfo hash CHILD_MERGEINFO, and the
> + mergeinfo of PATH's nearest ancestor PARENT_MERGEINFO, compare
> + CHILD_MERGEINFO to PARENT_MERGEINFO to see if the former elides to
> + the latter, following the elision rules described in
> + svn_client__elide_mergeinfo()'s docstring. If elision (full or partial)
> + does occur, then update PATH's mergeinfo appropriately. If CHILD_MERGEINFO
> + is NULL, do nothing.
> +
> + If PATH_SUFFIX and PARENT_MERGEINFO are not NULL append PATH_SUFFIX to each
> + path in PARENT_MERGEINFO before performing the comparison. */
> +static svn_error_t *
> +elide_mergeinfo(apr_hash_t *parent_mergeinfo,
...