On Sat, Feb 09, 2013 at 09:21:56PM +0100, Stefan Sperling wrote:
> I'll try to review this during the coming week. Thanks!
Hi again,
Please see below for my review.
> > Index: subversion/include/private/svn_subst_private.h
> > ===================================================================
> > --- subversion/include/private/svn_subst_private.h (revision 0)
> > +++ subversion/include/private/svn_subst_private.h (working copy)
> > @@ -0,0 +1,68 @@
> > +/**
> > + * @copyright
> > + * ====================================================================
> > + * Licensed to the Apache Software Foundation (ASF) under one
> > + * or more contributor license agreements. See the NOTICE file
> > + * distributed with this work for additional information
> > + * regarding copyright ownership. The ASF licenses this file
> > + * to you under the Apache License, Version 2.0 (the
> > + * "License"); you may not use this file except in compliance
> > + * with the License. You may obtain a copy of the License at
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing,
> > + * software distributed under the License is distributed on an
> > + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> > + * KIND, either express or implied. See the License for the
> > + * specific language governing permissions and limitations
> > + * under the License.
> > + * ====================================================================
> > + * @endcopyright
> > + *
> > + * @file svn_subst_private.h
> > + * @brief Non-public subst utility functions.
> > + */
> > +
> > +
> > +#ifndef SVN_SUBST_PRIVATE_H
> > +#define SVN_SUBST_PRIVATE_H
> > +
> > +#include "svn_subst.h" /* for svn_boolean_t, svn_error_t */
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif /* __cplusplus */
> > +
> > +/**
> > + * Set @a *kw to a new keywords hash filled with the appropriate contents
> > + * given a @a keywords_string (the contents of the svn:keywords
> > + * property for the file in question), the revision @a rev, the @a url,
> > + * the url of the root of the @a repos, the @a date the file was committed
> > + * on, and the @a author of the last commit. Any of these can be @c NULL
> > + * to indicate that the information is not present, or @c 0 for @a date.
> > + *
> > + * Hash keys are of type <tt>const char *</tt>.
> > + * Hash values are of type <tt>svn_string_t *</tt>.
> > + *
> > + * All memory is allocated out of @a pool.
> > + *
> > + * @since New in 1.6
> > + */
> > +svn_error_t *
> > +svn_subst_build_keywords3(apr_hash_t **kw,
> > + const char *keywords_string,
> > + const char *rev,
> > + const char *url,
> > + const char *repos,
> > + apr_time_t date,
> > + const char *author,
> > + apr_pool_t *pool);
> > +
Pleaes don't add a new header for this function.
It should be declared in svn_subst.h as it it a public API.
It should replace svn_subst_build_keywords2(), so the
svn_subst_build_keywords2() function should be marked deprecated.
The above docstring seems to be based on an outdated version of the
svn_subst_build_keywords2() docstring.
Here's what I'd suggest using instead (as a diff against svn_subst.h):
[[[
Index: subversion/include/svn_subst.h
===================================================================
--- subversion/include/svn_subst.h (revision 1445280)
+++ subversion/include/svn_subst.h (working copy)
@@ -127,13 +127,13 @@ typedef struct svn_subst_keywords_t
* Set @a *kw to a new keywords hash filled with the appropriate contents
* given a @a keywords_string (the contents of the svn:keywords
* property for the file in question), the revision @a rev, the @a url,
- * the @a date the file was committed on, and the @a author of the last
- * commit.
+ * the @a date the file was committed on, the @a author of the last
+ * commit, and the URL of the repository root @a repos_root_url.
*
- * Any of the inputs @a rev, @a url, @a date and @a author can be @c NULL,
- * or @c 0 for @a date, to indicate that the information is not present.
- * Each piece of information that is not present expands to the empty
- * string wherever it appears in an expanded keyword value. (This can
+ * Any of the inputs @a rev, @a url, @a date, @a author, and @a repos_root_url
+ * can be @c NULL, or @c 0 for @a date, to indicate that the information is
+ * not present. Each piece of information that is not present expands to the
+ * empty string wherever it appears in an expanded keyword value. (This can
* result in multiple adjacent spaces in the expansion of a multi-valued
* keyword such as "Id".)
*
@@ -142,8 +142,25 @@ typedef struct svn_subst_keywords_t
*
* All memory is allocated out of @a pool.
*
+ * @since New in 1.8.
+ */
+svn_error_t *
+svn_subst_build_keywords3(apr_hash_t **kw,
+ const char *keywords_string,
+ const char *rev,
+ const char *url,
+ const char *repos_root_url,
+ apr_time_t date,
+ const char *author,
+ apr_pool_t *pool);
+
+/** Similar to svn_subst_build_keywords3() except that it does not accept
+ * the @a repos_root_url parameter and hence supports less substitutions.
+ *
* @since New in 1.3.
+ * @deprecated Provided for backward compatibility with the 1.7 API.
*/
+SVN_DEPRECATED
svn_error_t *
svn_subst_build_keywords2(apr_hash_t **kw,
const char *keywords_string,
]]]
> > +
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif /* __cplusplus */
> > +
> > +#endif /* SVN_SUBST_PRIVATE_H */
> > Index: subversion/libsvn_client/cat.c
> > ===================================================================
> > --- subversion/libsvn_client/cat.c (revision 1441445)
> > +++ subversion/libsvn_client/cat.c (working copy)
> > @@ -40,6 +40,7 @@
> >
> > #include "svn_private_config.h"
> > #include "private/svn_wc_private.h"
> > +#include "private/svn_subst_private.h"
> >
> >
> > /*** Code. ***/
> > @@ -124,12 +125,15 @@
> > const char *author;
> > const char *url;
> > apr_time_t tm;
> > + const char *repos;
Bad variable name. It isn't clear what this is referring to.
It is a repository URL or a path in the repository?
I suppose either repos_relpath or repos_root_url would be suitable
here. Based on what svn_subst_build_keywords3() expects I suppose
repos_root_url is intended here?
> >
> > SVN_ERR(svn_wc__node_get_changed_info(&changed_rev, &tm, &author, wc_ctx,
> > local_abspath, scratch_pool,
> > scratch_pool));
> > SVN_ERR(svn_wc__node_get_url(&url, wc_ctx, local_abspath, scratch_pool,
> > scratch_pool));
> > + SVN_ERR(svn_wc__node_get_repos_info(&repos, NULL, wc_ctx, local_abspath,
This function takes additional arguments on trunk, so the patch
doesn't compile as it is.
> > + scratch_pool, scratch_pool));
> >
> > if (local_mod)
> > {
> > @@ -152,8 +156,8 @@
> > rev_str = apr_psprintf(scratch_pool, "%ld", changed_rev);
> > }
> >
> > - SVN_ERR(svn_subst_build_keywords2(&kw, keywords->data, rev_str, url, tm,
> > - author, scratch_pool));
> > + SVN_ERR(svn_subst_build_keywords3(&kw, keywords->data, rev_str, url,
> > + repos, tm, author, scratch_pool));
> > }
> >
> > /* Wrap the output stream if translation is needed. */
> > @@ -181,6 +185,7 @@
> > svn_string_t *eol_style;
> > svn_string_t *keywords;
> > apr_hash_t *props;
> > + const char *repos_root_url;
Much better name! :)
> > svn_stream_t *output = out;
> > svn_error_t *err;
> >
> > @@ -225,6 +230,9 @@
> > peg_revision,
> > revision, ctx, pool));
> >
> > + /* Find the repos root URL */
> > + SVN_ERR(svn_ra_get_repos_root2(ra_session, &repos_root_url, pool));
> > +
> > /* Grab some properties we need to know in order to figure out if anything
> > special needs to be done with this file. */
> > err = svn_ra_get_file(ra_session, "", loc->rev, NULL, NULL, &props, pool);
> > @@ -275,10 +283,11 @@
> > if (cmt_date)
> > SVN_ERR(svn_time_from_cstring(&when, cmt_date->data, pool));
> >
> > - SVN_ERR(svn_subst_build_keywords2
> > + SVN_ERR(svn_subst_build_keywords3
> > (&kw, keywords->data,
> > cmt_rev->data,
> > loc->url,
> > + repos_root_url,
Please don't use tabs for indentation, only spaces.
The coding conventions have been in place for a decade and we
try to stick to them for consistency, not necessarily beauty (I don't
really like the indentation style personally but I still stick to it).
> > when,
> > cmt_author ? cmt_author->data : NULL,
> > pool));
> > Index: subversion/libsvn_client/commit.c
> > ===================================================================
> > --- subversion/libsvn_client/commit.c (revision 1441445)
> > +++ subversion/libsvn_client/commit.c (working copy)
> > @@ -44,6 +44,7 @@
> > #include "client.h"
> > #include "private/svn_wc_private.h"
> > #include "private/svn_ra_private.h"
> > +#include "private/svn_subst_private.h"
> >
> > #include "svn_private_config.h"
> >
Why is this file changed? The only change seems to be an additional
include. And there's no need to include this header in the first place if
you declare svn_subst_build_keywords3() in svn_subst.h.
> > Index: subversion/libsvn_client/export.c
> > ===================================================================
> > --- subversion/libsvn_client/export.c (revision 1441445)
> > +++ subversion/libsvn_client/export.c (working copy)
> > @@ -45,6 +45,7 @@
> > #include "private/svn_subr_private.h"
> > #include "private/svn_delta_private.h"
> > #include "private/svn_wc_private.h"
> > +#include "private/svn_subst_private.h"
> >
> > #ifndef ENABLE_EV2_IMPL
> > #define ENABLE_EV2_IMPL 0
> > @@ -377,6 +378,7 @@
> > {
> > svn_revnum_t changed_rev = status->changed_rev;
> > const char *suffix;
> > + const char *repos;
Again, repos_relpath or repos_root_url please.
> > const char *url = svn_path_url_add_component2(status->repos_root_url,
> > status->repos_relpath,
> > scratch_pool);
> > @@ -395,10 +397,13 @@
> > suffix = "";
> > }
> >
> > - SVN_ERR(svn_subst_build_keywords2
> > + SVN_ERR(svn_wc__node_get_repos_info(&repos, NULL, wc_ctx,
> > + eib->origin_abspath /* XXX: ?? from_abspath */,
Seems whoever wrote the above comment wasn't sure what they were doing.
This should be checked.
> > + scratch_pool, scratch_pool));
> > + SVN_ERR(svn_subst_build_keywords3
> > (&kw, keywords->data,
> > apr_psprintf(scratch_pool, "%ld%s", changed_rev, suffix),
> > - url, tm, author, scratch_pool));
> > + url, repos, tm, author, scratch_pool));
> > }
> >
> > /* For atomicity, we translate to a tmp file and then rename the tmp file
> > @@ -544,6 +549,7 @@
> > /* Any keyword vals to be substituted */
> > const char *revision;
> > const char *url;
> > + const char *repos;
Again, bad variable name.
> > const char *author;
> > apr_time_t date;
> >
> > @@ -665,6 +671,7 @@
> > fb->edit_baton = eb;
> > fb->path = full_path;
> > fb->url = full_url;
> > + fb->repos = eb->root_url;
Same.
> > fb->pool = pool;
> >
> > *baton = fb;
> > @@ -830,8 +837,8 @@
> > }
> >
> > if (fb->keywords_val)
> > - SVN_ERR(svn_subst_build_keywords2(&final_kw, fb->keywords_val->data,
> > - fb->revision, fb->url, fb->date,
> > + SVN_ERR(svn_subst_build_keywords3(&final_kw, fb->keywords_val->data,
> > + fb->revision, fb->url, fb->repos, fb->date,
> > fb->author, pool));
> >
> > SVN_ERR(svn_subst_copy_and_translate4(fb->tmppath, fb->path,
> > Index: subversion/libsvn_client/import.c
> > ===================================================================
> > --- subversion/libsvn_client/import.c (revision 1441445)
> > +++ subversion/libsvn_client/import.c (working copy)
> > @@ -49,6 +49,7 @@
> > #include "private/svn_subr_private.h"
> > #include "private/svn_ra_private.h"
> > #include "private/svn_magic.h"
> > +#include "private/svn_subst_private.h"
> >
> > #include "svn_private_config.h"
> >
> > @@ -133,9 +134,9 @@
> > }
> >
> > if (keywords_val)
> > - SVN_ERR(svn_subst_build_keywords2(&keywords, keywords_val->data,
> > + SVN_ERR(svn_subst_build_keywords3(&keywords, keywords_val->data,
> > APR_STRINGIFY(SVN_INVALID_REVNUM),
> > - "", 0, "", pool));
> > + "", "", 0, "", pool));
> > else
> > keywords = NULL;
> >
> > Index: subversion/libsvn_subr/subst.c
> > ===================================================================
> > --- subversion/libsvn_subr/subst.c (revision 1441445)
> > +++ subversion/libsvn_subr/subst.c (working copy)
> > @@ -49,6 +49,7 @@
> > #include "svn_private_config.h"
> >
> > #include "private/svn_string_private.h"
> > +#include "private/svn_subst_private.h"
> >
> > /**
> > * The textual elements of a detranslated special file. One of these
> > @@ -135,8 +136,11 @@
> > * %b basename of the URL of this file
> > * %d short format of date of this revision
> > * %D long format of date of this revision
> > + * %P path relative to root of repos
> > * %r number of this revision
> > + * %R root url of repository
> > * %u URL of this file
> > + * %_ a space
> > * %% a literal %
Are supported expansions documented anywhere else than this code comment?
Seems strange to document them here.
Perhaps the only other place is in the svnbook, in which case we don't
need any more documentation in this patch. However, if there's a public
API docstring somewhere that documents them, we should update it, too.
I don't know if such a docstring exists, though. If it doesn't, we should
consider moving this coment or parts of it into svn_subst.h. But that can
be done in a separate patch as it's a separate documentation problem.
> > *
> > * All memory is allocated out of @a pool.
> > @@ -145,12 +149,14 @@
> > keyword_printf(const char *fmt,
> > const char *rev,
> > const char *url,
> > + const char *repos,
Bad name again.
> > apr_time_t date,
> > const char *author,
> > apr_pool_t *pool)
> > {
> > svn_stringbuf_t *value = svn_stringbuf_ncreate("", 0, pool);
> > const char *cur;
> > + const char *relative;
Same. repos_relpath please.
> > size_t n;
> >
> > for (;;)
> > @@ -203,6 +209,23 @@
> > svn_stringbuf_appendcstr(value,
> > svn_time_to_human_cstring(date, pool));
> > break;
> > + case 'P': /* relative path of this file */
> > + relative = url;
> > + if (relative && repos)
> > + {
> > + int len = strlen(repos);
> > +
> > + if (strncmp(repos, relative, len) == 0
> > + && relative[len] == '/')
> > + relative += len + 1;
There's tabs all over the place in this section.
I'm not sure I like the manual URL munging here. Consider checking
svn_dirent_uri.h for a suitable helper functions to split the in-repos
path portion off the URL (e.g. svn_uri_skip_ancestor() might work).
> > + }
> > + if (relative)
> > + svn_stringbuf_appendcstr(value, relative);
> > + break;
> > + case 'R': /* root of repos */
> > + if (repos)
> > + svn_stringbuf_appendcstr(value, repos);
> > + break;
> > case 'r': /* number of this revision */
> > if (rev)
> > svn_stringbuf_appendcstr(value, rev);
> > @@ -211,6 +234,9 @@
> > if (url)
> > svn_stringbuf_appendcstr(value, url);
> > break;
> > + case '_': /* '%_' => a space */
> > + svn_stringbuf_appendbytes(value, " ", 1);
Consider using svn_stringbuf_appendbyte(), which is new in trunk.
> > + break;
> > case '%': /* '%%' => a literal % */
> > svn_stringbuf_appendbyte(value, *cur);
> > break;
> > @@ -246,8 +272,8 @@
> > apr_hash_t *kwhash;
> > const svn_string_t *val;
> >
> > - SVN_ERR(svn_subst_build_keywords2(&kwhash, keywords_val, rev,
> > - url, date, author, pool));
> > + SVN_ERR(svn_subst_build_keywords3(&kwhash, keywords_val, rev,
> > + url, "", date, author, pool));
> >
> > /* The behaviour of pre-1.3 svn_subst_build_keywords, which we are
> > * replicating here, is to write to a slot in the svn_subst_keywords_t
> > @@ -286,6 +312,21 @@
> > const char *author,
> > apr_pool_t *pool)
> > {
> > + SVN_ERR(svn_subst_build_keywords3(kw, keywords_val, rev,
> > + url, "", date, author, pool));
> > + return SVN_NO_ERROR;
> > +}
> > +
> > +svn_error_t *
> > +svn_subst_build_keywords3(apr_hash_t **kw,
> > + const char *keywords_val,
> > + const char *rev,
> > + const char *url,
> > + const char *repos,
> > + apr_time_t date,
> > + const char *author,
> > + apr_pool_t *pool)
> > +{
> > apr_array_header_t *keyword_tokens;
> > int i;
> > *kw = apr_hash_make(pool);
> > @@ -296,14 +337,32 @@
> > for (i = 0; i < keyword_tokens->nelts; ++i)
> > {
> > const char *keyword = APR_ARRAY_IDX(keyword_tokens, i, const char *);
> > + apr_array_header_t *keyword_tokens2;
> >
> > + keyword_tokens2 = svn_cstring_split(keyword, "=", TRUE /* chop */, pool);
> > + if (keyword_tokens2->nelts == 2)
> > + {
> > + svn_string_t *custom_val;
> > + const char *custom_expand;
> > +
> > + keyword = APR_ARRAY_IDX(keyword_tokens2, 0, const char*);
> > + custom_expand = APR_ARRAY_IDX(keyword_tokens2, 1, const char*);
> > + if (! strcmp(custom_expand, "%H"))
> > + custom_expand = "%P %r %d %a";
> > + else if (! strcmp(custom_expand, "%I"))
> > + custom_expand = "%b %r %d %a";
Tabs above.
Are the new %H and %I expansions documented anywhere?
What is the reason to add them? Did they exist in CVS?
The rest looks good (apart from further use of the variable name 'repos').
Thanks! Are you willing to spend time on providing an updated patch?
Please also include a log message as described here:
http://subversion.apache.org/docs/community-guide/general.html#patches
Would you also be willing to tweak the regression tests in
subversion/test/cmdline/trans_tests.py to account for the
new keyword substitutions?
> > + custom_val = keyword_printf(custom_expand, rev, url, repos, date, author, pool);
> > + apr_hash_set(*kw, keyword, APR_HASH_KEY_STRING, custom_val);
> > + return SVN_NO_ERROR;
> > + }
> > +
> > if ((! strcmp(keyword, SVN_KEYWORD_REVISION_LONG))
> > || (! strcmp(keyword, SVN_KEYWORD_REVISION_MEDIUM))
> > || (! svn_cstring_casecmp(keyword, SVN_KEYWORD_REVISION_SHORT)))
> > {
> > svn_string_t *revision_val;
> >
> > - revision_val = keyword_printf("%r", rev, url, date, author, pool);
> > + revision_val = keyword_printf("%r", rev, url, repos, date, author, pool);
> > apr_hash_set(*kw, SVN_KEYWORD_REVISION_LONG,
> > APR_HASH_KEY_STRING, revision_val);
> > apr_hash_set(*kw, SVN_KEYWORD_REVISION_MEDIUM,
> > @@ -316,7 +375,7 @@
> > {
> > svn_string_t *date_val;
> >
> > - date_val = keyword_printf("%D", rev, url, date, author, pool);
> > + date_val = keyword_printf("%D", rev, url, repos, date, author, pool);
> > apr_hash_set(*kw, SVN_KEYWORD_DATE_LONG,
> > APR_HASH_KEY_STRING, date_val);
> > apr_hash_set(*kw, SVN_KEYWORD_DATE_SHORT,
> > @@ -327,7 +386,7 @@
> > {
> > svn_string_t *author_val;
> >
> > - author_val = keyword_printf("%a", rev, url, date, author, pool);
> > + author_val = keyword_printf("%a", rev, url, repos, date, author, pool);
> > apr_hash_set(*kw, SVN_KEYWORD_AUTHOR_LONG,
> > APR_HASH_KEY_STRING, author_val);
> > apr_hash_set(*kw, SVN_KEYWORD_AUTHOR_SHORT,
> > @@ -338,7 +397,7 @@
> > {
> > svn_string_t *url_val;
> >
> > - url_val = keyword_printf("%u", rev, url, date, author, pool);
> > + url_val = keyword_printf("%u", rev, url, repos, date, author, pool);
> > apr_hash_set(*kw, SVN_KEYWORD_URL_LONG,
> > APR_HASH_KEY_STRING, url_val);
> > apr_hash_set(*kw, SVN_KEYWORD_URL_SHORT,
> > @@ -348,7 +407,7 @@
> > {
> > svn_string_t *id_val;
> >
> > - id_val = keyword_printf("%b %r %d %a", rev, url, date, author,
> > + id_val = keyword_printf("%b %r %d %a", rev, url, repos, date, author,
> > pool);
> > apr_hash_set(*kw, SVN_KEYWORD_ID,
> > APR_HASH_KEY_STRING, id_val);
> > @@ -357,7 +416,7 @@
> > {
> > svn_string_t *header_val;
> >
> > - header_val = keyword_printf("%u %r %d %a", rev, url, date, author,
> > + header_val = keyword_printf("%u %r %d %a", rev, url, repos, date, author,
> > pool);
> > apr_hash_set(*kw, SVN_KEYWORD_HEADER,
> > APR_HASH_KEY_STRING, header_val);
> > Index: subversion/libsvn_wc/translate.c
> > ===================================================================
> > --- subversion/libsvn_wc/translate.c (revision 1441445)
> > +++ subversion/libsvn_wc/translate.c (working copy)
> > @@ -46,6 +46,7 @@
> >
> > #include "svn_private_config.h"
> > #include "private/svn_wc_private.h"
> > +#include "private/svn_subst_private.h"
> >
> >
> >
> > @@ -313,10 +314,10 @@
> > apr_time_t changed_date;
> > const char *changed_author;
> > const char *url;
> > + const char *repos_root_url;
> >
> > if (! for_normalization)
> > {
> > - const char *repos_root_url;
> > const char *repos_relpath;
> >
> > SVN_ERR(svn_wc__db_read_info(NULL, NULL, NULL, &repos_relpath,
> > @@ -341,13 +342,23 @@
> > changed_rev = SVN_INVALID_REVNUM;
> > changed_date = 0;
> > changed_author = "";
> > +
> > + SVN_ERR(svn_wc__db_read_info(NULL, NULL, NULL, NULL,
> > + &repos_root_url, NULL, NULL,
> > + NULL, NULL, NULL,
> > + NULL, NULL, NULL, NULL, NULL, NULL,
> > + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > + NULL, NULL, NULL, NULL,
> > + db, local_abspath,
> > + scratch_pool, scratch_pool));
> > }
> >
> > - SVN_ERR(svn_subst_build_keywords2(keywords,
> > + SVN_ERR(svn_subst_build_keywords3(keywords,
> > keyword_list,
> > apr_psprintf(scratch_pool, "%ld",
> > changed_rev),
> > url,
> > + repos_root_url,
> > changed_date,
> > changed_author,
> > result_pool));
Received on 2013-02-12 20:45:13 CET