Re: [PATCH] 2 modules, zsh/db, zsh/gdbm, bug-fixes

[Daniel Shahaf <d.s@daniel.shahaf.name>]

Bart Schaefer wrote on Fri, 23 Jun 2017 01:36 -0700:
> Given the bug-fixes in zsh/db/gdbm, this seems worth accepting if the
> license comments are fixed.  Other opinions?

There's no documentation.  That means I can't know what the patch _does_
unless I reverse engineer the implementation, which I'm ENOTIME for.
I assume the patch adds a zsh/db module that introduces abstractions
that all (existing and future) zsh/db/* modules can share?  +1 to the
concept, but again, I haven't reviewed the concrete details.

If (user or dev) documentation needs to be written, I would prefer it
were written _before_ the patch was accepted.

Is it a problem that the module name is a path-wise prefix of another
module's name (zsh/db v. zsh/db/gdbm).  Can't think of a particular
problem, but it's unprecedented.  [meant as a statement of fact]

There's a ton of vim folding added, {{{ }}}.  We don't have it
elsewhere, I assume it's just a scalpel.  Likewise with the
whitespace (indentation) changes.

The #if 0 looks alarming.  Maybe I missed discussion of it, but I'd be
surprised if the correct response to a bug in some configurations was to
disable the function call in all configurations, without even a version
number check (or, what would be better if it is possible, a runtime
probe for the bug).  The comment should link to the bug.  In closer
look, this change looks like a functionality change of zsh/db/gdbm
that's unrelated to the addition of zsh/db (and so should be a separate
patch).

db.c has variables declared in the middle of the block.  That's a
C99-ism, and while db/gdbm.c does it, the rest of zsh does not.  Not
critical.

db.c has a couple of zwarn() that don't state the origin of the error,
I think it'd be useful to make them zwarnnam(name="zsh/db") or so.

Cheers,

Daniel