On Fri, 2011-07-01, Paul Burba wrote:
> Paul Burba wrote:
> > Julian Foad wrote:
> >> I will just ask once more: as a matter of principle, are we comfortable
> >> it's OK to provide only an indication that "the server did in fact do
> >> this for you this time", but not to have a way of finding out in general
> >> whether the server is capable of doing this?
> >
> > On further reflection, I think using a server capabilities check is
> > the better approach:
[...]
> > I'll add the new capability if there are no objections.
>
> Done http://svn.apache.org/viewvc?view=revision&revision=1141981
Hi Paul. That looks good. I have some queries and suggestions about
the details, not all related specifically to this change, which I'm
attaching in the form of a patch (not to be committed as-is), which I'd
like your feedback on.
I wonder, do any callers really want to receive invalid mergeinfo? Is
this optional just because the server-side processing is slow and some
callers don't care?
I noticed some functions take an 'ignore_invalid_mergeinfo' parameter;
that confused me because I thought it was referring to the same thing
but instead it refers to a syntatically invalid svn:mergeinfo property
value. By contrast, what we're dealing with in the 'invalid_inherited'
case is mergeinfo that's syntactically valid but semantically invalid or
at least redundant because it refers to non-existent path-revs. I
wonder if it would be worth either using a different term (I know there
are loads of terms already, which can be confusing in itself) or
otherwise working towards simply eliminating this kind of mergeinfo from
our attention altogether by never exposing it and never giving the
caller the choice.
- Julian
Queries and suggestions on 'validate inherited mergeinfo'. A follow-up to
r1141981. Not to be committed as-is.
* subversion/libsvn_client/merge.c
(get_invalid_inherited_mergeinfo): Improve doc string. Ask questions; is
this function redundant?
(get_full_mergeinfo): Ask questions.
* subversion/libsvn_client/mergeinfo.c
(svn_client__get_wc_or_repos_mergeinfo_catalog, get_mergeinfo): Ask a
question.
* subversion/libsvn_client/mergeinfo.h
(svn_client__get_repos_mergeinfo): Improve doc string.
* subversion/libsvn_ra_neon/options.c
(svn_ra_neon__has_capability): Simplify.
* subversion/libsvn_ra_serf/options.c
(svn_ra_serf__has_capability): Simplify.
--This line, and those below, will be ignored--
* subversion/libsvn_client/merge.c
(fix_deleted_subtree_ranges):
(get_full_mergeinfo):
* subversion/libsvn_client/mergeinfo.c
(get_mergeinfo):
* subversion/libsvn_client/mergeinfo.h
(svn_client__get_wc_mergeinfo_catalog):
* subversion/libsvn_ra_neon/options.c
(svn_ra_neon__has_capability):
* subversion/libsvn_ra_serf/options.c
(svn_ra_serf__has_capability):
--This line, and those below, will be ignored--
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 1144309)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -3164,22 +3164,24 @@ fix_deleted_subtree_ranges(const char *u
mergeinfo.
Query the repository for the mergeinfo TARGET_ABSPATH inherits at its
- base revision and set *VALIDATED to indicate to the caller if we can
- determine what portions of that inherited mergeinfo are invalid.
+ base revision.
If no mergeinfo is inherited set *INVALID_INHERITED_MERGEINFO to NULL.
If only empty mergeinfo is inherited set *INVALID_INHERITED_MERGEINFO to
an empty hash.
- If non-empty inherited mergeinfo is inherited then, if the server
+ If non-empty mergeinfo is inherited then, if the server
supports the SVN_RA_CAPABILITY_VALIDATE_INHERITED_MERGEINFO capability,
remove all valid path-revisions from the raw inherited mergeinfo, and set
*INVALID_INHERITED_MERGEINFO to the remainder.
+ If non-empty mergeinfo is inherited but the server does not
+ support the SVN_RA_CAPABILITY_VALIDATE_INHERITED_MERGEINFO capability,
+ then ... ###?
+
Note that if validation occurs, but all inherited mergeinfo describes
- non-existent paths, then *INVALID_INHERITED_MERGEINFO is set to an empty
- hash.
+ existent paths, then *INVALID_INHERITED_MERGEINFO is set to an empty hash.
RA_SESSION is an open session that points to TARGET_ABSPATH's repository
location or to the location of one of TARGET_ABSPATH's parents. It may
@@ -3187,6 +3189,8 @@ fix_deleted_subtree_ranges(const char *u
RESULT_POOL is used to allocate *INVALID_INHERITED_MERGEINFO, SCRATCH_POOL
is used for any temporary allocations. */
+/* ### JAF: This function looks redundant. See the note "What are we doing
+ here?" at its call site. */
static svn_error_t *
get_invalid_inherited_mergeinfo(svn_mergeinfo_t *invalid_inherited_mergeinfo,
svn_ra_session_t *ra_session,
@@ -3311,6 +3315,10 @@ get_full_mergeinfo(svn_mergeinfo_t *reco
inherit, ra_session,
target_abspath,
ctx, scratch_pool));
+ /* ### JAF: Why is the variable called "inherited" when the output
+ * parameter from that function is called "indirect"? I'm unsure
+ * whether these words mean the same thing or else whether one
+ * implies the other. */
if (indirect)
*indirect = inherited;
@@ -3320,6 +3328,18 @@ get_full_mergeinfo(svn_mergeinfo_t *reco
{
svn_mergeinfo_t invalid_inherited_mergeinfo;
+ /* ### JAF: What are we doing here? We're asking for the *invalid*
+ * inherited mergeinfo (of TARGET_ABSPATH), and removing that from
+ * the total mergeinfo (of TARGET_ABSPATH) that was obtained from
+ * svn_client__get_wc_or_repos_mergeinfo(), so that we end up with
+ * what's known as 'validated' mergeinfo. But this is the only
+ * use of get_invalid_inherited_mergeinfo() - to remove the
+ * invalid mergeinfo from the result of another function. It
+ * seems this should be done (and could be done more efficiently)
+ * by asking svn_client__get_wc_or_repos_mergeinfo() to ignore
+ * invalid mergeinfo, and indeed its counterpart
+ * svn_client__get_wc_or_repos_mergeinfo_catalog(), which it
+ * calls, already has an option to do so. */
SVN_ERR(get_invalid_inherited_mergeinfo(
&invalid_inherited_mergeinfo,
ra_session, target_abspath, ctx,
Index: subversion/libsvn_client/mergeinfo.c
===================================================================
--- subversion/libsvn_client/mergeinfo.c (revision 1144309)
+++ subversion/libsvn_client/mergeinfo.c (working copy)
@@ -610,6 +610,7 @@ svn_client__get_wc_or_repos_mergeinfo_ca
const char *session_url = NULL;
apr_pool_t *sesspool = NULL;
svn_boolean_t validate_inherited_mergeinfo = FALSE;
+ /* ### JAF: Shouldn't we use a passed-in parameter instead? */
if (ra_session)
{
@@ -1067,6 +1068,7 @@ get_mergeinfo(svn_mergeinfo_catalog_t *m
{
svn_mergeinfo_catalog_t tmp_catalog;
svn_boolean_t validate_inherited_mergeinfo = FALSE;
+ /* ### JAF: Shouldn't we use a passed-in parameter instead? */
rev = peg_rev;
SVN_ERR(svn_client__get_repos_mergeinfo_catalog(
Index: subversion/libsvn_client/mergeinfo.h
===================================================================
--- subversion/libsvn_client/mergeinfo.h (revision 1144309)
+++ subversion/libsvn_client/mergeinfo.h (working copy)
@@ -168,17 +168,11 @@ svn_client__get_wc_mergeinfo_catalog(svn
doesn't support a mergeinfo capability and SQUELCH_INCAPABLE is
TRUE, set *TARGET_MERGEINFO to NULL.
- If the *TARGET_MERGEINFO for REL_PATH path is inherited and
- *VALIDATE_INHERITED_MERGEINFO is TRUE, then *TARGET_MERGEINFO
- will only contain merge source path-revisions that actually
- exist in repository.
-
- If the *TARGET_MERGEINFO for REL_PATH path is inherited,
+ If the mergeinfo for REL_PATH path is inherited,
VALIDATE_INHERITED_MERGEINFO is TRUE, and the server supports
the #SVN_RA_CAPABILITY_VALIDATE_INHERITED_MERGEINFO capability,
- then request that the server validate the mergeinfo in
- *TARGET_MERGEINFO, so it contains only merge source path-revisions
- that actually exist in repository. */
+ then *TARGET_MERGEINFO will only contain merge source path-revisions
+ that actually exist in the repository. */
svn_error_t *
svn_client__get_repos_mergeinfo(svn_ra_session_t *ra_session,
svn_mergeinfo_t *target_mergeinfo,
Index: subversion/libsvn_ra_neon/options.c
===================================================================
--- subversion/libsvn_ra_neon/options.c (revision 1144309)
+++ subversion/libsvn_ra_neon/options.c (working copy)
@@ -453,14 +453,8 @@ svn_ra_neon__has_capability(svn_ra_sessi
else
cap_result = capability_yes;
- if (strcmp(capability, SVN_RA_CAPABILITY_MERGEINFO) == 0)
- apr_hash_set(ras->capabilities,
- SVN_RA_CAPABILITY_MERGEINFO, APR_HASH_KEY_STRING,
- cap_result);
- else
- apr_hash_set(ras->capabilities,
- SVN_RA_CAPABILITY_VALIDATE_INHERITED_MERGEINFO,
- APR_HASH_KEY_STRING, cap_result);
+ apr_hash_set(ras->capabilities,
+ capability, APR_HASH_KEY_STRING, cap_result);
}
else
{
Index: subversion/libsvn_ra_serf/options.c
===================================================================
--- subversion/libsvn_ra_serf/options.c (revision 1144309)
+++ subversion/libsvn_ra_serf/options.c (working copy)
@@ -612,14 +612,8 @@ svn_ra_serf__has_capability(svn_ra_sessi
else
cap_result = capability_yes;
- if (strcmp(capability, SVN_RA_CAPABILITY_MERGEINFO) == 0)
- apr_hash_set(serf_sess->capabilities,
- SVN_RA_CAPABILITY_MERGEINFO, APR_HASH_KEY_STRING,
- cap_result);
- else
- apr_hash_set(serf_sess->capabilities,
- SVN_RA_CAPABILITY_VALIDATE_INHERITED_MERGEINFO,
- APR_HASH_KEY_STRING, cap_result);
+ apr_hash_set(serf_sess->capabilities,
+ capability, APR_HASH_KEY_STRING, cap_result);
}
else
{
