Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV) Version 2

On 01/13/2010 05:07 PM, Kamesh Jayachandran wrote:
> On 01/13/2010 12:08 AM, C. Michael Pilato wrote:
>> Kamesh Jayachandran wrote:
>>> Hi All,
>>>
>>> Last week I posted the patch to implement 'svn client' to identify the
>>> svn operation they are about to do with a given ra_session.
>>>
>>> Following thread has the detailed discussion.
>>>
>>> http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/%3C4B448959.1070400@collab.net%3E
>>>
>>>
>>>
>>>
>>> Below is the summary:
>>>
>>> Concern/Suggestion 1:
>>> Michael Pilato and Philip Martin were suggesting to tweak
>>> libsvn_ra_(serf|neon) to detect the operation rather than making a
>>> change across all layers from the simplicity point of view.
>>>
>>> My answer to 1:
>>> I feel it would be too hackish to tweak one general API inside these
>>> modules to flag 'commit or write' operation to the server when the
>>> solution I propose handles this in a transparent way.
>> I'm sorry, but did you say "transparent"? What's transparent about
>> bubbling
>> an RA-layer hack all the way up into the client?! A "transparent"
>> solution
>
> This patch is *not* an RA layer hack rather a transparent generic
> feature so I see nothing wrong in propagating it to higher layers.
>
>> is one that preserves the existing transparency of the mirroring
>> subsystem.
>
> I do *not* like to expose the back-end internals to higher layers
> especially when it is not so common setup and not normally supposed to
> know(I mean why my client should know the repository it commits to is
> a mirror directly or indirectly).
>
>> A "transparent" solution is one that doesn't allow ignorant
>> third-party
>> consumers of the Subversion APIs to accidentally break their proxy
>> setups
>> because they decide they wanted to pass "checkin" instead of "commit"
>> as the
>> innocuous-appearing 'high_level_svn_operation' parameter.
>
> Yes I agree. I was concerned about the *magical flag deep inside the
> code* for only one operation, now it looks like the only way to go.
>
>
>
>> There *must* be a better way to do this.
>>
>> subversion/libsvn_ra_neon/commit.c:apply_revprops() has this comment:
>>
>> /* ### we should use DAV:apply-to-version on the CHECKOUT so we
>> can skip
>> ### retrieval of the baseline */
>>
>> I looked a little bit into this. IIUC, we can theoretically avoid the
>> problematic PROPFIND altogether by passing this special (yet
>> part-of-an-existing-standard) flag in the CHECKOUT request. Currently,
>> though, mod_dav_svn doesn't handle the flag. So there's still a
>> server-side
>> change needed, but at least its one that would take us closer to better
>> WebDAV handling. Maybe you could explore this option instead?
>>
>
> I will take a fresh look at this problem and a independent patch in
> this way.
>
> Thanks
> With regards
> Kamesh Jayachandran

Attached patch just fixes this at ra_neon layer alone(ra_serf do not
need the fix at least on trunk, as it do not use the PROPFIND for this
rather uses POST which do not suffer from this issue as it simply
proxies to MASTER).

Have not tested the full test suite, I hope this is final unless I learn
something new.

With regards
Kamesh Jayachandran