You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David O'Brien <ob...@FreeBSD.org> on 2010/09/22 14:52:46 UTC
Subversion issue 890 [custom keywords]
Hi folks,
I am wondering if issue 890 has fallen thru the cracks.
http://subversion.tigris.org/issues/show_bug.cgi?id=890
I know Peter Wemm said he needs some more time to work on the patch at
http://svn.haxx.se/dev/archive-2008-07/0148.shtml, but the current
implementation we use at FreeBSD.org have been deployed in the wild for
over 2 years.
Peter is no longer maintaining the FreeBSD Subversion port, so I do not
expect anything different from him than we have at
ftp://ftp.FreeBSD.org/pub/FreeBSD/ports/local-distfiles/lev/svn_hacks_1.4.diff
This is the 4th iteration of this patchset (which includes an
implementation to address issue 890) and has been trouble free.
Are there specific issues with the custom keyword implementation in
svn_hacks_1.4.diff? Is this implementation ready to be committed to
Subversion? Is a diff needed specifically against
http://svn.apache.org/repos/asf/subversion?
thanks!
--
-- David (obrien@FreeBSD.org)
Re: Subversion issue 890 [custom keywords]
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Wed, Sep 22, 2010 at 20:07:52 +0200:
> On Wed, Sep 22, 2010 at 07:52:46AM -0700, David O'Brien wrote:
> > + case '_': /* '%_' => a space */
> > + svn_stringbuf_appendbytes(value, " ", 1);
> > + break;
>
> Hmmm, no tabs here...
>
> > case '%': /* '%%' => a literal % */
> > svn_stringbuf_appendbytes(value, cur, 1);
> > break;
This could use svn_stringbuf_appendbyte().
Re: Subversion issue 890 [custom keywords]
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Wed, Sep 22, 2010 at 20:07:52 +0200:
> On Wed, Sep 22, 2010 at 07:52:46AM -0700, David O'Brien wrote:
> > + case '_': /* '%_' => a space */
> > + svn_stringbuf_appendbytes(value, " ", 1);
> > + break;
>
> Hmmm, no tabs here...
>
> > case '%': /* '%%' => a literal % */
> > svn_stringbuf_appendbytes(value, cur, 1);
> > break;
This could use svn_stringbuf_appendbyte().
Re: Subversion issue 890 [custom keywords]
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Oct 05, 2010 at 11:45:18AM -0700, David O'Brien wrote:
> On Wed, Sep 22, 2010 at 08:07:52PM +0200, Stefan Sperling wrote:
> > On Wed, Sep 22, 2010 at 07:52:46AM -0700, David O'Brien wrote:
> > > http://subversion.tigris.org/issues/show_bug.cgi?id=890
> ..
> > However, the patch needs some work. I suppose it has not been synced
> > to Subversion trunk yet, so that will need to be done.
> > Would you be willing to do this?
>
> Hi Stefan,
> Yes, I will look into doing that. As you mention below, the patch is
> still using the 1.6 API and I don't believe anyone has looked at what
> it will take to use the 1.7 API.
>
> Thank you for pointing out various things about the APIs used in the
> patch. That is a big help.
Ok, I'll wait for an updated patch, then. Thank you!
> > > diff -ruN subversion/libsvn_wc/merge.c subversion/libsvn_wc/merge.c
> > > --- subversion/libsvn_wc/merge.c 2009-02-16 23:35:25.000000000 +0300
> > > +++ subversion/libsvn_wc/merge.c 2009-04-03 22:11:01.307500000 +0400
> > > @@ -369,7 +369,7 @@
> > > target_marker,
> > > right_marker,
> > > "=======", /* separator */
> > > - svn_diff_conflict_display_modified_latest,
> > > + svn_diff_conflict_display_modified_original_latest,
> >
> > The above change looks unrelated to issue #890.
>
> Correct. Should an issue be raised for this?
I don't know if there already is an issue related to custom
conflict markers. If you cannot find one, feel free to file one.
> The API support various conflict markers, but I can find no way to
> configure the svn client on which to use. Thus we have to patch the
> code directly as we've chose "Perforce"-style markers as being most
> useful to our community (both FreeBSD and $WORK).
>
> I wonder if with 1.7 it wouldn't be a good point to change the default?
I'd rather add configuration options than changing the default.
> > > + * Since we're adding freebsd-specific tokens to the log message,
> > > + * clean out any leftovers to avoid accidently sending them to other
> > > + * projects that won't be expecting them.
> > > + */
> > > +
> > > +#define NPREFIX 7
> > > +char *prefixes[NPREFIX] = {
> > > + "PR:",
> ..
> > Your log message template changes are interesting but unrelated to issue #890.
> > The related issue would be:
> > http://subversion.tigris.org/issues/show_bug.cgi?id=1973
>
> Correct -- it is my understanding this issue is already being worked on
> for a future version of Subversion. I fully acknowledge this change is
> not generic and not suitable for inclusion in the stock Subversion.
The request is being tracked, but that doesn't imply that anyone is
working on it. I don't know if anyone is actively working on this.
> I have had to make a similar change at $WORK as many folks depend on the
> template to remind them the various markers we put in our log messages.
>
> So the feature is a highly desired one from the two communities I am
> most active in.
Feel free to work towards a more generic solution, if you have the time
to spare.
I suppose it would help if a design discussion was started on this
mailing list before any actual coding is done. It's important to make
sure the svn community agrees with the approach before doing any serious work.
This is usually low overhead if the design proposal is well thought out.
Usually the design, no matter how good it already is, benefits a lot from
community discussion. Also, you can regard developers involved in the
discussion as potential candidates for reviewing related patches :)
Thanks,
Stefan
Re: Subversion issue 890 [custom keywords]
Posted by David O'Brien <ob...@FreeBSD.org>.
On Wed, Sep 22, 2010 at 08:07:52PM +0200, Stefan Sperling wrote:
> On Wed, Sep 22, 2010 at 07:52:46AM -0700, David O'Brien wrote:
> > http://subversion.tigris.org/issues/show_bug.cgi?id=890
..
> However, the patch needs some work. I suppose it has not been synced
> to Subversion trunk yet, so that will need to be done.
> Would you be willing to do this?
Hi Stefan,
Yes, I will look into doing that. As you mention below, the patch is
still using the 1.6 API and I don't believe anyone has looked at what
it will take to use the 1.7 API.
Thank you for pointing out various things about the APIs used in the
patch. That is a big help.
> > diff -ruN subversion/libsvn_wc/merge.c subversion/libsvn_wc/merge.c
> > --- subversion/libsvn_wc/merge.c 2009-02-16 23:35:25.000000000 +0300
> > +++ subversion/libsvn_wc/merge.c 2009-04-03 22:11:01.307500000 +0400
> > @@ -369,7 +369,7 @@
> > target_marker,
> > right_marker,
> > "=======", /* separator */
> > - svn_diff_conflict_display_modified_latest,
> > + svn_diff_conflict_display_modified_original_latest,
>
> The above change looks unrelated to issue #890.
Correct. Should an issue be raised for this?
The API support various conflict markers, but I can find no way to
configure the svn client on which to use. Thus we have to patch the
code directly as we've chose "Perforce"-style markers as being most
useful to our community (both FreeBSD and $WORK).
I wonder if with 1.7 it wouldn't be a good point to change the default?
> > + * Since we're adding freebsd-specific tokens to the log message,
> > + * clean out any leftovers to avoid accidently sending them to other
> > + * projects that won't be expecting them.
> > + */
> > +
> > +#define NPREFIX 7
> > +char *prefixes[NPREFIX] = {
> > + "PR:",
..
> Your log message template changes are interesting but unrelated to issue #890.
> The related issue would be:
> http://subversion.tigris.org/issues/show_bug.cgi?id=1973
Correct -- it is my understanding this issue is already being worked on
for a future version of Subversion. I fully acknowledge this change is
not generic and not suitable for inclusion in the stock Subversion.
I have had to make a similar change at $WORK as many folks depend on the
template to remind them the various markers we put in our log messages.
So the feature is a highly desired one from the two communities I am
most active in.
--
-- David (obrien@FreeBSD.org)
Re: Subversion issue 890 [custom keywords]
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 22, 2010 at 07:52:46AM -0700, David O'Brien wrote:
> Hi folks,
> I am wondering if issue 890 has fallen thru the cracks.
> http://subversion.tigris.org/issues/show_bug.cgi?id=890
>
> I know Peter Wemm said he needs some more time to work on the patch at
> http://svn.haxx.se/dev/archive-2008-07/0148.shtml, but the current
> implementation we use at FreeBSD.org have been deployed in the wild for
> over 2 years.
>
> Peter is no longer maintaining the FreeBSD Subversion port, so I do not
> expect anything different from him than we have at
> ftp://ftp.FreeBSD.org/pub/FreeBSD/ports/local-distfiles/lev/svn_hacks_1.4.diff
>
> This is the 4th iteration of this patchset (which includes an
> implementation to address issue 890) and has been trouble free.
>
> Are there specific issues with the custom keyword implementation in
> svn_hacks_1.4.diff? Is this implementation ready to be committed to
> Subversion? Is a diff needed specifically against
> http://svn.apache.org/repos/asf/subversion?
>
> thanks!
Hi David,
thanks for bringing this up. I've been wondering when this patch would
be submitted for another round of review. Last we heard from Peter he
said he felt the patch wasn't ready for review yet, as he was still
working on it.
I'm eager to help you get the functionality implemented in this patch
into Subversion, so FreeBSD can use Subversion as shipped from upstream.
However, the patch needs some work. I suppose it has not been synced
to Subversion trunk yet, so that will need to be done.
Would you be willing to do this?
It would also be nice to have a log message as documented here:
http://subversion.apache.org/docs/community-guide/general.html#patches
For further remarks see below for an inline review.
> diff -ruN subversion/include/svn_subst.h subversion/include/svn_subst.h
> --- subversion/include/svn_subst.h 2009-01-26 14:30:42.000000000 +0300
> +++ subversion/include/svn_subst.h 2009-04-03 21:49:30.541875000 +0400
> @@ -122,16 +122,31 @@
> * 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. Any of these can be @c NULL to indicate that the information is
> - * not present, or @c 0 for @a date.
> + * 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.3.
> + * @since New in 1.6
This needs to say "New in 1.7".
> + */
> +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);
> +
> +/** Similar to svn_subst_build_keywords3() except that it does not
> + * supply the repository location.
> + *
> + * @deprecated Provided for backward compatibility with the 1.3 API.
> */
> svn_error_t *
> svn_subst_build_keywords2(apr_hash_t **kw,
> diff -ruN subversion/libsvn_client/cat.c subversion/libsvn_client/cat.c
> --- subversion/libsvn_client/cat.c 2009-02-13 23:37:02.000000000 +0300
> +++ subversion/libsvn_client/cat.c 2009-04-03 21:49:30.557500000 +0400
> @@ -127,10 +127,10 @@
> author = entry->cmt_author;
> }
>
> - SVN_ERR(svn_subst_build_keywords2
> + SVN_ERR(svn_subst_build_keywords3
> (&kw, keywords->data,
> apr_psprintf(pool, fmt, entry->cmt_rev),
> - entry->url, tm, author, pool));
> + entry->url, entry->repos, tm, author, pool));
This uses the 1.6.x working copy 'entry' data structure.
Virtually the entire 1.6.x working copy API has been removed from trunk
as part of the working copy library rewrite. New code should use the new
API instead (for this particular case, I think svn_wc__node_get_repos_info()
declared in include/private/svn_wc_private.h is the right function to use).
Use of the old API on trunk exists only in compat code at this point,
so there are plenty of examples of the new API in our current trunk code base.
> }
>
> /* Our API contract says that OUTPUT will not be closed. The two paths
> @@ -160,6 +160,7 @@
> svn_string_t *keywords;
> apr_hash_t *props;
> const char *url;
> + const char *repos_root_url;
> svn_stream_t *output = out;
>
> /* ### Inconsistent default revision logic in this command. */
> @@ -198,6 +199,8 @@
> &url, path_or_url, NULL,
> peg_revision,
> revision, ctx, pool));
> + /* Find the repos root URL */
> + SVN_ERR(svn_ra_get_repos_root2(ra_session, &repos_root_url, pool));
This API call is still fine.
>
> /* Make sure the object isn't a directory. */
> SVN_ERR(svn_ra_check_path(ra_session, "", rev, &url_kind, pool));
> @@ -242,10 +245,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,
> url,
> + repos_root_url,
> when,
> cmt_author ? cmt_author->data : NULL,
> pool));
> diff -ruN subversion/libsvn_client/commit.c subversion/libsvn_client/commit.c
> --- subversion/libsvn_client/commit.c 2009-02-13 18:32:52.000000000 +0300
> +++ subversion/libsvn_client/commit.c 2009-04-03 21:49:30.557500000 +0400
> @@ -118,9 +118,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;
>
> diff -ruN subversion/libsvn_client/export.c subversion/libsvn_client/export.c
> --- subversion/libsvn_client/export.c 2009-01-26 14:30:42.000000000 +0300
> +++ subversion/libsvn_client/export.c 2009-04-03 21:49:30.557500000 +0400
> @@ -197,10 +197,10 @@
> author = entry->cmt_author;
> }
>
> - SVN_ERR(svn_subst_build_keywords2
> + SVN_ERR(svn_subst_build_keywords3
> (&kw, keywords->data,
> apr_psprintf(pool, fmt, entry->cmt_rev),
> - entry->url, tm, author, pool));
> + entry->url, entry->repos, tm, author, pool));
Using the obsolete 'entry' structure again, see above.
> }
>
> /* For atomicity, we translate to a tmp file and then rename the tmp file
> @@ -509,6 +509,7 @@
> /* Any keyword vals to be substituted */
> const char *revision;
> const char *url;
> + const char *repos;
> const char *author;
> apr_time_t date;
>
> @@ -625,6 +626,7 @@
> fb->edit_baton = eb;
> fb->path = full_path;
> fb->url = full_url;
> + fb->repos = eb->root_url;
> fb->pool = pool;
>
> *baton = fb;
> @@ -790,8 +792,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_translate3
> diff -ruN subversion/libsvn_subr/subst.c subversion/libsvn_subr/subst.c
> --- subversion/libsvn_subr/subst.c 2009-01-26 14:30:42.000000000 +0300
> +++ subversion/libsvn_subr/subst.c 2009-04-03 21:50:34.276250000 +0400
> @@ -127,8 +127,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.
> @@ -137,12 +140,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;
> int n;
>
> for (;;)
> @@ -196,6 +201,23 @@
> svn_stringbuf_appendcstr(value,
> svn_time_to_human_cstring(date, pool));
> break;
> + case 'P': /* relative path of this file */
> + relative = url;
The above line and several lines below use tabs.
We only use spaces, sorry.
> + if (relative && repos)
> + {
> + int len = strlen(repos);
> +
> + if (strncmp(repos, relative, len) == 0
> + && relative[len] == '/')
> + relative += len + 1;
Maybe use svn_uri_is_child(), declared in include/svn_dirent_uri.h, above?
> + }
> + if (relative)
> + svn_stringbuf_appendcstr(value, relative);
> + break;
This logic can cause 'url' to appear instead of the repository-root relative
path. Is this really desired? Maybe move the appendcstr() call below as
well as the 'relative' variable into the if (relative && repos) block?
So, in summary, the 'P' case could be simplified to something like this:
case 'P':
if (url && repos)
{
const char *repos_relpath;
repos_relpath = svn_uri_is_child(repos, url, NULL);
if (repos_relpath)
svn_stringbuf_appendcstr(value, repos_relpath);
}
break;
> + case 'R': /* root of repos */
> + if (repos)
> + svn_stringbuf_appendcstr(value, repos);
> + break;
Tabs again in lines above.
> case 'r': /* number of this revision */
> if (rev)
> svn_stringbuf_appendcstr(value, rev);
> @@ -204,6 +226,9 @@
> if (url)
> svn_stringbuf_appendcstr(value, url);
> break;
> + case '_': /* '%_' => a space */
> + svn_stringbuf_appendbytes(value, " ", 1);
> + break;
Hmmm, no tabs here...
> case '%': /* '%%' => a literal % */
> svn_stringbuf_appendbytes(value, cur, 1);
> break;
> @@ -239,8 +264,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
> @@ -279,6 +304,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);
> @@ -289,6 +329,24 @@
> 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 in 3 lines above.
#define named constants for the above strings?
Also, %H and %I aren't documented anywhere. Their effect should at least
be documented in this function's docstring.
Also, why aren't the %H and %I format strings handled by
keyword_printf() itself?
If knowing the format codes is necessary to configure custom keywords,
they should also be documented in user-facing documentation, e.g. in the
output of "svn help propset". I guess the lack of documentation updates
is the biggest issue I have with this patch as is.
> + 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))
> @@ -296,7 +354,7 @@
> {
> 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,
> @@ -309,7 +367,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,
> @@ -320,7 +378,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,
> @@ -331,7 +389,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,
> @@ -341,7 +399,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);
> @@ -350,7 +408,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);
> diff -ruN subversion/libsvn_wc/merge.c subversion/libsvn_wc/merge.c
> --- subversion/libsvn_wc/merge.c 2009-02-16 23:35:25.000000000 +0300
> +++ subversion/libsvn_wc/merge.c 2009-04-03 22:11:01.307500000 +0400
> @@ -369,7 +369,7 @@
> target_marker,
> right_marker,
> "=======", /* separator */
> - svn_diff_conflict_display_modified_latest,
> + svn_diff_conflict_display_modified_original_latest,
The above change looks unrelated to issue #890.
> pool));
> SVN_ERR(svn_stream_close(ostream));
>
> diff -ruN subversion/libsvn_wc/translate.c subversion/libsvn_wc/translate.c
> --- subversion/libsvn_wc/translate.c 2009-02-16 09:25:05.000000000 +0300
> +++ subversion/libsvn_wc/translate.c 2009-04-03 21:49:30.541875000 +0400
> @@ -284,11 +284,12 @@
>
> SVN_ERR(svn_wc__entry_versioned(&entry, path, adm_access, FALSE, pool));
>
> - SVN_ERR(svn_subst_build_keywords2(keywords,
> + SVN_ERR(svn_subst_build_keywords3(keywords,
> list,
> apr_psprintf(pool, "%ld",
> entry->cmt_rev),
> entry->url,
> + entry->repos,
> entry->cmt_date,
> entry->cmt_author,
> pool));
> diff -ruN subversion/svn/util.c subversion/svn/util.c
> --- subversion/svn/util.c 2009-02-13 19:28:37.000000000 +0300
> +++ subversion/svn/util.c 2009-04-03 21:49:30.541875000 +0400
> @@ -639,6 +639,67 @@
> }
>
>
> +/*
> + * Since we're adding freebsd-specific tokens to the log message,
> + * clean out any leftovers to avoid accidently sending them to other
> + * projects that won't be expecting them.
> + */
> +
> +#define NPREFIX 7
> +char *prefixes[NPREFIX] = {
> + "PR:",
> + "Submitted by:",
> + "Reviewed by:",
> + "Approved by:",
> + "Obtained from:",
> + "MFC after:",
> + "Security:",
> +};
Your log message template changes are interesting but unrelated to issue #890.
The related issue would be:
http://subversion.tigris.org/issues/show_bug.cgi?id=1973
If you would like a log message template feature, please submit a separate
patch. But it would need to be of a more general nature than these changes,
of course. The labels should be configurable, for instance via the client
configuration. (The repository-dictated configuration mentioned in
issue #1973 does not exist yet. Neither do inheritable properties.
So the client configuration is the best place we can put such functionality
right now, I suppose.)
If you have any further questions, just ask!
Thanks,
Stefan
> +
> +void
> +cleanmsg(apr_size_t *l, char *s)
> +{
> + int i;
> + char *pos;
> + char *kw;
> + char *p;
> + int empty;
> +
> + for (i = 0; i < NPREFIX; i++) {
> + pos = s;
> + while ((kw = strstr(pos, prefixes[i])) != NULL) {
> + /* Check to see if keyword is at start of line (or buffer) */
> + if (!(kw == s || kw[-1] == '\r' || kw[-1] == '\n')) {
> + pos = kw + 1;
> + continue;
> + }
> + p = kw + strlen(prefixes[i]);
> + empty = 1;
> + while (1) {
> + if (*p == ' ' || *p == '\t') {
> + p++;
> + continue;
> + }
> + if (*p == '\0' || *p == '\r' || *p == '\n')
> + break;
> + empty = 0;
> + break;
> + }
> + if (empty && (*p == '\r' || *p == '\n')) {
> + memmove(kw, p + 1, strlen(p + 1) + 1);
> + if (l)
> + *l -= (p + 1 - kw);
> + } else if (empty) {
> + *kw = '\0';
> + if (l)
> + *l -= (p - kw);
> + } else {
> + pos = p;
> + }
> + }
> + }
> +}
> +
> #define EDITOR_EOF_PREFIX _("--This line, and those below, will be ignored--")
>
> svn_error_t *
> @@ -654,8 +715,26 @@
>
> /* Set default message. */
> default_msg = svn_stringbuf_create(APR_EOL_STR, pool);
> + svn_stringbuf_appendcstr(default_msg, APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "PR:\t\t" APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "Submitted by:\t" APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "Reviewed by:\t" APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "Approved by:\t" APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "Obtained from:\t" APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "MFC after:\t" APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "Security:\t" APR_EOL_STR);
> svn_stringbuf_appendcstr(default_msg, EDITOR_EOF_PREFIX);
> - svn_stringbuf_appendcstr(default_msg, APR_EOL_STR APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "> Description of fields to fill in above: 76 columns --|" APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "> PR: If a GNATS PR is affected by the change." APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "> Submitted by: If someone else sent in the change." APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "> Reviewed by: If someone else reviewed your modification." APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "> Approved by: If you needed approval for this commit." APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "> Obtained from: If the change is from a third party." APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "> MFC after: N [day[s]|week[s]|month[s]]. Request a reminder email." APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "> Security: Vulnerability reference (one per line) or description." APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, "> Empty fields above will be automatically removed." APR_EOL_STR);
> + svn_stringbuf_appendcstr(default_msg, APR_EOL_STR);
>
> *tmp_file = NULL;
> if (lmb->message)
> @@ -667,6 +746,7 @@
> that follows it. */
> truncate_buffer_at_prefix(&(log_msg_buf->len), log_msg_buf->data,
> EDITOR_EOF_PREFIX);
> + cleanmsg(NULL, (char*)log_msg_buf->data);
>
> /* Make a string from a stringbuf, sharing the data allocation. */
> log_msg_str->data = log_msg_buf->data;
> @@ -786,6 +866,13 @@
> if (message)
> truncate_buffer_at_prefix(&message->len, message->data,
> EDITOR_EOF_PREFIX);
> + /*
> + * Since we're adding freebsd-specific tokens to the log message,
> + * clean out any leftovers to avoid accidently sending them to other
> + * projects that won't be expecting them.
> + */
> + if (message)
> + cleanmsg(&message->len, message->data);
>
> if (message)
> {