Re: svn commit: r36782 - trunk/subversion/libsvn_ra_serf

On Thu, Mar 26, 2009 at 16:40, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> Greg Stein wrote:
>> On Wed, Mar 25, 2009 at 22:24, C. Michael Pilato <cmpilato_at_collab.net> wrote:
>>> ...
>>> +++ trunk/subversion/libsvn_ra_serf/merge.c     Wed Mar 25 14:24:32 2009        (r36782)
>>> @@ -287,40 +287,41 @@ end_merge(svn_ra_serf__xml_parser_t *par
>>>                          apr_hash_get(info->props,
>>>                                       "post-commit-err", APR_HASH_KEY_STRING));
>>>         }
>>> -      else if (ctx->session->wc_callbacks->push_wc_prop)
>>> +      else
>>>         {
>>> -          const char *href, *checked_in;
>>> -          svn_string_t checked_in_str;
>>> +          const char *href;
>>>
>>>           href = apr_hash_get(info->props, "href", APR_HASH_KEY_STRING);
>>> -          checked_in = apr_hash_get(info->props, "checked-in",
>>> -                                    APR_HASH_KEY_STRING);
>>> -
>>>           if (! svn_path_is_ancestor(ctx->merge_url, href))
>>>             {
>>>               /* ### need something better than APR_EGENERAL */
>>>               return svn_error_createf(APR_EGENERAL, NULL,
>>> -                                       _("A MERGE response for '%s' is not a child "
>>> -                                         "of the destination ('%s')"),
>>> +                                       _("A MERGE response for '%s' is not "
>>> +                                         "a child of the destination ('%s')"),
>>>                                        href, ctx->merge_url);
>>>             }
>
> Hi.  Remember me?

Ah. Missed that usage.

>>>           href = svn_path_is_child(ctx->merge_url, href, NULL);
>>>           if (! href) /* the paths are equal */
>>>             href = "";
>>>
>>> -          checked_in_str.data = checked_in;
>>> -          checked_in_str.len = strlen(checked_in);
>>> -
>>>           /* We now need to dive all the way into the WC to update the
>>>            * base VCC url.
>>>            */
>>> -          SVN_ERR(ctx->session->wc_callbacks->push_wc_prop(
>>> -                                       ctx->session->wc_callback_baton,
>>> -                                       href,
>>> -                                       SVN_RA_SERF__WC_CHECKED_IN_URL,
>>> -                                       &checked_in_str,
>>> -                                       info->pool));
>>> +          if ((! SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(ctx->session))
>>> +              && ctx->session->wc_callbacks->push_wc_prop)
>>> +            {
>>> +              svn_string_t checked_in_str;
>>> +              const char *checked_in;
>>>
>>> +              checked_in = apr_hash_get(info->props, "checked-in",
>>> +                                        APR_HASH_KEY_STRING);
>>> +              checked_in_str.data = checked_in;
>>> +              checked_in_str.len = strlen(checked_in);
>>> +
>>> +              SVN_ERR(ctx->session->wc_callbacks->push_wc_prop(
>>> +                ctx->session->wc_callback_baton, href,
>>> +                SVN_RA_SERF__WC_CHECKED_IN_URL, &checked_in_str, info->pool));
>>
>> href is only needed here, so you can tighten up its computation/scope.
>
> No, I expressly wish to always hit the sanity check above, which previously
> only "ran" if there was a push_wc_prop() callback function to use.  We've
> seen enough bugs over time in this area to make me want to always perform
> the sanity check.  When this error triggers, it has traditionally meant that
> a commit has gone to the *wrong* FS node ... like changes intended for
> /trunk winding up on some branch.  Users will want to know about this
> scenario, even if they aren't writing out wcprops.

Yup. Fair enough.

So the second bits of href usage can be shifted inside the block.

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1432872
Received on 2009-03-26 18:44:27 CET