You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by iv...@apache.org on 2015/09/24 15:59:17 UTC
svn commit: r1705060 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h serf.c util.c
Author: ivan
Date: Thu Sep 24 13:59:16 2015
New Revision: 1705060
URL: http://svn.apache.org/viewvc?rev=1705060&view=rev
Log:
Refactor common code in ra_serf.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__uri_parse): New function declaration.
* subversion/libsvn_ra_serf/util.c
(svn_ra_serf__uri_parse): New. Factored out from svn_ra_serf__open().
* subversion/libsvn_ra_serf/serf.c
(svn_ra_serf__open, svn_ra_serf__reparent): Use svn_ra_serf__uri_parse().
Modified:
subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
subversion/trunk/subversion/libsvn_ra_serf/serf.c
subversion/trunk/subversion/libsvn_ra_serf/util.c
Modified: subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h?rev=1705060&r1=1705059&r2=1705060&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h Thu Sep 24 13:59:16 2015
@@ -1548,6 +1548,17 @@ svn_ra_serf__create_bucket_with_eagain(c
apr_size_t len,
serf_bucket_alloc_t *allocator);
+/* Parse a given URL_STR, fill in all supplied fields of URI
+ * structure.
+ *
+ * This function is a compatibility wrapper around apr_uri_parse().
+ * Different apr-util versions set apr_uri_t.path to either NULL or ""
+ * for root paths, and serf expects to see "/". This function always
+ * sets URI.path to "/" for these paths. */
+svn_error_t *
+svn_ra_serf__uri_parse(apr_uri_t *uri,
+ const char *url_str,
+ apr_pool_t *pool);
#if defined(SVN_DEBUG)
Modified: subversion/trunk/subversion/libsvn_ra_serf/serf.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/serf.c?rev=1705060&r1=1705059&r2=1705060&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/serf.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/serf.c Thu Sep 24 13:59:16 2015
@@ -513,19 +513,8 @@ svn_ra_serf__open(svn_ra_session_t *sess
serf_sess->pool));
- status = apr_uri_parse(serf_sess->pool, session_URL, &url);
- if (status)
- {
- return svn_error_createf(SVN_ERR_RA_ILLEGAL_URL, NULL,
- _("Illegal URL '%s'"),
- session_URL);
- }
- /* Depending the version of apr-util in use, for root paths url.path
- will be NULL or "", where serf requires "/". */
- if (url.path == NULL || url.path[0] == '\0')
- {
- url.path = apr_pstrdup(serf_sess->pool, "/");
- }
+ SVN_ERR(svn_ra_serf__uri_parse(&url, session_URL, serf_sess->pool));
+
if (!url.port)
{
url.port = apr_uri_port_of_scheme(url.scheme);
@@ -827,25 +816,11 @@ svn_ra_serf__reparent(svn_ra_session_t *
"URL '%s'"), url, session->repos_root_str);
}
- status = apr_uri_parse(pool, url, &new_url);
- if (status)
- {
- return svn_error_createf(SVN_ERR_RA_ILLEGAL_URL, NULL,
- _("Illegal repository URL '%s'"), url);
- }
+ SVN_ERR(svn_ra_serf__uri_parse(&new_url, url, pool));
- /* Depending the version of apr-util in use, for root paths url.path
- will be NULL or "", where serf requires "/". */
/* ### Maybe we should use a string buffer for these strings so we
### don't allocate memory in the session on every reparent? */
- if (new_url.path == NULL || new_url.path[0] == '\0')
- {
- session->session_url.path = apr_pstrdup(session->pool, "/");
- }
- else
- {
- session->session_url.path = apr_pstrdup(session->pool, new_url.path);
- }
+ session->session_url.path = apr_pstrdup(session->pool, new_url.path);
session->session_url_str = apr_pstrdup(session->pool, url);
return SVN_NO_ERROR;
Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1705060&r1=1705059&r2=1705060&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/util.c Thu Sep 24 13:59:16 2015
@@ -1951,3 +1951,29 @@ svn_ra_serf__create_handler(svn_ra_serf_
return handler;
}
+svn_error_t *
+svn_ra_serf__uri_parse(apr_uri_t *uri,
+ const char *url_str,
+ apr_pool_t *pool)
+{
+ apr_status_t status;
+
+ status = apr_uri_parse(pool, url_str, uri);
+ if (status)
+ {
+ /* Do not use returned error status in error message because currently
+ apr_uri_parse() returns APR_EGENERAL for all parsing errors. */
+ return svn_error_createf(SVN_ERR_RA_ILLEGAL_URL, NULL,
+ _("Illegal URL '%s'"),
+ url_str);
+ }
+
+ /* Depending the version of apr-util in use, for root paths uri.path
+ will be NULL or "", where serf requires "/". */
+ if (uri->path == NULL || uri->path[0] == '\0')
+ {
+ uri->path = apr_pstrdup(pool, "/");
+ }
+
+ return SVN_NO_ERROR;
+}
Re: svn commit: r1705060 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h serf.c util.c
Posted by Julian Foad <ju...@btopenworld.com>.
Ivan Zhakov wrote:
> As far I remember this thread about two pools paradigm itself
> (result_pool and scratch_pool). It's not about making all functions
> follow two pools paradigm.
Correct.
> Some of API introduced in 1.9 doesn't
> follow this paradigm for some reason.
Yes, I see that.
> Please understand me correctly:
> I really like two pool paradigm, but sometimes it's not necessary,
> especially for local helpers.
OK, you have partially convinced me. For a simple local function, we
might not want to take two pools when that seems unnecessarily
verbose. If this function takes one pool, allocates its result in that
pool, and perhaps allocates a small amount of temporary data as well,
I accept your argument that we don't necessarily want to name the pool
'result_pool' because that could be confusing in places where its
implementation is storing scratch data. In cases like this I agree we
may want to use the name 'pool'. And in other cases there is
flexibility too. We don't enforce a rigid rule.
- Julian
Re: svn commit: r1705060 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 24 September 2015 at 19:15, Julian Foad <ju...@btopenworld.com> wrote:
> Ivan Zhakov wrote:
>> Julian Foad wrote:
>>> Ivan Zhakov wrote:
>>>> [...] we also have many functions that accepts just POOL and use
>>>> it as scratch pool. And we also have many functions that uses it as
>>>> result pool.
>>>
>>> Yes, we do have many of those. That was the Old Way. Naming the pools
>>> 'scratch_pool' or 'result_pool' is the New Way. We seem to generally
>>> agree that is better, and sometimes we rename the single 'pool'
>>> argument of old functions to either 'scratch_pool' or 'result_pool'.
>>
>> Could you please give me link to the thread where we discussed The New
>> Way? Yes, we use result_pool/scratch_pool, but I don't remember
>> discussion about never using just one POOL.
>
> I don't know if this was ever explicitly discussed. (The thread where
> the two-pools paradigm was first publicly is: Subject "result_pool and
> scratch_pool", from Greg Stein, on 2008-10-06, archived at e.g.
> http://svn.haxx.se/dev/archive-2008-10/0268.shtml )
>
As far I remember this thread about two pools paradigm itself
(result_pool and scratch_pool). It's not about making all functions
follow two pools paradigm. Some of API introduced in 1.9 doesn't
follow this paradigm for some reason. Please understand me correctly:
I really like two pool paradigm, but sometimes it's not necessary,
especially for local helpers.
--
Ivan Zhakov
Re: svn commit: r1705060 - in
/subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h serf.c util.c
Posted by Branko Čibej <br...@apache.org>.
On 24.09.2015 19:15, Julian Foad wrote:
> Ivan Zhakov wrote:
>> Julian Foad wrote:
>>> Ivan Zhakov wrote:
>>>> [...] we also have many functions that accepts just POOL and use
>>>> it as scratch pool. And we also have many functions that uses it as
>>>> result pool.
>>> Yes, we do have many of those. That was the Old Way. Naming the pools
>>> 'scratch_pool' or 'result_pool' is the New Way. We seem to generally
>>> agree that is better, and sometimes we rename the single 'pool'
>>> argument of old functions to either 'scratch_pool' or 'result_pool'.
>> Could you please give me link to the thread where we discussed The New
>> Way? Yes, we use result_pool/scratch_pool, but I don't remember
>> discussion about never using just one POOL.
> I don't know if this was ever explicitly discussed. (The thread where
> the two-pools paradigm was first publicly is: Subject "result_pool and
> scratch_pool", from Greg Stein, on 2008-10-06, archived at e.g.
> http://svn.haxx.se/dev/archive-2008-10/0268.shtml )
>
>> One problem with single pool argument named result_pool is performing
>> temporary allocations. I find temporary allocations in result_pool
>> slightly confusing.
> I agree with that. That is a good point. Maybe the argument it is not
> so clear. I have sometimes found that situation, and I have wondered
> if the best thing is to declare "apr_pool_t *scratch_pool =
> result_pool;" inside the function, but I don't often do that.
Only old functions that have a single apr_pool_t parameter called 'pool'
should be using it as both a scratch and result pool. Any new functions
that both kinds of allocations should take two pool parameters, and it's
up to the caller to select which pools to pass.
Temporary local scratch pools should be very rare indeed; most of the
uses I've seen (outside of bindings) are in compatibility wrappers.
We've not been entirely consistent in following these "rules" but
there's a really thin line between "entriely consistent" and "stubborn
beancounter". :)
-- Brane
-- Brane
Re: svn commit: r1705060 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h serf.c util.c
Posted by Julian Foad <ju...@btopenworld.com>.
Ivan Zhakov wrote:
> Julian Foad wrote:
>> Ivan Zhakov wrote:
>>> [...] we also have many functions that accepts just POOL and use
>>> it as scratch pool. And we also have many functions that uses it as
>>> result pool.
>>
>> Yes, we do have many of those. That was the Old Way. Naming the pools
>> 'scratch_pool' or 'result_pool' is the New Way. We seem to generally
>> agree that is better, and sometimes we rename the single 'pool'
>> argument of old functions to either 'scratch_pool' or 'result_pool'.
>
> Could you please give me link to the thread where we discussed The New
> Way? Yes, we use result_pool/scratch_pool, but I don't remember
> discussion about never using just one POOL.
I don't know if this was ever explicitly discussed. (The thread where
the two-pools paradigm was first publicly is: Subject "result_pool and
scratch_pool", from Greg Stein, on 2008-10-06, archived at e.g.
http://svn.haxx.se/dev/archive-2008-10/0268.shtml )
> One problem with single pool argument named result_pool is performing
> temporary allocations. I find temporary allocations in result_pool
> slightly confusing.
I agree with that. That is a good point. Maybe the argument it is not
so clear. I have sometimes found that situation, and I have wondered
if the best thing is to declare "apr_pool_t *scratch_pool =
result_pool;" inside the function, but I don't often do that.
- Julian
Re: svn commit: r1705060 - in
/subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h serf.c util.c
Posted by Branko Čibej <br...@apache.org>.
On 24.09.2015 18:56, Branko Čibej wrote:
> On 24.09.2015 18:41, Ivan Zhakov wrote:
>> On 24 September 2015 at 19:29, Julian Foad <ju...@btopenworld.com> wrote:
>>> Ivan Zhakov wrote:
>>>> On 24 September 2015 at 18:50, Stefan Sperling <st...@elego.de> wrote:
>>>>> On Thu, Sep 24, 2015 at 05:40:45PM +0300, Ivan Zhakov wrote:
>>>>>> I think we use POOL name if function accepts just one pool, and
>>>>>> SCRATCH_POOL/RESULT_POOL in other case. Is not it?
>>>>>>
>>>>>> I would not mind to rename POOL to RESULT_POOL in this particular
>>>>>> case, but I'm not sure that we should use RESULT_POOL in all other
>>>>>> cases if function accepts one pool.
>>>>> We certainly have functions that take only a scratch_pool.
>>>>> The idea is to identify the purpose of the pool, and not only
>>>>> in the case where there are 2 pools.
>>>> I don't think we may use other places with only scratch_pool argument
>>>> as reason:
>>> Perhaps there was a slight misunderstanding there. When you wrote "I'm
>>> not sure that we should use RESULT_POOL in all other cases if function
>>> accepts one pool", perhaps Stefan thought you meant all other cases
>>> where a function accepts one pool, regardless of the purpose of that
>>> pool, and he wanted to refute that suggestion. (I wondered if you
>>> meant that.) But if you meant all other cases where a function takes
>>> one pool and that pool is used for results, then I'd say yes, we
>>> should rename them ... eventually.
>>>
>>>> we also have many functions that accepts just POOL and use
>>>> it as scratch pool. And we also have many functions that uses it as
>>>> result pool.
>>> Yes, we do have many of those. That was the Old Way. Naming the pools
>>> 'scratch_pool' or 'result_pool' is the New Way. We seem to generally
>>> agree that is better, and sometimes we rename the single 'pool'
>>> argument of old functions to either 'scratch_pool' or 'result_pool'.
>>>
>> Could you please give me link to the thread where we discussed The New
>> Way? Yes, we use result_pool/scratch_pool, but I don't remember
>> discussion about never using just one POOL.
> There hasn't been just "a thread". This discussion started way back when
> the design of WC-NG started. And Julian did not say "always use two
> pools". See above; he says "'sratch_pool' or 'result_pool'" twice, not
> the *or*.
I mean "note the *or*" of course.
-- Brane
Re: svn commit: r1705060 - in
/subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h serf.c util.c
Posted by Branko Čibej <br...@apache.org>.
On 24.09.2015 18:41, Ivan Zhakov wrote:
> On 24 September 2015 at 19:29, Julian Foad <ju...@btopenworld.com> wrote:
>> Ivan Zhakov wrote:
>>> On 24 September 2015 at 18:50, Stefan Sperling <st...@elego.de> wrote:
>>>> On Thu, Sep 24, 2015 at 05:40:45PM +0300, Ivan Zhakov wrote:
>>>>> I think we use POOL name if function accepts just one pool, and
>>>>> SCRATCH_POOL/RESULT_POOL in other case. Is not it?
>>>>>
>>>>> I would not mind to rename POOL to RESULT_POOL in this particular
>>>>> case, but I'm not sure that we should use RESULT_POOL in all other
>>>>> cases if function accepts one pool.
>>>> We certainly have functions that take only a scratch_pool.
>>>> The idea is to identify the purpose of the pool, and not only
>>>> in the case where there are 2 pools.
>>> I don't think we may use other places with only scratch_pool argument
>>> as reason:
>> Perhaps there was a slight misunderstanding there. When you wrote "I'm
>> not sure that we should use RESULT_POOL in all other cases if function
>> accepts one pool", perhaps Stefan thought you meant all other cases
>> where a function accepts one pool, regardless of the purpose of that
>> pool, and he wanted to refute that suggestion. (I wondered if you
>> meant that.) But if you meant all other cases where a function takes
>> one pool and that pool is used for results, then I'd say yes, we
>> should rename them ... eventually.
>>
>>> we also have many functions that accepts just POOL and use
>>> it as scratch pool. And we also have many functions that uses it as
>>> result pool.
>> Yes, we do have many of those. That was the Old Way. Naming the pools
>> 'scratch_pool' or 'result_pool' is the New Way. We seem to generally
>> agree that is better, and sometimes we rename the single 'pool'
>> argument of old functions to either 'scratch_pool' or 'result_pool'.
>>
> Could you please give me link to the thread where we discussed The New
> Way? Yes, we use result_pool/scratch_pool, but I don't remember
> discussion about never using just one POOL.
There hasn't been just "a thread". This discussion started way back when
the design of WC-NG started. And Julian did not say "always use two
pools". See above; he says "'sratch_pool' or 'result_pool'" twice, not
the *or*.
-- Brane
Re: svn commit: r1705060 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 24 September 2015 at 19:29, Julian Foad <ju...@btopenworld.com> wrote:
> Ivan Zhakov wrote:
>> On 24 September 2015 at 18:50, Stefan Sperling <st...@elego.de> wrote:
>>> On Thu, Sep 24, 2015 at 05:40:45PM +0300, Ivan Zhakov wrote:
>>>> I think we use POOL name if function accepts just one pool, and
>>>> SCRATCH_POOL/RESULT_POOL in other case. Is not it?
>>>>
>>>> I would not mind to rename POOL to RESULT_POOL in this particular
>>>> case, but I'm not sure that we should use RESULT_POOL in all other
>>>> cases if function accepts one pool.
>>>
>>> We certainly have functions that take only a scratch_pool.
>>> The idea is to identify the purpose of the pool, and not only
>>> in the case where there are 2 pools.
>>
>> I don't think we may use other places with only scratch_pool argument
>> as reason:
>
> Perhaps there was a slight misunderstanding there. When you wrote "I'm
> not sure that we should use RESULT_POOL in all other cases if function
> accepts one pool", perhaps Stefan thought you meant all other cases
> where a function accepts one pool, regardless of the purpose of that
> pool, and he wanted to refute that suggestion. (I wondered if you
> meant that.) But if you meant all other cases where a function takes
> one pool and that pool is used for results, then I'd say yes, we
> should rename them ... eventually.
>
>> we also have many functions that accepts just POOL and use
>> it as scratch pool. And we also have many functions that uses it as
>> result pool.
>
> Yes, we do have many of those. That was the Old Way. Naming the pools
> 'scratch_pool' or 'result_pool' is the New Way. We seem to generally
> agree that is better, and sometimes we rename the single 'pool'
> argument of old functions to either 'scratch_pool' or 'result_pool'.
>
Could you please give me link to the thread where we discussed The New
Way? Yes, we use result_pool/scratch_pool, but I don't remember
discussion about never using just one POOL.
One problem with single pool argument named result_pool is performing
temporary allocations. I find temporary allocations in result_pool
slightly confusing.
--
Ivan Zhakov
Re: svn commit: r1705060 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h serf.c util.c
Posted by Julian Foad <ju...@btopenworld.com>.
Ivan Zhakov wrote:
> On 24 September 2015 at 18:50, Stefan Sperling <st...@elego.de> wrote:
>> On Thu, Sep 24, 2015 at 05:40:45PM +0300, Ivan Zhakov wrote:
>>> I think we use POOL name if function accepts just one pool, and
>>> SCRATCH_POOL/RESULT_POOL in other case. Is not it?
>>>
>>> I would not mind to rename POOL to RESULT_POOL in this particular
>>> case, but I'm not sure that we should use RESULT_POOL in all other
>>> cases if function accepts one pool.
>>
>> We certainly have functions that take only a scratch_pool.
>> The idea is to identify the purpose of the pool, and not only
>> in the case where there are 2 pools.
>
> I don't think we may use other places with only scratch_pool argument
> as reason:
Perhaps there was a slight misunderstanding there. When you wrote "I'm
not sure that we should use RESULT_POOL in all other cases if function
accepts one pool", perhaps Stefan thought you meant all other cases
where a function accepts one pool, regardless of the purpose of that
pool, and he wanted to refute that suggestion. (I wondered if you
meant that.) But if you meant all other cases where a function takes
one pool and that pool is used for results, then I'd say yes, we
should rename them ... eventually.
> we also have many functions that accepts just POOL and use
> it as scratch pool. And we also have many functions that uses it as
> result pool.
Yes, we do have many of those. That was the Old Way. Naming the pools
'scratch_pool' or 'result_pool' is the New Way. We seem to generally
agree that is better, and sometimes we rename the single 'pool'
argument of old functions to either 'scratch_pool' or 'result_pool'.
> Anyway I agree that in this particular place RESULT_POOL makes more
> sense so I renamed argument in r1705088.
Thank you.
- Julian
Re: svn commit: r1705060 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 24 September 2015 at 18:50, Stefan Sperling <st...@elego.de> wrote:
> On Thu, Sep 24, 2015 at 05:40:45PM +0300, Ivan Zhakov wrote:
>> On 24 September 2015 at 17:34, Bert Huijben <be...@qqmail.nl> wrote:
>> >>
>> >> +/* Parse a given URL_STR, fill in all supplied fields of URI
>> >> + * structure.
>> >> + *
>> >> + * This function is a compatibility wrapper around apr_uri_parse().
>> >> + * Different apr-util versions set apr_uri_t.path to either NULL or ""
>> >> + * for root paths, and serf expects to see "/". This function always
>> >> + * sets URI.path to "/" for these paths. */
>> >> +svn_error_t *
>> >> +svn_ra_serf__uri_parse(apr_uri_t *uri,
>> >> + const char *url_str,
>> >> + apr_pool_t *pool);
>> >
>> > I think the pool should be named result_pool here.
>> >
>
> +1
>
>> I think we use POOL name if function accepts just one pool, and
>> SCRATCH_POOL/RESULT_POOL in other case. Is not it?
>>
>> I would not mind to rename POOL to RESULT_POOL in this particular
>> case, but I'm not sure that we should use RESULT_POOL in all other
>> cases if function accepts one pool.
>
> We certainly have functions that take only a scratch_pool.
> The idea is to identify the purpose of the pool, and not only
> in the case where there are 2 pools.
I don't think we may use other places with only scratch_pool argument
as reason: we also have many functions that accepts just POOL and use
it as scratch pool. And we also have many functions that uses it as
result pool.
Anyway I agree that in this particular place RESULT_POOL makes more
sense so I renamed argument in r1705088.
--
Ivan Zhakov
Re: svn commit: r1705060 - in
/subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h serf.c util.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Sep 24, 2015 at 05:40:45PM +0300, Ivan Zhakov wrote:
> On 24 September 2015 at 17:34, Bert Huijben <be...@qqmail.nl> wrote:
> >>
> >> +/* Parse a given URL_STR, fill in all supplied fields of URI
> >> + * structure.
> >> + *
> >> + * This function is a compatibility wrapper around apr_uri_parse().
> >> + * Different apr-util versions set apr_uri_t.path to either NULL or ""
> >> + * for root paths, and serf expects to see "/". This function always
> >> + * sets URI.path to "/" for these paths. */
> >> +svn_error_t *
> >> +svn_ra_serf__uri_parse(apr_uri_t *uri,
> >> + const char *url_str,
> >> + apr_pool_t *pool);
> >
> > I think the pool should be named result_pool here.
> >
+1
> I think we use POOL name if function accepts just one pool, and
> SCRATCH_POOL/RESULT_POOL in other case. Is not it?
>
> I would not mind to rename POOL to RESULT_POOL in this particular
> case, but I'm not sure that we should use RESULT_POOL in all other
> cases if function accepts one pool.
We certainly have functions that take only a scratch_pool.
The idea is to identify the purpose of the pool, and not only
in the case where there are 2 pools.
Re: svn commit: r1705060 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h serf.c util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 24 September 2015 at 17:34, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: ivan@apache.org [mailto:ivan@apache.org]
>> Sent: donderdag 24 september 2015 15:59
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1705060 - in
>> /subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h serf.c util.c
>>
>> Author: ivan
>> Date: Thu Sep 24 13:59:16 2015
>> New Revision: 1705060
>>
>> URL: http://svn.apache.org/viewvc?rev=1705060&view=rev
>> Log:
>> Refactor common code in ra_serf.
>>
>> * subversion/libsvn_ra_serf/ra_serf.h
>> (svn_ra_serf__uri_parse): New function declaration.
>>
>> * subversion/libsvn_ra_serf/util.c
>> (svn_ra_serf__uri_parse): New. Factored out from svn_ra_serf__open().
>>
>> * subversion/libsvn_ra_serf/serf.c
>> (svn_ra_serf__open, svn_ra_serf__reparent): Use
>> svn_ra_serf__uri_parse().
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
>> subversion/trunk/subversion/libsvn_ra_serf/serf.c
>> subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>> Modified: subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/
>> ra_serf.h?rev=1705060&r1=1705059&r2=1705060&view=diff
>> ==========================================================
>> ====================
>> --- subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h (original)
>> +++ subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h Thu Sep 24
>> 13:59:16 2015
>> @@ -1548,6 +1548,17 @@ svn_ra_serf__create_bucket_with_eagain(c
>> apr_size_t len,
>> serf_bucket_alloc_t *allocator);
>>
>> +/* Parse a given URL_STR, fill in all supplied fields of URI
>> + * structure.
>> + *
>> + * This function is a compatibility wrapper around apr_uri_parse().
>> + * Different apr-util versions set apr_uri_t.path to either NULL or ""
>> + * for root paths, and serf expects to see "/". This function always
>> + * sets URI.path to "/" for these paths. */
>> +svn_error_t *
>> +svn_ra_serf__uri_parse(apr_uri_t *uri,
>> + const char *url_str,
>> + apr_pool_t *pool);
>
> I think the pool should be named result_pool here.
>
I think we use POOL name if function accepts just one pool, and
SCRATCH_POOL/RESULT_POOL in other case. Is not it?
I would not mind to rename POOL to RESULT_POOL in this particular
case, but I'm not sure that we should use RESULT_POOL in all other
cases if function accepts one pool.
--
Ivan Zhakov
RE: svn commit: r1705060 - in /subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h serf.c util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: ivan@apache.org [mailto:ivan@apache.org]
> Sent: donderdag 24 september 2015 15:59
> To: commits@subversion.apache.org
> Subject: svn commit: r1705060 - in
> /subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h serf.c util.c
>
> Author: ivan
> Date: Thu Sep 24 13:59:16 2015
> New Revision: 1705060
>
> URL: http://svn.apache.org/viewvc?rev=1705060&view=rev
> Log:
> Refactor common code in ra_serf.
>
> * subversion/libsvn_ra_serf/ra_serf.h
> (svn_ra_serf__uri_parse): New function declaration.
>
> * subversion/libsvn_ra_serf/util.c
> (svn_ra_serf__uri_parse): New. Factored out from svn_ra_serf__open().
>
> * subversion/libsvn_ra_serf/serf.c
> (svn_ra_serf__open, svn_ra_serf__reparent): Use
> svn_ra_serf__uri_parse().
>
> Modified:
> subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
> subversion/trunk/subversion/libsvn_ra_serf/serf.c
> subversion/trunk/subversion/libsvn_ra_serf/util.c
>
> Modified: subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/
> ra_serf.h?rev=1705060&r1=1705059&r2=1705060&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h Thu Sep 24
> 13:59:16 2015
> @@ -1548,6 +1548,17 @@ svn_ra_serf__create_bucket_with_eagain(c
> apr_size_t len,
> serf_bucket_alloc_t *allocator);
>
> +/* Parse a given URL_STR, fill in all supplied fields of URI
> + * structure.
> + *
> + * This function is a compatibility wrapper around apr_uri_parse().
> + * Different apr-util versions set apr_uri_t.path to either NULL or ""
> + * for root paths, and serf expects to see "/". This function always
> + * sets URI.path to "/" for these paths. */
> +svn_error_t *
> +svn_ra_serf__uri_parse(apr_uri_t *uri,
> + const char *url_str,
> + apr_pool_t *pool);
I think the pool should be named result_pool here.
+1 on the rest.
Bert