On Fri, Jul 15, 2011 at 12:59 PM, Julian Foad <julian.f...@wandisco.com> wrote: > 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.
Hi Julian, Ugh, you're correct, I had it completely backwards. I suspect part of the reason this is so confounding is that get_invalid_inherited_mergeinfo() answers such an odd question, i.e. "What non-existent merge sources does a path inherit?". The sole caller has to remove the result from the target's inherited mergeinfo to come up with something meaningful. In r1148436 I replaced get_invalid_inherited_mergeinfo() with remove_non_existent_inherited_mergeinfo(), which instead asks "Remove non-existent merge sources from the input mergeinfo". That, to me anyway, makes a lot more sense. I also reworked all the comments (again). While nothing is radically different with the code, hopefully this will save some pain for the next person through this. Paul >> 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 > > >
