From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from scc-mailout.scc.kit.edu (scc-mailout.scc.kit.edu [129.13.185.202]) by krisdoz.my.domain (8.14.5/8.14.5) with ESMTP id pBP0cJcX025129 for ; Sat, 24 Dec 2011 19:38:19 -0500 (EST) Received: from hekate.usta.de (asta-nat.asta.uni-karlsruhe.de [172.22.63.82]) by scc-mailout-02.scc.kit.edu with esmtp (Exim 4.72 #1) id 1Rec6T-0001vJ-9m; Sun, 25 Dec 2011 01:38:17 +0100 Received: from donnerwolke.usta.de ([172.24.96.3]) by hekate.usta.de with esmtp (Exim 4.72) (envelope-from ) id 1Rec6T-0006y8-7H for tech@mdocml.bsd.lv; Sun, 25 Dec 2011 01:38:17 +0100 Received: from iris.usta.de ([172.24.96.5] helo=usta.de) by donnerwolke.usta.de with esmtp (Exim 4.72) (envelope-from ) id 1Rec6T-0006MK-69 for tech@mdocml.bsd.lv; Sun, 25 Dec 2011 01:38:17 +0100 Received: from schwarze by usta.de with local (Exim 4.72) (envelope-from ) id 1Rec6S-0007iI-Rm for tech@mdocml.bsd.lv; Sun, 25 Dec 2011 01:38:16 +0100 Date: Sun, 25 Dec 2011 01:38:16 +0100 From: Ingo Schwarze To: tech@mdocml.bsd.lv Subject: Re: implement mandocdb -t Message-ID: <20111225003816.GI15148@iris.usta.de> References: <20111224164653.GD15148@iris.usta.de> <4EF65E2F.5070009@bsd.lv> X-Mailinglist: mdocml-tech Reply-To: tech@mdocml.bsd.lv MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4EF65E2F.5070009@bsd.lv> User-Agent: Mutt/1.5.21 (2010-09-15) Hi Kristaps, thanks for the quick review! Kristaps Dzonsons wrote on Sun, Dec 25, 2011 at 01:20:15AM +0200: > I have a few issues with this patch -- not the intent or code, but > rather with the consistency of the verbosity and warnings. > > First, for consistency, all warnings/errors should follow "CAUSE: > REASON" -- including those in the getopt() switch. Thus, > "conflicting options" messages should be prefixed by the offending > switch case and "Too many arguments" should begin with "-C". Makes sense to me; fixed. > Second, verbosity. I think we should settle this now. BSD.lv only > has a single `-v' and I think this should stay as such ("%s: added > to index" or "%s: removed from index" (or whatever the message is)). > This is completely clear and is also valuable to the user. Noting > keywords added or removed is unnecessary when you consider that > mandocdb(8) isn't there to debug the keywords of individual manuals: > it's there to index them. Indeed, i didn't put dumping of keywords back. I'm ok with having only a single -v, both in the documentation and in the long term. For now, i'd like to keep the "scanning" and "scheduling" messages around, they help tremendously for debugging, but can be removed when the dust has settled. > Regarding errors, the existences of (and not reasons for) mandoc(3) > parse errors are printed to stderr ("parse failed"). For catpages, > I think it's unnecessary to print anything more than the same. > After all, it's not like the operator can do anything about the > page, no? Just give the same error regardless the reason. Maybe an > extra kick like "%s: parse failed (not a manpage?)". Well, mandocdb -t is not for the operator, but for the porter, and for the porter, the message should be as specific as possible, because a diligent porter may wish to fix the manuals and send patches upstream. > Moving on to the warnings (wrong sections, architectures, etc.). > This shouldn't be a `-v', as this isn't a verbose message, it's a > warning. Maybe we can pull in `-W' from mandoc(1)? I don't think > these should be printed by default, as they don't affect parse > status. Definitely, because printing them by default would clobber the output of pkg_add(1), and espie@ would not be amused. > An extra "warning" flag is both consistent with other > utilities and doesn't confuse the utility of `-v'. Interesting idea. Right now, makewhatis(8) doesn't have a -W flag. The only use of -W i can find is in Linux man(1): -W [Linux man] list the pathnames of matching formatted pages which is similar to man -w, just for formatted manuals. I don't think we have to respect that. > Another option is to print the warnings by default, but use `-q' to > silence them. No way: pkg_add(1)! > But that's also confusing because one `-q' silences > warnings, but `-qq' silences parse errors also? Ugh... Ugh indeed. > If we follow this logic, then `-t' is much clearer, as it does > nothing but run in `add' mode without changing the database. (Isn't > this usually termed `-n', as with make(1)? Sigh.) Well, both OpenBSD makewhatis(8) and Linux mandb(8) call it -t, so i'd like to stick with that. > However, the `-W' flag (or whatever it becomes) could be turned on > by default just like `-Tlint' turns on `-Wall' in mandoc(1). Indeed, -t without -W doesn't make much sense. Besides, -t has to imply -W because otherwise, we would break established behaviour of makewhatis -t. > `-v' would be unaffected and meaningless as no files are actually > added or removed. > > Thoughts? Lots of useful input, see below for a patch to be applied on top of the previous one, for easier review. OK in that revised form? Ingo --- mandocdb.8.orig +++ mandocdb.8 @@ -22,21 +22,20 @@ .Nd index UNIX manuals .Sh SYNOPSIS .Nm -.Op Fl avvv +.Op Fl avW .Op Fl C Ar file .Nm -.Op Fl avvv +.Op Fl avW .Ar dir ... .Nm -.Op Fl vvv +.Op Fl vW .Fl d Ar dir .Op Ar .Nm -.Op Fl vvv +.Op Fl vW .Fl u Ar dir .Op Ar .Nm -.Op Fl vv .Fl t Ar .Sh DESCRIPTION The @@ -98,8 +97,8 @@ for potential problems. No databases are modified. Implies .Fl a -and one -.Fl v . +and +.Fl W . All diagnostic messages are printed to the standard output; the standard error output is not used. .It Fl u Ar dir @@ -109,19 +108,10 @@ from the database in .Ar dir without truncating it. .It Fl v -Verbose operation; can be specified multiple times. -By default, -.Nm -operates silently. -One -.Fl v -warns about potential problems. -A second -.Fl v -displays all files added to or removed from the index. -A third -.Fl v -displays additional debugging information. +Display all files added or removed to the index. +.It Fl W +Print warnings about potential problems with manual pages +to the standard error output. .El .Pp If fatal parse errors are encountered while parsing, the offending file --- mandocdb.c.orig +++ mandocdb.c @@ -267,6 +267,7 @@ static const struct mdoc_handler mdocs[MDOC_MAX] = { static const char *progname; static int use_all; /* Use all directories and files. */ static int verb; /* Output verbosity level. */ +static int warnings; /* Potential problems in manuals. */ int mandocdb(int argc, char *argv[]) @@ -299,22 +300,24 @@ mandocdb(int argc, char *argv[]) memset(&mdb, 0, sizeof(struct mdb)); memset(&recs, 0, sizeof(struct recs)); - verb = 0; use_all = 0; + verb = 0; + warnings = 0; of = NULL; mp = NULL; hash = NULL; op = OP_DEFAULT; dir = NULL; - while (-1 != (ch = getopt(argc, argv, "aC:d:tu:v"))) + while (-1 != (ch = getopt(argc, argv, "aC:d:tu:vW"))) switch (ch) { case ('a'): use_all = 1; break; case ('C'): if (op) { - fprintf(stderr, "conflicting options\n"); + fprintf(stderr, + "-C: conflicting options\n"); goto usage; } dir = optarg; @@ -322,7 +325,8 @@ mandocdb(int argc, char *argv[]) break; case ('d'): if (op) { - fprintf(stderr, "conflicting options\n"); + fprintf(stderr, + "-d: conflicting options\n"); goto usage; } dir = optarg; @@ -331,16 +335,18 @@ mandocdb(int argc, char *argv[]) case ('t'): dup2(STDOUT_FILENO, STDERR_FILENO); if (op) { - fprintf(stderr, "conflicting options\n"); + fprintf(stderr, + "-t: conflicting options\n"); goto usage; } op = OP_TEST; use_all = 1; - verb++; + warnings = 1; break; case ('u'): if (op) { - fprintf(stderr, "conflicting options\n"); + fprintf(stderr, + "-u: conflicting options\n"); goto usage; } dir = optarg; @@ -349,6 +355,9 @@ mandocdb(int argc, char *argv[]) case ('v'): verb++; break; + case ('W'): + warnings = 1; + break; default: goto usage; } @@ -357,7 +366,7 @@ mandocdb(int argc, char *argv[]) argv += optind; if (OP_CONFFILE == op && argc > 0) { - fprintf(stderr, "too many arguments for -C\n"); + fprintf(stderr, "-C: too many arguments\n"); goto usage; } @@ -489,11 +498,6 @@ mandocdb(int argc, char *argv[]) exit((int)MANDOCLEVEL_SYSERR); } - if (verb > 2) { - printf("%s: truncated\n", mdb.dbn); - printf("%s: truncated\n", mdb.idxn); - } - ofile_free(of); of = NULL; @@ -610,7 +614,7 @@ index_merge(const struct of *of, struct mparse *mp, assert(of->sec); assert(msec); if (strcasecmp(msec, of->sec)) { - if (verb) + if (warnings) fprintf(stderr, "%s: " "section \"%s\" manual " "in \"%s\" directory\n", @@ -621,7 +625,7 @@ index_merge(const struct of *of, struct mparse *mp, assert(of->arch); assert(march); if (strcasecmp(march, of->arch)) { - if (verb) + if (warnings) fprintf(stderr, "%s: " "architecture \"%s\" manual " "in \"%s\" directory\n", @@ -639,7 +643,7 @@ index_merge(const struct of *of, struct mparse *mp, assert(of->title); assert(mtitle); if (strcasecmp(mtitle, of->title)) { - if (verb) + if (warnings) fprintf(stderr, "%s: " "title \"%s\" in file " "but \"%s\" in filename\n", @@ -737,7 +741,7 @@ index_merge(const struct of *of, struct mparse *mp, val.data = dbuf->cp; val.size = dbuf->len; - if (verb > 1) + if (verb) printf("%s: adding to index\n", fn); dbt_put(mdb->idx, mdb->idxn, &key, &val); @@ -825,7 +829,7 @@ index_prune(const struct of *ofile, struct mdb *mdb, struct recs *recs) exit((int)MANDOCLEVEL_SYSERR); } - if (verb > 1) + if (verb) printf("%s: deleting from index\n", fn); val.size = 0; @@ -1344,7 +1348,7 @@ pformatted(DB *hash, struct buf *buf, struct buf *dbuf, size_t len, plen; if (NULL == (stream = fopen(of->fname, "r"))) { - if (verb) + if (warnings) perror(of->fname); return; } @@ -1384,7 +1388,7 @@ pformatted(DB *hash, struct buf *buf, struct buf *dbuf, line = fgetln(stream, &len); if (NULL == line || ' ' != *line || '\n' != line[(int)len - 1]) { - if (verb) + if (warnings) fprintf(stderr, "%s: cannot find NAME section\n", of->fname); buf_appendb(dbuf, buf->cp, buf->size); @@ -1405,7 +1409,7 @@ pformatted(DB *hash, struct buf *buf, struct buf *dbuf, for (p += 2; ' ' == *p || '\b' == *p; p++) /* Skip to next word. */ ; } else { - if (verb) + if (warnings) fprintf(stderr, "%s: no dash in title line\n", of->fname); p = line; @@ -1485,7 +1489,7 @@ ofile_argbuild(int argc, char *argv[], struct of **of) break; } if ('\0' == *title) { - if (verb) + if (warnings) fprintf(stderr, "%s: cannot deduce title " "from filename\n", @@ -1508,7 +1512,7 @@ ofile_argbuild(int argc, char *argv[], struct of **of) * Add the structure to the list. */ - if (verb > 2) + if (verb > 1) printf("%s: scheduling\n", argv[i]); if (NULL == *of) { *of = nof; @@ -1543,7 +1547,7 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, int src_form; if (NULL == (d = opendir(dir))) { - if (verb) + if (warnings) perror(dir); return; } @@ -1574,8 +1578,9 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, src_form |= MANDOC_FORM; sec = fn + 3; } else { - if (verb) fprintf(stderr, "%s/%s: " - "bad section\n", dir, fn); + if (warnings) fprintf(stderr, + "%s/%s: bad section\n", + dir, fn); if (use_all) sec = fn; else @@ -1583,14 +1588,15 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, } } else if ('\0' == *arch) { if (NULL != strchr(fn, '.')) { - if (verb) fprintf(stderr, "%s/%s: " - "bad architecture\n", dir, fn); + if (warnings) fprintf(stderr, + "%s/%s: bad architecture\n", + dir, fn); if (0 == use_all) continue; } arch = fn; } else { - if (verb) fprintf(stderr, "%s/%s: " + if (warnings) fprintf(stderr, "%s/%s: " "excessive subdirectory\n", dir, fn); if (0 == use_all) continue; @@ -1602,12 +1608,12 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, sz = strlcat(buf, fn, MAXPATHLEN); if (MAXPATHLEN <= sz) { - if (verb) fprintf(stderr, "%s/%s: " + if (warnings) fprintf(stderr, "%s/%s: " "path too long\n", dir, fn); continue; } - if (verb > 2) + if (verb > 1) printf("%s: scanning\n", buf); ofile_dirbuild(buf, sec, arch, src_form, of); @@ -1615,7 +1621,7 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, } if (DT_REG != dp->d_type) { - if (verb) + if (warnings) fprintf(stderr, "%s/%s: not a regular file\n", dir, fn); @@ -1624,7 +1630,7 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, if (!strcmp(MANDOC_DB, fn) || !strcmp(MANDOC_IDX, fn)) continue; if ('\0' == *psec) { - if (verb) + if (warnings) fprintf(stderr, "%s/%s: file outside section\n", dir, fn); @@ -1640,7 +1646,7 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, suffix = strrchr(fn, '.'); if (NULL == suffix) { - if (verb) + if (warnings) fprintf(stderr, "%s/%s: no filename suffix\n", dir, fn); @@ -1650,7 +1656,7 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, strcmp(suffix + 1, psec)) || (MANDOC_FORM & src_form && strcmp(suffix + 1, "0"))) { - if (verb) + if (warnings) fprintf(stderr, "%s/%s: wrong filename suffix\n", dir, fn); @@ -1687,8 +1693,9 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, strlcat(buf, "/", MAXPATHLEN); sz = strlcat(buf, fn, MAXPATHLEN); if (sz >= MAXPATHLEN) { - if (verb) fprintf(stderr, "%s/%s: " - "path too long\n", dir, fn); + if (warnings) fprintf(stderr, + "%s/%s: path too long\n", + dir, fn); continue; } q = strrchr(buf, '.'); @@ -1696,8 +1703,9 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, *q = '\0'; sz = strlcat(buf, psec, MAXPATHLEN); if (sz >= MAXPATHLEN) { - if (verb) fprintf(stderr, "%s/%s: " - "path too long\n", dir, fn); + if (warnings) fprintf(stderr, + "%s/%s: path too long\n", + dir, fn); continue; } if (0 == access(buf, R_OK)) @@ -1713,7 +1721,8 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, } sz = strlcat(buf, fn, MAXPATHLEN); if (sz >= MAXPATHLEN) { - fprintf(stderr, "%s/%s: path too long\n", dir, fn); + if (warnings) fprintf(stderr, + "%s/%s: path too long\n", dir, fn); continue; } @@ -1736,7 +1745,7 @@ ofile_dirbuild(const char *dir, const char* psec, const char *parch, * Add the structure to the list. */ - if (verb > 2) + if (verb > 1) printf("%s: scheduling\n", buf); if (NULL == *of) { *of = nof; -- To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv