Re: svn commit: r1352252 - /subversion/trunk/subversion/libsvn_ra_serf/update.c

[C. Michael Pilato]

On 06/20/2012 02:32 PM, Greg Stein wrote:
> On Wed, Jun 20, 2012 at 2:30 PM, C. Michael Pilato <cmpil...@collab.net> 
> wrote:
>> On 06/20/2012 02:24 PM, gst...@apache.org wrote:
>>> +  /* If we are talking to an old server, then the sha1-checksum property
>>> +     will not exist. In our property parsing code, we don't bother to
>>> +     check the <status> element which would indicate a 404. That section
>>> +     needs to name the 404'd property, so our parsing code only sees:
>>> +
>>> +       <g0:sha1-checksum/>
>>> +
>>> +     That is parsed as an empty string value for the property.
>>> +
>>> +     When checking the property, if it is missing (NULL), or the above
>>> +     empty string, then we know the property doesn't exist.
>>> +
>>> +     Strictly speaking, we could start parsing <status> and omit any
>>> +     properties that were 404'd. But the 404 only happens when we ask
>>> +     for a specific property, and it is easier to just check for the
>>> +     empty string. Since it isn't a legal value in this case, we won't
>>> +     get confused with a truly existent property.  */
>>
>> I remember writing ra_neon's PROPFIND parsing logic to parse the
>> per-property status field.  Surely it can't be *that hard* to do the right
>> thing here, and stop conflating the empty string with a NULL one.
> 
> 404 can only happen for properties we specifically ask for. They are
> always there.
> 
> Except for this one, on old servers.
> 
> I'm not going to write a hundred lines of code when an empty string
> test works just fine.

Can you at least then give a quick review on this approach for a more proper
fix?  I've not tested it ... just trying to see if I can grok your new
magical XML handling stuff (which is pretty sweet, by the way!)


Index: subversion/libsvn_ra_serf/property.c
===================================================================
--- subversion/libsvn_ra_serf/property.c        (revision 1352206)
+++ subversion/libsvn_ra_serf/property.c        (working copy)
@@ -193,6 +193,10 @@
     {
       svn_ra_serf__xml_note(xes, PROPVAL, "altvalue", cdata->data);
     }
+  else if (leaving_state == PROPSTAT)
+    {
+      svn_ra_serf__xml_note(xes, PROPVAL, "propstat", cdata->data);
+    }
   else
     {
       const char *encoding = apr_hash_get(attrs, "V:encoding",
@@ -202,10 +206,22 @@
       const char *path;
       const char *ns;
       const char *name;
+      const char *propstat;
       const char *altvalue;

       SVN_ERR_ASSERT(leaving_state == PROPVAL);

+      /* Check the status of the property.  If it isn't 200, it isn't
+         interesting to us. */
+      propstat = apr_hash_get(attrs, "propstat", APR_HASH_KEY_STRING);
+      if (propstat)
+        {
+          apr_int64_t status;
+          SVN_ERR(svn_cstring_atoi64(&status, propstat));
+          if (status != 200)
+            return SVN_NO_ERROR;
+        }
+
       if (encoding)
         {
           if (strcmp(encoding, "base64") != 0)

-- 
C. Michael Pilato <cmpil...@collab.net>
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development

signature.asc
Description: OpenPGP digital signature