You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2014/03/26 12:56:56 UTC
Re: svn commit: r1544182 - in /subversion/trunk/subversion: include/svn_client.h libsvn_client/cat.c libsvn_client/deprecated.c
Hi Bert...
> URL: http://svn.apache.org/r1544182
> Log:
> Make the 'svn_client_cat()' API a bit more generic usable for api users,
> by allowing the suppression of keyword expansion and by optionally returning
> the properties of the node.
Shouldn't we have an EOL-style translation option as well? Usually the two go together.
> * subversion/include/svn_client.h
> (svn_client_cat3): New function.
> (svn_client_cat2): Deprecate function.
>
> * subversion/libsvn_client/cat.c
> (svn_client_cat2): Rename to ...
> (svn_client_cat3): ... this. Add support for not expanding keywords and
> for returning the properties.
>
> * subversion/libsvn_client/deprecated.c
> (svn_client_cat2): New function.
> Modified: subversion/trunk/subversion/include/svn_client.h
> ==============================================================================
> /**
> * Output the content of a file.
> *
> - * @param[in] out The stream to which the content will be written.
> - * @param[in] path_or_url The path or URL of the file.
> - * @param[in] peg_revision The peg revision.
> - * @param[in] revision The operative revision.
> + * @param[out] props Optional output argument to obtain properties.
> + * @param[in] out The stream to which the content will be written.
> + * @param[in] path_or_url The path or URL of the file.
> + * @param[in] peg_revision The peg revision.
> + * @param[in] revision The operative revision.
> + * @param[in] expand_keywords When true, keywords (when set) are expanded.
And what if @a expand_keywords is false -- are keywords contracted to their repository-normal form, or left unchanged? The implementation looks like it leaves them unchanged.
> * @param[in] ctx The standard client context, used for possible
> * authentication.
> * @param[in] pool Used for any temporary allocation.
There's no longer a 'pool', there's a result_pool and a scratch_pool.
> *
> * @todo Add an expansion/translation flag?
That comment seems to be redundant now.
> *
> - * @since New in 1.2.
> - *
You should leave that "@since 1.2" in the deprecated function's doc string.
> * @see #svn_client_ctx_t <br> @ref clnt_revisions for
> * a discussion of operative and peg revisions.
> + *
> + * @since New in 1.9.
> */
> svn_error_t *
> +svn_client_cat3(apr_hash_t **props,
> + svn_stream_t *out,
> + const char *path_or_url,
> + const svn_opt_revision_t *peg_revision,
> + const svn_opt_revision_t *revision,
> + svn_boolean_t expand_keywords,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
> +
> +/**
> + * Similar to svn_client_cat3() except without the option of directly
> + * reading the properties, and with @a expand_keywords always TRUE.
... and only one pool.
> + *
> + * @deprecated Provided for backward compatibility with the 1.8 API.
> + */
> +SVN_DEPRECATED
> +svn_error_t *
> svn_client_cat2(svn_stream_t *out,
> const char *path_or_url,
> const svn_opt_revision_t *peg_revision,
>
> Modified: subversion/trunk/subversion/libsvn_client/cat.c
> ==============================================================================
> svn_error_t *
> -svn_client_cat2(svn_stream_t *out,
> +svn_client_cat3(apr_hash_t **props,
> + svn_stream_t *out,
> const char *path_or_url,
> const svn_opt_revision_t *peg_revision,
> const svn_opt_revision_t *revision,
> + svn_boolean_t expand_keywords,
> svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> {
> SVN_ERR(svn_client__get_normalized_stream(&normal_stream, ctx->wc_ctx,
> - local_abspath, revision, TRUE, FALSE,
> + local_abspath, revision,
> + expand_keywords, FALSE,
> - if (keywords)
> + if (keywords && expand_keywords)
> {
> SVN_ERR(svn_subst_build_keywords3(&kw, keywords->data,
> cmt_rev->data, loc->url,
> repos_root_url, when,
> cmt_author ?
> cmt_author->data : NULL,
> - pool));
> + scratch_pool));
> }
> else
> kw = NULL;
Re: svn commit: r1544182 - in /subversion/trunk/subversion: include/svn_client.h libsvn_client/cat.c libsvn_client/deprecated.c
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
>> Shouldn't we have an EOL-style translation option as well? Usually the
>> two go together.
>
> Not sure.
>
> svn_client_cat2() already implemented keyword expansion, but I don't think
> it changes EOL?
>
>
> I just added an option to allow disabling a magic feature of
> svn_client_cat2() that api developers could only work around by directly
> going to either the wc or ra apis.
>
> Translating the EOL style can be done by wrapping the passed output stream
> by a translation... So I don't see that as an as-interesting case.
OK.
>>> + * @param[in] expand_keywords When true, keywords (when set) are
>>> expanded.
>>
>> And what if @a expand_keywords is false -- are keywords contracted to their
>> repository-normal form, or left unchanged? The implementation looks like it
>> leaves them unchanged.
>
> We should never have expanded keywords in the repository normal form...
>
> And we certainly never retract them when translating from that form...
Yes, but 'cat' can also operate on a WC path with revision = 'working', and in that case it's not clear whether the caller should expect keywords to be contracted or left as-is.
> I removed the documentation for pool and the todo in r1581796.
Thanks for that and the other tweaks.
>>> +/**
>>> + * Similar to svn_client_cat3() except without the option of directly
>>> + * reading the properties, and with @a expand_keywords always TRUE.
>>
>> ... and only one pool.
>
> I don't think we document those pool changes in other similar cases.
> (svn_client_cat2() didn't have output arguments, so I don't think there
> is anything special to document here)
We do normally document the pools in public API functions, although I find it often seems rather redundant. (In some cases there can be non-obvious implications but we aren't always careful to document those...)
- Julian
RE: svn commit: r1544182 - in /subversion/trunk/subversion: include/svn_client.h libsvn_client/cat.c libsvn_client/deprecated.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: woensdag 26 maart 2014 12:57
> To: Bert Huijben
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r1544182 - in /subversion/trunk/subversion:
> include/svn_client.h libsvn_client/cat.c libsvn_client/deprecated.c
>
> Hi Bert...
>
> > URL: http://svn.apache.org/r1544182
>
> > Log:
> > Make the 'svn_client_cat()' API a bit more generic usable for api users,
> > by allowing the suppression of keyword expansion and by optionally
> returning
> > the properties of the node.
>
> Shouldn't we have an EOL-style translation option as well? Usually the two
go
> together.
Not sure.
svn_client_cat2() already implemented keyword expansion, but I don't think
it changes EOL?
I just added an option to allow disabling a magic feature of
svn_client_cat2() that api developers could only work around by directly
going to either the wc or ra apis.
Translating the EOL style can be done by wrapping the passed output stream
by a translation... So I don't see that as an as-interesting case.
> > * subversion/include/svn_client.h
> > (svn_client_cat3): New function.
> > (svn_client_cat2): Deprecate function.
> >
> > * subversion/libsvn_client/cat.c
> > (svn_client_cat2): Rename to ...
> > (svn_client_cat3): ... this. Add support for not expanding keywords
and
> > for returning the properties.
> >
> > * subversion/libsvn_client/deprecated.c
> > (svn_client_cat2): New function.
>
> > Modified: subversion/trunk/subversion/include/svn_client.h
> >
> ==========================================================
> ====================
> > /**
> > * Output the content of a file.
> > *
> > - * @param[in] out The stream to which the content will be
written.
> > - * @param[in] path_or_url The path or URL of the file.
> > - * @param[in] peg_revision The peg revision.
> > - * @param[in] revision The operative revision.
> > + * @param[out] props Optional output argument to obtain
> properties.
> > + * @param[in] out The stream to which the content will be
written.
> > + * @param[in] path_or_url The path or URL of the file.
> > + * @param[in] peg_revision The peg revision.
> > + * @param[in] revision The operative revision.
> > + * @param[in] expand_keywords When true, keywords (when set) are
> expanded.
>
> And what if @a expand_keywords is false -- are keywords contracted to
their
> repository-normal form, or left unchanged? The implementation looks like
it
> leaves them unchanged.
We should never have expanded keywords in the repository normal form...
And we certainly never retract them when translating from that form...
>
> > * @param[in] ctx The standard client context, used for possible
> > * authentication.
> > * @param[in] pool Used for any temporary allocation.
>
> There's no longer a 'pool', there's a result_pool and a scratch_pool.
>
> > *
> > * @todo Add an expansion/translation flag?
>
> That comment seems to be redundant now.
I removed the documentation for pool and the todo in r1581796.
>
> > *
> > - * @since New in 1.2.
> > - *
>
> You should leave that "@since 1.2" in the deprecated function's doc
string.
Fixed in r1581796.
> > * @see #svn_client_ctx_t <br> @ref clnt_revisions for
> > * a discussion of operative and peg revisions.
> > + *
> > + * @since New in 1.9.
> > */
> > svn_error_t *
> > +svn_client_cat3(apr_hash_t **props,
> > + svn_stream_t *out,
> > + const char *path_or_url,
> > + const svn_opt_revision_t *peg_revision,
> > + const svn_opt_revision_t *revision,
> > + svn_boolean_t expand_keywords,
> > + svn_client_ctx_t *ctx,
> > + apr_pool_t *result_pool,
> > + apr_pool_t *scratch_pool);
> > +
> > +/**
> > + * Similar to svn_client_cat3() except without the option of directly
> > + * reading the properties, and with @a expand_keywords always TRUE.
>
> ... and only one pool.
I don't think we document those pool changes in other similar cases.
(svn_client_cat2() didn't have output arguments, so I don't think there is
anything special to document here)
Bert