You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2013/08/15 13:48:38 UTC
[PATCH] Add serf library version information to svn --version
Hi,
I think it will be useful to have svn --version display serf library
version information. Attached patch makes output of svn --version like
this:
[[[
C:\Ivan\SVN\trunk\Debug\subversion\svn>svn.exe --version
svn, version 1.9.0-dev (under development)
compiled Aug 7 2013, 15:48:17 on x86/x86_64-microsoft-windows6.1.7601
Copyright (C) 2013 The Apache Software Foundation.
This software consists of contributions made by many people;
see the NOTICE file for more information.
Subversion is open source software, see http://subversion.apache.org/
The following repository access (RA) modules are available:
* ra_svn : Module for accessing a repository using the svn network protocol.
- handles 'svn' scheme
* ra_local : Module for accessing a repository on local disk.
- handles 'file' scheme
* ra_serf : Module for accessing a repository via WebDAV protocol
using serf 1.2.99.
- handles 'http' scheme
- handles 'https' scheme
]]]
It will be very useful for diagnostic serf/ra_serf issues.
Log message:
[[[
Add serf library version to svn --version output.
* subversion/libsvn_ra_serf/serf.c
(RA_SERF_DESCRIPTION): Add version placeholders.
(ra_serf_get_description): Add serf library version information to RA layer
description.
]]]
Thoughts?
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: [PATCH] Add serf library version information to svn --version
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, Aug 15, 2013 at 8:07 PM, Ben Reser <be...@reser.org> wrote:
> On 8/15/13 4:48 AM, Ivan Zhakov wrote:
>> Thoughts?
>
> It'd be nice if this was printed under the linked dependencies section
> of `svn --version --verbose`, but I don't see a good way of adding that
> since serf is required by the dynamically loadable ra layer and isn't a
> core dependency.
>
> Maybe we should extend the ra layer vtable to provide a function to ask
> for linked libs to be added to that list. Since you're already talking
> about changing the vtable that would be the solution I would prefer.
>
> Shoving the version number of the library in the description doesn't fit
> with the existing behavior of our output and forces users of our library
> that want to find this information out to parse the description.
>
> That said I'd be fine with doing this in this manor for 1.8.x if we
> don't want to rev the vtable for it. It seems like a reasonable quick
> hack to get the information available.
>
I agree that using RA description for version information is not good
thing in general. But ra layer vtable is not solution also, because
printing dependencies done in libsvn_subr and libsvn_subr doesn't
depend on libsvn_ra. So I think we could stay with implemented
solution at least for 1.8.x.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: [PATCH] Add serf library version information to svn --version
Posted by Ben Reser <be...@reser.org>.
On 8/15/13 4:48 AM, Ivan Zhakov wrote:
> Thoughts?
It'd be nice if this was printed under the linked dependencies section
of `svn --version --verbose`, but I don't see a good way of adding that
since serf is required by the dynamically loadable ra layer and isn't a
core dependency.
Maybe we should extend the ra layer vtable to provide a function to ask
for linked libs to be added to that list. Since you're already talking
about changing the vtable that would be the solution I would prefer.
Shoving the version number of the library in the description doesn't fit
with the existing behavior of our output and forces users of our library
that want to find this information out to parse the description.
That said I'd be fine with doing this in this manor for 1.8.x if we
don't want to rev the vtable for it. It seems like a reasonable quick
hack to get the information available.
Re: [PATCH] Add serf library version information to svn --version
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, Aug 15, 2013 at 5:38 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> On Thu, Aug 15, 2013 at 4:14 PM, Philip Martin <ph...@codematters.co.uk> wrote:
>>> Philip Martin <ph...@wandisco.com> writes:
>>>
>>>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>>>
>>>>> Good point. I'll fix it in separate commit. What do you think about
>>>>> patch itself?
>>>>
>>>> Is there any reason for the static optimisation? Does performance
>>>> matter? Why not simply format each time?
>>>
>>> Ah! You don't have a pool.
>>>
>> Yes, that is the problem. But I think we can change vtable for RA
>> layer since it's not part of our API?
>
> Yes, the vtable is private and can be changed.
>
> For the original patch an optimising compiler is allowed to reorder the
> call to apr_snprintf and the assignment to description. However since
> description_buf is static it will initially be all null so even if there
> is a thread race the returned buffer should always be a null-terminated
> string.
>
I've added pool parameter to get_description() RA vtable callback and
commited in Completed: r1514295. Thanks for review!
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: [PATCH] Add serf library version information to svn --version
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> On Thu, Aug 15, 2013 at 4:14 PM, Philip Martin <ph...@codematters.co.uk> wrote:
>> Philip Martin <ph...@wandisco.com> writes:
>>
>>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>>
>>>> Good point. I'll fix it in separate commit. What do you think about
>>>> patch itself?
>>>
>>> Is there any reason for the static optimisation? Does performance
>>> matter? Why not simply format each time?
>>
>> Ah! You don't have a pool.
>>
> Yes, that is the problem. But I think we can change vtable for RA
> layer since it's not part of our API?
Yes, the vtable is private and can be changed.
For the original patch an optimising compiler is allowed to reorder the
call to apr_snprintf and the assignment to description. However since
description_buf is static it will initially be all null so even if there
is a thread race the returned buffer should always be a null-terminated
string.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: [PATCH] Add serf library version information to svn --version
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, Aug 15, 2013 at 4:14 PM, Philip Martin <ph...@codematters.co.uk> wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>
>>> Good point. I'll fix it in separate commit. What do you think about
>>> patch itself?
>>
>> Is there any reason for the static optimisation? Does performance
>> matter? Why not simply format each time?
>
> Ah! You don't have a pool.
>
Yes, that is the problem. But I think we can change vtable for RA
layer since it's not part of our API?
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: [PATCH] Add serf library version information to svn --version
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@wandisco.com> writes:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> Good point. I'll fix it in separate commit. What do you think about
>> patch itself?
>
> Is there any reason for the static optimisation? Does performance
> matter? Why not simply format each time?
Ah! You don't have a pool.
--
Philip
Re: [PATCH] Add serf library version information to svn --version
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> Good point. I'll fix it in separate commit. What do you think about
> patch itself?
Is there any reason for the static optimisation? Does performance
matter? Why not simply format each time?
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: [PATCH] Add serf library version information to svn --version
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, Aug 15, 2013 at 3:59 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> Log message:
>> [[[
>> Add serf library version to svn --version output.
>>
>> * subversion/libsvn_ra_serf/serf.c
>> (RA_SERF_DESCRIPTION): Add version placeholders.
>> (ra_serf_get_description): Add serf library version information to RA layer
>> description.
>> ]]]
>>
>> Thoughts?
>
> I noticed today that the default user agent string produced by ra_serf
> is
>
> #define USER_AGENT "SVN/" SVN_VER_NUMBER " (" SVN_BUILD_TARGET ")" \
> " serf/" \
> APR_STRINGIFY(SERF_MAJOR_VERSION) "." \
> APR_STRINGIFY(SERF_MINOR_VERSION) "." \
> APR_STRINGIFY(SERF_PATCH_VERSION)
>
> so it's fixed at compile time. If I build against a serf shared library
> and then upgrade the serf library the default user agent remains at the
> old value. I think the user agent should use serf_lib_version as well.
>
Good point. I'll fix it in separate commit. What do you think about
patch itself?
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: [PATCH] Add serf library version information to svn --version
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> Log message:
> [[[
> Add serf library version to svn --version output.
>
> * subversion/libsvn_ra_serf/serf.c
> (RA_SERF_DESCRIPTION): Add version placeholders.
> (ra_serf_get_description): Add serf library version information to RA layer
> description.
> ]]]
>
> Thoughts?
I noticed today that the default user agent string produced by ra_serf
is
#define USER_AGENT "SVN/" SVN_VER_NUMBER " (" SVN_BUILD_TARGET ")" \
" serf/" \
APR_STRINGIFY(SERF_MAJOR_VERSION) "." \
APR_STRINGIFY(SERF_MINOR_VERSION) "." \
APR_STRINGIFY(SERF_PATCH_VERSION)
so it's fixed at compile time. If I build against a serf shared library
and then upgrade the serf library the default user agent remains at the
old value. I think the user agent should use serf_lib_version as well.
> Index: subversion/libsvn_ra_serf/serf.c
> ===================================================================
> --- subversion/libsvn_ra_serf/serf.c (revision 1513462)
> +++ subversion/libsvn_ra_serf/serf.c (working copy)
> @@ -59,13 +59,25 @@
> }
>
> #define RA_SERF_DESCRIPTION \
> - N_("Module for accessing a repository via WebDAV protocol using serf.")
> + N_("Module for accessing a repository via WebDAV protocol using serf %d.%d.%d.")
>
> /* Implements svn_ra__vtable_t.get_description(). */
> static const char *
> ra_serf_get_description(void)
> {
> - return _(RA_SERF_DESCRIPTION);
> + static char description_buf[256];
> + static char *description = NULL;
> +
> + if (!description)
> + {
> + int major, minor, patch;
> + serf_lib_version(&major, &minor, &patch);
> + apr_snprintf(description_buf, sizeof(description_buf),
> + _(RA_SERF_DESCRIPTION), major, minor, patch);
> + description = description_buf;
> + }
> +
> + return description;
> }
>
> /* Implements svn_ra__vtable_t.get_schemes(). */
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*