On Fri, Jul 15, 2011 at 12:59 PM, Julian Foad <julian.foad_at_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.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.
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
>
>
>
Received on 2011-07-19 18:34:07 CEST