On Thu, 2008-06-26 at 11:37 +0200, Stefan Sperling wrote:
> On Thu, Jun 26, 2008 at 01:48:23AM -0700, Peter Wemm wrote:
> > The custom keywords change was done with an eye towards submitting it.
> > I was inspired by the patch in the issue database, but there were a
> > number of serious problems with it. And I wanted something truely
> > flexible.
> >
> > FWIW, the version we're running right now is here:
> > http://people.freebsd.org/~peter/svn_keyword.diff
> > Unlike some of our other hacks, this one is non invasive. It doesn't
> > affect svn's operation unless the features are activated via
> > properties.
>
> The patch says it was for issue #860, but you meant #890, right?
>
> "Implementation of Custom keywords"
> http://subversion.tigris.org/issues/show_bug.cgi?id=890
>
> Would you mind if I added the URL to the patch to issue #890?
> Your patch is certainly worth tracking, even if you don't think
> it's ready for submission yet.
I'm sure Peter won't mind it being linked from issue #890, so I've done
that.
Peter's patch looks pretty sane to me. It appears to do several simple
things:
1. Support composed keyword values. Expand the "svn:keywords" property
syntax so that each keyword name can optionally be followed by an equals
sign and a format string that defines the expansion of that keyword,
composed from static text and replaceable parameters that expand to the
bits of information that are available through keyword expansion. The
default expansion (where no equals sign is given) of an existing keyword
matches its existing meaning.
2. Support some new replaceable parameters. The existing and new
parameters (new ones marked with "+") are, from keyword_printf() in
subversion/libsvn_subr/subst.c:
* %a author of this revision
* %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 %
3. Support arbitrary keyword names. Any name within the dollar signs
syntax will be recognised as a keyword and expanded as defined in (1),
if it is listed in the "svn:keywords" property. The default expansion of
a custom keyword is empty.
4. Add some aliases for format strings. Where the format string (after
an equals sign in svn:keywords) is "%H" or "%I", interpret it as:
* "%H" => "%P %r %d %a" (like CVS's $CVSHeader$)
* "%I" => "%b %r %d %a" (like $Id$)
To specify these expansions in full in svn:keywords you'd have to write
them with the spaces replaced by %_, like "%P%_%r%_%d%_%a".
Peter says in the patch file:
> # http://people.freebsd.org/~peter/svn_keyword.diff
> #
> # This patch is an extension of patch in issue #860.
> # Instead of aliasing one keyword to another, and a big bug (only
> # supports aliasing to $Id$ anyway), this version allows
> # keyword_printf() format strings.
I couldn't see what previous patch that was referring to, but I'm not sure it matters.
> # svn propset svn:keywords 'Foo=%b Bar=%r' file (expands $Foo: filename $, etc)
> # %I is an alias for $id$-like expansion. I've added %_ to insert a space.
> # eg: 'CustomId=%b%_Some%_Random%_String'. The svn:keywords parser doesn't
> # allow plain spaces.
> #
> # it adds to keyword_printf():
> # %P - $CVSHeader$ like repository-relative paths.
> # %_ - a space
> # %R - root of repository
Those seem absolutely fine.
> # and as aliases in the keyword builder:
> # %H - alias for $CVSHeader$ expansion. (%P %r %d %a)
> # %I - alias for $Id$ expansion. (%b %r %d %a)
> # the keyword builder aliases are because the svn:keyword splitter
> # uses spaces.
I don't quite follow the reasoning. As you have added "%_" to mean a
space, these two aliases "%H" and "%I" don't seem necessary. Are they
just conveniences to avoid having to write something like "MyKeyword=%P%
_%r%_%d%_%a" ?
> #
> # I had to hack the API to support this. If there is a better way
> # to do it, please let me know. I'm sure I've done everything wrong.
If you mean revising the APIs and deprecating the old versions, that
looks perfect.
> # freebsd.org uses this patch, relative to 1.5, and sets
> # svn:keywords to "FreeBSD=%H'. This expands thus:
> # $FreeBSD: stable/7/bin/ls/ls.c 157160 2006-03-24 17:09:03Z jhb $
> # $FreeBSD: head/bin/ls/ls.c 177989 2008-04-04 03:57:46Z grog $
> # This works the same way as $CVSHeader$ in gnu-cvs.
I have lightly reviewed this patch and don't see any major problems. I
haven't looked at the edge cases and exception handling (like what if
there is a dollar sign in the keyword expansion value, and so on). I did
notice that the aliases "%H" and "%I" are restrictive in that you can't
use them within a longer expression such as "%R%_%I".
I don't see any particular backward compatibility problems or any major
departure from existing practice.
Given a log message, which I could write myself if persuaded to do so,
and a slightly more thorough review, I would suggest we accept and
commit parts (1) (2) and (3) of this.
Any other thoughts?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-07-02 22:40:06 CEST