On Sat, Dec 27, 2003 at 01:23:23PM +0100, Branko ??ibej wrote:
> >The ConfigParser module from python doesn't support this feature,
> >so there is no obvious guidance in terms of syntax. However, I
> >imagine something like Unix-shell expansion is unambiguous, relatively
> >simple and should not break many existing configuration files.
> >
> >
> We already have a syntax for expanding variables. If people really,
> really want env var expansion, I'd use the standard "%(NAME)s" syntax,
> but expand from environment variables _only_ in the [DEFAULT] section.
How do we distinguish between internal options and environment variables
as the intended target of expansion? I'm not sure if you intended to
suggest it, but all-caps is not sufficient since environment variables
can be mixed case (although this is not usually the convention).
> >Since the parser (in config_file.c) is fairly simple, I've created
> >the attached patch which implements the following rules:
> >
> > * Expansion is of the form ${ENV} where ENV is the name of the
> > environment variable being inserted.
> > * A non-existent environment variable expands as nothing (it's
> > not an error).
> This is wrong. Silently ignoring invalid entries in the config file is a
> recipe for disaster.
I guess it depends on what you mean by invalid. At least in the
Unix world, the shell silently expanding non-existent environment
variables is expected (and anticipated) behaviour. Admittedly, the
most common pattern where the behaviour is used is when testing
whether it's defined or not. Some shells also allow one to make it
an error if you attempt to expand an environment variable that
doesn't exist.
My motivation for not making it an error was related to my experiences
with the openssl configuration format. It also allows environment
variables to be imported, and it's an error if the environment
variable is not set. However this causes a lot of pain since it
means that often a heap of environment variables must be set, even
if the command you're invoking doesn't make any use of it. I can
give concrete examples, if required.
A reasonable compromise, I hope, would be to take advantage of the
lazy expansion and only make it an error if it's required during
the svn_option_get expansion but not present. To give a concrete
example, should it matter if the config file references SSL_CA_CERT
but no use of https is made?
Finally, it's worth noting that the existing expansion syntax fails
silently if an expansion is requested but the target does not exist.
(I call it silent since it simply leaves the expansion tokens
untouched.)
> >@@ -96,17 +164,27 @@
> > /* Read the first line of the value */
> > svn_stringbuf_setempty (ctx->value);
> > for (ch = getc (ctx->fd); /* last ch seen was ':' or '=' in parse_option. */
> >- ch != EOF && ch != '\n';
> >+ err == SVN_NO_ERROR && ch != EOF && ch != '\n';
> > ch = getc (ctx->fd))
> > {
> >- const char char_from_int = ch;
> >- svn_stringbuf_appendbytes (ctx->value, &char_from_int, 1);
> >+ char char_from_int;
> >+ switch(ch)
> >+ {
> >+ case '$':
> >+ /* This indicates an environment variable needs to be expanded. */
> >+ err = parse_expand_env_in_value (ctx);
> >+ break;
> >+
> >+ default:
> >+ char_from_int = ch;
> >+ svn_stringbuf_appendbytes (ctx->value, &char_from_int, 1);
> >+ }
> > }
> > /* Leading and trailing whitespace is ignored. */
> > svn_stringbuf_strip_whitespace (ctx->value);
> >
> >
> And what happens if the loop exited with an error? You should just use
> SVN_ERR above. acually, looking at this function, we could do away with
> the "err" variable completely...
If the loop exited with an error, the following strip_whitespace() call is
harmless; the buffer started off empty, it may have something in it but
since the error will be returned whatever if in the buffer will be ignored.
Admittedly it's not optimal, and may not be elegant. Prior to last Friday
I'd never looked at the subversion codebase so I apologise if I violated
some coding conventions here.
Finally, do the following rules sound acceptable for environment expansion:
* Syntax is of the form %(env_var)s.
* Only allowed in the [DEFAULT] section.
* Internal options are tried first, and over-ride a search of the
environment.
* As is currently the case, an expansion for something that doesn't
exist results in the '%(var)s' token being left untouched.
Hopefully these rules are acceptable. I don't mind coming up with a patch
that implements the change if no-one objects.
- Andrew
PS. Despite README.txt saying that lists are comma-delimited,
ssl-authority-files is semi-colon delimited.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Dec 29 14:11:05 2003