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.f...@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
