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

Re: svn commit: r1335639 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c

From: Ivan Zhakov <ivan_at_visualsvn.com>
Date: Thu, 10 May 2012 14:21:25 +0400

On Wed, May 9, 2012 at 8:51 PM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> On 05/08/2012 05:47 PM, Ivan Zhakov wrote:
>> On Wed, May 9, 2012 at 1:34 AM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>>> On Wed, May 9, 2012 at 12:49 AM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
>>>> On 05/08/2012 04:39 PM, Mark Phippard wrote:
>>>>> On Tue, May 8, 2012 at 4:20 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>>>>>> On Wed, May 9, 2012 at 12:09 AM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
>>>>>>> On 05/08/2012 03:35 PM, Greg Stein wrote:
>>>>>>>> One question: the ordering of PROPFIND and GET. Do you know if that is
>>>>>>>> a requirement, or simply that you were preserving prior behavior?
>>>>>>>
>>>>>>> Upon reflection, it's probably not a hard requirement.  In general, I
>>>>>>> suppose it's easier (and more efficient) to cache properties and stream
>>>>>>> contents while we drive an editor than the reverse, so that's probably why
>>>>>>> that ordering was chosen prior.
>>>>>>>
>>>>>> Another option is to include properties in REPORT even in skelta-mode
>>>>>> if they are small. With defining small something like 0.5-1k.
>>>>>
>>>>> Agreed.  I had forgotten that we would still need these roundtrips to
>>>>> get the properties.  Maybe the REPORT request could at least indicate
>>>>> which items have properties.  It would be better for performance if
>>>>> things like svn:eol-style and svn:mimetype were included in the
>>>>> request so we then only had to go back to the server for custom props
>>>>> (and we knew which files have them).
>>>>
>>>> The REPORT request does include a <fetch-props/> type of indicator which
>>>> says "there's something worth fetching here".
>>>>
>>>> I'm quite in favor of including, say, the "svn:" class of properties in the
>>>> REPORT response proper.
>>>>
>>> We could include all properties if we know there are small. It seems
>>> to be possible implement this on server side, but I'm not sure that
>>> current client code is ready for mixing embedded and external
>>> properties in REPORT response.
>>>
>> Well, it seems things are more complicated: current mod_dav_svn
>> implementation never sends <fetch-props /> tag and ra_serf always asks
>> for properties, even if there is no properties.
>
> I double-checked this claim, Ivan, and don't believe it to be accurate.  I
> started with code inspection, and found things to be as I'd hoped.
>
> I then created an empty repository, and made these changes:
>
>   r1:  add a file, with properties set on it.
>   r2:  modified the file's contents only
>   r3:  modified the file's svn:eol-style property value (which also
>        tweaked its content)
>   r4;  delete the file's svn:eol-style property.
>
> Then, I backdated my working copy to r0, and started updating to each
> successive revision.
>
> $ svn up -r1:
>
>   "OPTIONS /tests/repos HTTP/1.1"
>   "OPTIONS /tests/repos HTTP/1.1"
>   "REPORT /tests/repos/!svn/me HTTP/1.1"
>   "PROPFIND /tests/repos/!svn/rvr/1/file.py HTTP/1.1"
>   "HEAD /tests/repos/!svn/rvr/1/file.py HTTP/1.1"
>
> This update adds the new file back to the working copy.  I get a PROPFIND to
> fetch its properties (the server never transmits props for added files/dirs)
> and a HEAD for the contents (because they are cached in the WC pristine
> store already ... else this would have been a GET).
>
> $ svn up -r2:
>
>   "OPTIONS /tests/repos HTTP/1.1"
>   "OPTIONS /tests/repos HTTP/1.1"
>   "REPORT /tests/repos/!svn/me HTTP/1.1"
>   "HEAD /tests/repos/!svn/rvr/2/file.py HTTP/1.1"
>
> No PROPFIND.  HEAD to install the new contents.
>
> $ svn up -r3:
>
>   "OPTIONS /tests/repos HTTP/1.1"
>   "OPTIONS /tests/repos HTTP/1.1"
>   "REPORT /tests/repos/!svn/me HTTP/1.1"
>   "HEAD /tests/repos/!svn/rvr/3/file.py HTTP/1.1"
>
> Again no PROPFIND, because the <D:set-prop> REPORT response item carried the
> svn:eol-style propchange.  HEAD because the eol-style changed and new
> contents were required.
>
> $ svn up -r4:
>
>   "OPTIONS /tests/repos HTTP/1.1"
>   "OPTIONS /tests/repos HTTP/1.1"
>   "REPORT /tests/repos/!svn/me HTTP/1.1"
>
> Again, no PROPFIND, because the <D:remove-prop> REPORT response item carried
> the propchange info.  No HEAD because the contents didn't change.
>
> So, it's only the added files and directories that seem to necessitate a
> PROPFIND.
>
Hi Mike,

I've only checked that mod_dav_svn never sends <fetch-props /> tags
while you mentioned that it does. Actually I checked it only by
analyzing the server-side code, not by capturing real transfer. Sorry
for confusion.

It seems that ra_serf unconditionally retrieves properties using
PROPFIND for *all* added files:
subversion\libsvn_ra_serf\update.c:1633 (start_report)
[[[
  else if ((state == OPEN_DIR || state == ADD_DIR) &&
           strcmp(name.name, "add-file") == 0)
    {
      const char *file_name, *cf, *cr;
      report_info_t *info;

      file_name = svn_xml_get_attr_value("name", attrs);
      cf = svn_xml_get_attr_value("copyfrom-path", attrs);
      cr = svn_xml_get_attr_value("copyfrom-rev", attrs);

      if (!file_name)
        {
          return svn_error_create(
            SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
            _("Missing name attr in add-file element"));
        }

      info = push_state(parser, ctx, ADD_FILE);

      info->base_rev = SVN_INVALID_REVNUM;
      info->fetch_props = TRUE;
      info->fetch_file = TRUE;
]]]

Do you know why?

-- 
Ivan Zhakov
Received on 2012-05-10 12:22:19 CEST

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