You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@subversion.apache.org by Alfred Perlstein <br...@mu.org> on 2013/02/09 16:12:38 UTC

PATCH: keyword expansion 1.8

[[ Adding users@subversion.apache.org ]]

He folks.  I am in no way a Subversion expert, however per request I've 
taken the FreeBSD patch for adding custom keyword expansion and forward 
ported it to what is soon to be Subversion 1.8.

I am wondering if anyone can help me test this or give me some things to 
try to ensure that it works properly.

-Alfred

On 2/1/13 7:40 AM, Alfred Perlstein wrote:
> [[ bringing in Lev Serebryakov, FreeBSD port maintainer for Subversion ]]
>
> Hello Lev,
>
> Stefan of the Subversion project asked me about bringing in our 
> keywords patch for subversion 1.8.
>
> I've taken a bit of time and gotten the patch to apply to subversion 
> trunk (untested).
>
> Can you have a look and let us know what we need to do with regards to 
> testing or anything else?
>
>
> Stefan, any suggestions on going forward?
>
> -Alfred
>
> On 2/1/13 8:34 AM, Stefan Sperling wrote:
>> On Fri, Feb 01, 2013 at 08:01:02AM -0500, Alfred Perlstein wrote:
>>> [[ reply private ]]
>>>
>>> First of all, thank you very much for taking the time to explain this.
>>>
>>> I've forwarded your response to a few people who are more
>>> knowledgeable about Subversion in our community (John Baldwin /
>>> Peter Wemm) to hopefully get back us on the issue of mergeinfo and
>>> reintegrate.
>> Thanks!
>>
>>> Regarding the patch: 
>>> http://subversion.tigris.org/issues/show_bug.cgi?id=890
>>> I just read this.  What can I do to help on this issue?
>> See the mailing list thread linked from
>> http://subversion.tigris.org/issues/show_bug.cgi?id=890#desc40
>> That's the last time the patch got reviewed. It needs to be updated
>> to Subversion's trunk (to-be-1.8) before it can be committed.
>>
>


Re: PATCH: keyword expansion 1.8

Posted by Alfred Perlstein <br...@mu.org>.
On 4/10/13 11:05 AM, Stefan Sperling wrote:
> On Tue, Feb 12, 2013 at 08:42:29PM +0100, Stefan Sperling wrote:
>> Thanks! Are you willing to spend time on providing an updated patch?
> I've taken the time to wrap this up myself today, see:
> http://svn.apache.org/r1466570
> http://svn.apache.org/r1466598
>
> So hopefully keyword expansion for svn.freebsd.org will work fine
> with a stock Subversion 1.8 client.
>
> It seems to work fine in my testing. I'd be very happy about test
> reports from the FreeBSD side with an svn client compiled from
> Subversion's trunk as of r1466598.
>
> Thanks for contributing this patch!
>
Thank you, this is very exciting.

-Alfred

Re: PATCH: keyword expansion 1.8

Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 12, 2013 at 08:42:29PM +0100, Stefan Sperling wrote:
> Thanks! Are you willing to spend time on providing an updated patch?

I've taken the time to wrap this up myself today, see:
http://svn.apache.org/r1466570
http://svn.apache.org/r1466598

So hopefully keyword expansion for svn.freebsd.org will work fine
with a stock Subversion 1.8 client.

It seems to work fine in my testing. I'd be very happy about test
reports from the FreeBSD side with an svn client compiled from
Subversion's trunk as of r1466598.

Thanks for contributing this patch!

Re: PATCH: keyword expansion 1.8

Posted by Stefan Sperling <st...@elego.de>.
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));

Re: PATCH: keyword expansion 1.8

Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Mon, Feb 11, 2013 at 12:36:29 +0100:
> On Mon, Feb 11, 2013 at 09:38:05AM +0000, Philip Martin wrote:
> > Stefan Sperling <st...@elego.de> writes:
> > >> +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);
> > >> +
> > 
> > Our private API uses a double underscore svn_subst__build_keywords3
> > however this is replacing a public API svn_subst_build_keywords2
> > declared in include/svn_subst.h.
> > 
> > Is the intention to deprecate the entire svn_subst_build_keywords public
> > API?  Or is svn_subst_build_keywords3 intended to be the new public API?
> 
> I believe I can explain this, given the history of this patch.
> 
> This was a patch to the FreeBSD port, and I suppose they made the new
> APIs they added in that patch private because it wasn't part of the
> official public API. So, yes, this API should be public now that the
> intention is to add it to the upstream code base.

Yes, that's correct.  They used to declare the API in
subversion/include/, but the declaration has been moved to
subversion/include/private/ since it wasn't part of our (upstream's)
public API:

http://www.freebsd.org/cgi/query-pr.cgi?pr=165311

Re: PATCH: keyword expansion 1.8

Posted by Stefan Sperling <st...@elego.de>.
On Mon, Feb 11, 2013 at 09:38:05AM +0000, Philip Martin wrote:
> Stefan Sperling <st...@elego.de> writes:
> 
> >> 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 @@
> 
> >> + * @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
> 
> 1.6 is wrong.

Indeed. It should say "1.8" instead.

> >> + */
> >> +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);
> >> +
> 
> Our private API uses a double underscore svn_subst__build_keywords3
> however this is replacing a public API svn_subst_build_keywords2
> declared in include/svn_subst.h.
> 
> Is the intention to deprecate the entire svn_subst_build_keywords public
> API?  Or is svn_subst_build_keywords3 intended to be the new public API?

I believe I can explain this, given the history of this patch.

This was a patch to the FreeBSD port, and I suppose they made the new
APIs they added in that patch private because it wasn't part of the
official public API. So, yes, this API should be public now that the
intention is to add it to the upstream code base.

I don't think Alfred knows about the subtleties of our API versioning
guidelines yet. Nor should he have to -- after all, that's why we're
reviewing the patch and are pointing this out.

Re: PATCH: keyword expansion 1.8

Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:

>> 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 @@

>> + * @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

1.6 is wrong.

>> + */
>> +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);
>> +

Our private API uses a double underscore svn_subst__build_keywords3
however this is replacing a public API svn_subst_build_keywords2
declared in include/svn_subst.h.

Is the intention to deprecate the entire svn_subst_build_keywords public
API?  Or is svn_subst_build_keywords3 intended to be the new public API?

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: PATCH: keyword expansion 1.8

Posted by Stefan Sperling <st...@elego.de>.
On Sat, Feb 09, 2013 at 07:12:38AM -0800, Alfred Perlstein wrote:
> [[ Adding users@subversion.apache.org ]]
> 
> He folks.  I am in no way a Subversion expert, however per request
> I've taken the FreeBSD patch for adding custom keyword expansion and
> forward ported it to what is soon to be Subversion 1.8.
> 
> I am wondering if anyone can help me test this or give me some
> things to try to ensure that it works properly.
> 
> -Alfred

Hi Alfred,

Patches should go to dev@, not users@.
I've changed the Cc to the right list in this reply.

I'll try to review this during the coming week. Thanks!

> On 2/1/13 7:40 AM, Alfred Perlstein wrote:
> >[[ bringing in Lev Serebryakov, FreeBSD port maintainer for Subversion ]]
> >
> >Hello Lev,
> >
> >Stefan of the Subversion project asked me about bringing in our
> >keywords patch for subversion 1.8.
> >
> >I've taken a bit of time and gotten the patch to apply to
> >subversion trunk (untested).
> >
> >Can you have a look and let us know what we need to do with
> >regards to testing or anything else?
> >
> >
> >Stefan, any suggestions on going forward?
> >
> >-Alfred
> >
> >On 2/1/13 8:34 AM, Stefan Sperling wrote:
> >>On Fri, Feb 01, 2013 at 08:01:02AM -0500, Alfred Perlstein wrote:
> >>>[[ reply private ]]
> >>>
> >>>First of all, thank you very much for taking the time to explain this.
> >>>
> >>>I've forwarded your response to a few people who are more
> >>>knowledgeable about Subversion in our community (John Baldwin /
> >>>Peter Wemm) to hopefully get back us on the issue of mergeinfo and
> >>>reintegrate.
> >>Thanks!
> >>
> >>>Regarding the patch:
> >>>http://subversion.tigris.org/issues/show_bug.cgi?id=890
> >>>I just read this.  What can I do to help on this issue?
> >>See the mailing list thread linked from
> >>http://subversion.tigris.org/issues/show_bug.cgi?id=890#desc40
> >>That's the last time the patch got reviewed. It needs to be updated
> >>to Subversion's trunk (to-be-1.8) before it can be committed.
> >>
> >
> 

> 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);
> +
> +
> +
> +#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;
>  
>        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,
> +					  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;
>    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,
>                     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"
>  
> 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;
>        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 */,
> +	      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;
>    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;
>    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 %
>   *
>   * 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,
>                 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;
>    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;
> +	    }
> +	  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);
> +          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";
> +          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));