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

On 06/20/2012 02:32 PM, Greg Stein wrote:
> On Wed, Jun 20, 2012 at 2:30 PM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
>> On 06/20/2012 02:24 PM, gstein_at_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 <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development