[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: API review - svn_ra_get_mergeinfo2()

From: Paul Burba <ptburba_at_gmail.com>
Date: Tue, 19 Jul 2011 12:33:31 -0400

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

This is an archived mail posted to the Subversion Dev mailing list.