"David Glasser" <glasser_at_davidglasser.net> writes:
>> Unrelatedly, make mod_dav transmit an svn_error_t correctly:
>>
>> * subversion/mod_dav_svn/reports/mergeinfo.c
>> (dav_svn__get_mergeinfo_report): Don't flush before returning error,
>> as flushing might obscure the error due to certain intricacies of
>> mod_dav's plumbing.
>
> It would be great if this got review from somebody else familiar with
> DAV... I'm unable to revision this myself, and it does seem like a
> subtle change.
Just FYI: I'm going to make a separate thread about whether we should
make this change across all of mod_dav_svn (I think there are good
arguments for doing so).
>> @@ -119,14 +119,18 @@
>> an authentication request, which is a command response whose arguments
>> match the prototype:
>>
>> - auth-request: ( ( mech:word ... ) realm:string )
>> + auth-request: ( ( mech:word ... ) realm:string ( cap:word ... ) )
>>
>> The mech values give a list of SASL mechanisms supported by the
>> server. The realm string is similar to an HTTP authentication realm
>> as defined in [RFC 2617]; it allows the server to indicate which of
>> -several protection spaces the server wishes to authenticate in. If
>> -the mechanism list is empty, then no authentication is required and no
>> -further action takes place as part of the authentication challenge;
>> +several protection spaces the server wishes to authenticate in. The
>> +cap values list the repository capabilities (that is, capabilities
>> +that require both the server and repository to support them before the
>> +server can claim them as capabilities, e.g., SVN_RA_SVN_CAP_MERGEINFO).
>> +
>> +If the mechanism list is empty, then no authentication is required and
>> +no further action takes place as part of the authentication challenge;
>> otherwise, the client responds with a tuple matching the prototype:
>
> You've changed the doc of the wrong part: I think you added the cap
> list to repos-info, not auth-request.
Oh, duh. Thank you. Will fix.
>> + SVN_ERR(svn_fs_revision_root(&root, repos->fs, 0, pool));
>> + APR_ARRAY_PUSH(paths, const char *) = "";
>> + err = svn_fs_get_mergeinfo(&ignored_mergeoutput, root,
>> + paths, FALSE, FALSE, pool);
>
> I really think this should ask the FS directly if it supports
> mergeinfo, instead of relying on error handling (which has a tendancy
> to leak memory, etc). Yeah, this requires adding Yet Another Layer of
> capabilities, but the two backends already have this function.
Is there a memory leak here? (I understand that this style makes it
easy to write a memory-leak bug, but I don't think there's one
actually here.)
I just hate to add a public API before we're certain we'll need it for
anything else. Could we wait until the second fs capability we need,
and do it then? Then at least we'd know fs caps aren't a one-off...
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-14 06:16:34 CET