hi!
On Thu, Aug 17, 2023 at 10:59:48AM +0200, Michiel van den Heuvel wrote:
I named the command `Eval` instead of `IncludeCmd` because it seemed
clearer to me, but you're welcome to change that.
my thinking was that IncludeCmd would nicely complement a hypothetical
Include command, following the structure of the pre-existing *Cmd
commands.
the code needs no renaming, eval is nice and short.
Patch 1 was necessary to print error line numbers correctly after adding
`Eval` without modifying every line where errors are printed.
It might be worthwhile to set `cfile->error` here too, and I'll gladly
amend the patch if you agree.
yeah, i'd do that.
On the other hand, it is a formatting function...
nah, the name doesn't say so.
Patch 2 is where the actual work happens. I indeed tried to follow
`cred_from_cmd`.
Nesting `Eval` statements is disallowed. Implementing
this would certainly be doable, but I don't really see the point.
agreed.
I was working on an extra patch, which would extend the parser to
allow multiline values for slightly longer commands. [...]
- Do you see merit in this feature? I don't currently need it, but
like configuration file formats which offer possibilities to split
lines if they get too long.
theoretically yes, though in practice i'd recommend that users just call
a script, because the extra layer of quoting/escaping makes things ugly
and easy to get wrong. add it if you can't help yourself. ;)
- If so, what would you think is the better implementation? Just make
the buffer really large (what is large?) and error if a script is
longer? The current line limit is 1024. [...]
i think the static 1k buffer is just fine - that amounts to a script
with up to ~20 lines, and it seems a tad insane to inline more than
that.
- Do you agree with the syntax? This still requires backslashes even
when quoted, which I like due to the missing closing quote error
which would otherwise be hard to correctly detect.
i guess. it also has the advantage of somewhat signaling the fact of the
extra layer of quoting.
- What should the behaviour be when not quoted?
A literal newline (current implementation)?
that seems convenient, but it violates expectations about how line
continuations usually behave, regardless of whether quoted or not.
Read it as whitespace?
that would be more conventional. it would of course also mean that users
need to include `&&` (or `;`) to chain the commands, which many would
probably get wrong on first try.
the alternative would be more like a here-doc, and would warrant a
corresponding syntax. seems like mild overkill, though.
so just call a script already! ;)
Subject: [PATCH 2/2] Add Eval directive to config parser
--- a/src/config.c
+++ b/src/config.c
+void
static
+conf_print_loc( const conffile_t *cfile )
+{
+ if (cfile->eval_fp != NULL) {
i treat pointers a booleans in conditionals.
also, excess braces.
+ fprintf( stderr, "%s:%d:eval %d: ", cfile->file, cfile->line,
cfile->eval_line );
eval => included, i guess.
also, that space afterwards should be probably a colon, otherwise it's
kinda confusing and weird-looking.
on that matter, have you tested all error scenarios?
+void
should return an int (zero is error), which should lead to early
termination.
also, static.
+eval_popen( conffile_t *cfile, const char *cmd )
+{
+ if (*cmd == '+') {
+ flushn();
+ cmd++;
+ }
that makes no sense here - unlike the credential commands, this is
executed at program startup where no progress info has been printed yet.
don't forget to prune it from the docu as well.
+ if (!(cfile->eval_fp = popen( cmd, "r" ))) {
+ sys_error( "%s failed", cmd );
i think it makes no sense to print the command here, as failure here
would be a fork() failure or something like that. so just "popen" should
do.
(though i wonder how things would look like in an msys build.)
+void
as above.
+eval_pclose( conffile_t *cfile )
+{
+ int ret;
+
+ ret = pclose( cfile->eval_fp );
+ cfile->eval_fp = NULL;
+ if (ret) {
+ if (ret < 0)
+ conf_sys_error( cfile, "command failed" );
+ else if (WIFSIGNALED( ret ))
+ conf_error( cfile, "command crashed\n" );
+ else
+ conf_error( cfile, "command exited with status %d\n",
WEXITSTATUS( ret ) );
... but here it gets "interesting":
seeing the command after dequoting may be actually helpful, especially
when multi-line commands are supported. though of course this can be
ridiculously long. but i guess we'll get away with just qouting it.
here-doc syntax would make printing the command superfluous, because it
would be used verbatim anyway ... until someone actually implements
expandos (like %{Host}).
+char *
i think that should be just an int.
also, static.
+read_cline( conffile_t *cfile )
+{
+ char *ret;
+
+ ret = cfile->buf;
+ if (cfile->eval_fp) {
+ ret = fgets( cfile->buf, cfile->bufl, cfile->eval_fp );
+ cfile->eval_line++;
+ if (!ret) {
+ eval_pclose( cfile );
+ ret = cfile->buf;
+ }
+ }
+ if (!cfile->eval_fp && ret) {
+ ret = fgets( cfile->buf, cfile->bufl, cfile->fp );
+ cfile->line++;
+ }
+ return ret;
i think using early returns would make the thing less convoluted.
--- a/src/mbsync.1
+++ b/src/mbsync.1
@@ -152,6 +152,16 @@ even combinations thereof.
Mailbox names, OTOH, always use canonical path separators, which are
Unix-like forward slashes.
.
+.SS Dynamic options
why a separate section? it doesn't really fit the pattern for the *Cmd
commands.
Subject: [PATCH 1/2] Refactor printing configuration errors
--- a/src/config.c
+++ b/src/config.c
+void
+conf_error( const conffile_t *cfile, const char* fmt, ... )
second asterisk has space on the wrong side.
this repeats for several other string arguments.
but i think this is actually the only whitespace mistake you've made,
which may be a first among non-trivial external isync contributions
during my "reign". ^^
regards
_______________________________________________
isync-devel mailing list
isync-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/isync-devel
