On Tue, Jun 28, 2011 at 12:46 PM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> I'm curious about the new 'validate_inherited_mergeinfo' parameter of
> svn_ra_get_mergeinfo2(), which is the public API that calls
> svn_ra__vtable_t.get_mergeinfo().
>
> /* ...
> * If the mergeinfo for any path is inherited and
> * @a *validate_inherited_mergeinfo is TRUE, then request that the server
> * validate the mergeinfo in @a *catalog, so it contains only merge source
> * path-revisions that actually exist in repository. If validation is
> * requested and the server supports it, then set
> * @a *validate_inherited_mergeinfo to TRUE on return. Set it to FALSE
> * in all other cases.
> * ... */
>
> This was introduced in r1035389, with the log message: "If the server is
> asked to validate inherited mergeinfo, enable it to communicate back to
> the client that it was actually able to do this."
>
> Exactly one caller passes '*validate_inherited_mergeinfo' as TRUE, which
> is get_invalid_inherited_mergeinfo() in libsvn_client. That function
> then returns the output value from that parameter through its own
> separate output parameter '*inherited_validated'.
>
> Hyrum noticed that having an in-out parameter behave like this is a bit
> ugly (all callers have to have a variable to pass the address of, even
> if they are not interested in it), and a bit of a trap for the unwary,
> although of course it is not terribly complex. But this made me wonder
> whether it's the best way to handle this kind of information.
Hi Julian,
I hadn't realized using in-out parameters was considered such bad
form. When I tweaked that API I thought we used in-out parms in other
APIs, but obviously I should have confirmed! (Maybe I had a private
API like svn_client__update_internal(svn_boolean_t *timestamp_sleep)
in mind).
If we need to change this, then your second alternative, splitting the
parameter into two, seems the more straightforward option. I'm happy
to make the change if that is what we want.
Paul
> There are two other APIs closely related to this one:
> svn_fs_get_mergeinfo2() and svn_repos_fs_get_mergeinfo2(). Each of
> those has a similar parameter except that in those cases the parameter
> is input-only.
>
> Question to the experts: Could this feedback of 'OK, it was validated'
> be handled instead by means of the "server capabilities" mechanism, and
> if so would that be better? I mean something like having a new
> capability meaning "server can validate inherited mergeinfo", and
> changing that in-out parameter of svn_ra_get_mergeinfo2() to an
> input-only parameter, and then the caller can assume its request for
> "validation" will be honoured if that server capability is present.
>
> Alternatively, shall we simply change the function prototype to have one
> input argument and one optional output argument, like this:
>
> /* ...
> * If the mergeinfo for any path is inherited and
> * @a validate_inherited_mergeinfo is TRUE, then request that the
> * server validate the mergeinfo in @a *catalog, so it contains
> * only merge source path-revisions that actually exist in the
> * repository.
> *
> * If @a inherited_mergeinfo_was_validated is non-NULL, then if
> * @a validate_inherited_mergeinfo is TRUE and the server did
> * in fact "validate" it, set @a *inherited_mergeinfo_was_validated
> * to TRUE, else to FALSE. */
> * ... */
> svn_boolean_t *inherited_mergeinfo_was_validated,
> svn_boolean_t validate_inherited_mergeinfo,
>
> ?
>
> - Julian
>
>
>
Received on 2011-06-28 19:38:01 CEST