Hi Paul. Thanks for the comprehensive reply and sorry for the wait.
Responses in line.
On Tue, 2011-07-12, Paul Burba wrote:
> On Fri, Jul 8, 2011 at 1:04 PM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> > 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.
>
> Hi Julian,
>
> Sorry for the long wait, your questions have prompted some extensive
> thinking about this issue #3668 and issue #3669 code.
>
> I've included the feedback on the patch inline at the end of this message.
>
> > 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?
>
> It doesn't matter to some callers (merge.c:calculate_left_hand_side(),
> merge.c:find_unmerged_mergeinfo()), so they don't impose the overhead
> on the server...
>
> ...but sometimes we really do want the invalid mergeinfo -- see my
> comments on your patch for merge.c:get_full_mergeinfo().
>
> > 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)
>
> It would be more accurate to compare the boolean
> ignore_invalid_mergeinfo parameter to the boolean
> validate_inherited_mergeinfo parameter. Those are both input
> parameters where 'invalid_inherited' is an svn_mergeinfo_t output
> parameter. I'm very open to any suggested name changes, but there's
> no getting away from the reality that there are a lot of terms.
>
> I have been guilty of occasionally referring to non-existent path-rev
> mergeinfo (which is otherwise syntactically valid) as "invalid" in
> some comments. I'll clean that up to the extent possible, but I do
> rely on context at times to point to which type of "invalid" I'm
> talking about (it is such a useful word after all :-)
Well, it's not just comments: there is this function called
get_invalid_inherited_mergeinfo() that returns its result in a parameter
called "invalid_inherited_mergeinfo". But maybe that's the only
remaining place where 'invalid' is used in that sense, I haven't
checked.
> > 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.
>
> Your question has prompted me to change the default behavior of
> svn_client__get_wc_or_repos_mergeinfo and
> svn_client__get_wc_or_repos_mergeinfo_catalog such that they always
> request inherited mergeinfo validation *if* contacting the server. I
> can't see that any callers actually rely on the previous behavior and
> I'd rather default to correct mergeinfo and deal with the problems (if
> any) that causes.
Sounds good.
> [[[
> > Index: subversion/libsvn_client/merge.c
> > ===================================================================
[re. get_invalid_inherited_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 ... ###?
>
> If the server does not have this capability and non-empty mergeinfo is
> inherited, then *INVALID_INHERITED_MERGEINFO would be set to an empty
> hash...*BUT* this function is only intended to be called if the server
> has that capability. I'll add that to the doc string.
Thanks.
> > 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.
>
> Your change is the complete opposite of what the code is doing, but
> perhaps it's because the original wording isn't clear enough. How's
> this instead:
>
> Note that if validation occurs and all inherited mergeinfo is
> discarded as non-existent, then *INVALID_INHERITED_MERGEINFO is set to
> an empty hash.
That (and your r1145653 edit) still looks the wrong way around.
Because, unless I'm misreading the double-negatives, this function
supposedly returns a set of mergeinfo that refers to *non-existent*
path-revs.
> I reworked this entire doc string in r1145653. Hopefully it makes bit
> more sense now.
It's good apart from the bit above, thanks.
> > + /* ### 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;
>
> They are synonymous, I purged use of the 'indirect' term in r1145202
Thanks.
> > + /* ### 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.
>
> What's going on here is
[...]
> So we want to keep what README inherits from its working copy parent
> less any inherited mergeinfo that the repository can confirm describes
> non-existent paths.
OK, now I understand. Thank you for taking the time to spell this out.
> In r1145653 I conditioned this block so it only occurs when mergeinfo
> is inherited from the working copy. We could further optimize this by
> only entering the block if the target inherits mergeinfo from a
> working copy parent with local mergeinfo changes, but I want to get
> your feedback first.
I'm tempted to think of radically different ways of achieving the
desired result, and I suggest leaving it as it is now, not trying to
optimize it further. I might see if I can put a comment in here that
would have helped me. (I know that, as a new reader, the things I want
to know are not necessarily the things that you as the writer would
always think of writing.)
> > 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. */
>
> As I think you realized based on your other questions, the
> ignore_invalid_mergeinfo parameter has nothing to do with validating
> inherited mergeinfo parameter.
Oh yes, I did realize after writing that that the param I referred to
isn't the one we need.
> > Index: subversion/libsvn_client/mergeinfo.c
> > ===================================================================
> > svn_boolean_t validate_inherited_mergeinfo = FALSE;
> > + /* ### JAF: Shouldn't we use a passed-in parameter instead? */
>
> I got rid of the needless local variable in r1145653 but I don't
> follow what you want re a passed-in parameter.
Thanks. I was probably a bit confused; ignore that.
> > Index: subversion/libsvn_client/mergeinfo.h
> > ===================================================================
> > - 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. */
>
> Committed this bit in r1145269.
Thanks.
> > Index: subversion/libsvn_ra_neon/options.c
> > ===================================================================
> > - 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);
>
> There are no guarantees that the hash key you use here, the CAPABILITY
> argument to svn_ra_neon__has_capability(), has the necessary lifetime.
> What if the caller of svn_ra_has_capability didn't pass a string
> literal? Unlikely I know, but possible.
OK. We could strdup it ... but never mind. Just looking, as always,
for simplifications.
- Julian
Received on 2011-07-15 19:00:16 CEST