sl0ppty: say what the config could not honour

A config that half-applied said nothing. An unknown shader, a binding
that does not parse, an include that is not there: the file loaded, that
line did not, and the session announced "config reloaded" as though
nothing had happened. Every one of those messages was already being
produced -- config_load has filled `err` on a successful load since it
was written -- and then dropped on the floor by every caller, because a
non-zero return meant "fine".

So the complaint from the last load is kept, and the front end says it
where it would have said "config reloaded". Once at startup too: a config
that loaded with a complaint still started the session, and the stderr
line goes where nobody attached can read it. The `reload` control verb
answers `{"ok":true,"warning":"..."}` -- ok, because the file did apply,
with the reason attached for a script that cares.

Two things fell out of writing the test for it.

An include's complaint was reported by nobody when the file in the middle
was fine. apply_includes only looked at the return value, so a theme that
loaded while *its* include was missing swallowed the message: the child
returned true, and true meant there was nothing to say. The complaint now
travels up whether or not the include itself loaded.

And a complaint now carries the name of the file it happened in, exactly
once -- added by the file that produced it, never by the files that
included it, because prefixing at every level builds a chain that
truncates before the part that matters. So:

    t.kdl: unknown shader: bloom
    keys.kdl: line 2: unknown action fly
    b.kdl: includes nested too deep (a cycle?)
    cannot open /home/you/.config/sl0ppty/themes/nope.kdl

which is enough to know which file to open, in a message that fits in a
toast.

8 files changed, 110 insertions, 13 deletions

diff --git a/README.md b/README.md
index d6f10f1..ee826c4 100644
--- a/README.md
+++ b/README.md
@@ -153,7 +153,9 @@ opens your config in a pane, writing that starting file first if you have
 none. [`config/config.kdl`](config/config.kdl) is the same list with the
 reasoning attached, if you want to know *why* a setting exists. **Saving
 it applies it to every running session immediately** — and a file with a
-mistake in it is refused rather than half-applied.
+mistake in it is refused rather than half-applied. Anything smaller than that —
+an unknown shader, a binding that does not parse, an include that is not there —
+applies the rest of the file and tells you which line it could not honour.
 
 Geometry, key bindings and every colour are configurable. Six ready-made
 themes are in [`contrib/themes`](contrib/themes), and a config can be made of
diff --git a/config/config.kdl b/config/config.kdl
index 7e242c9..1c2bd88 100644
--- a/config/config.kdl
+++ b/config/config.kdl
@@ -62,8 +62,11 @@
 // or `states` block replaces the one it inherited, like every one of those.
 //
 // A file that will not load is skipped and the rest of the config still
-// applies. Losing your keybindings over a mistyped theme name would be the
-// worse answer.
+// applies -- losing your keybindings over a mistyped theme name would be the
+// worse answer -- and the session says which file it could not honour, in the
+// same place it says "config reloaded". That goes for anything a config
+// complains about: an unknown shader, a binding that does not parse, an include
+// that is not there. The file applied; one line of it did not; you get told.
 
 // ── geometry ────────────────────────────────────────────────────────────────
 
diff --git a/src/app.c b/src/app.c
index 9c33601..fefb2ba 100644
--- a/src/app.c
+++ b/src/app.c
@@ -26,6 +26,13 @@
  * look; `reload` re-reads it in place, which is why nothing caches a colour. */
 static config_t CFG;
 static bool CFG_LOADED = false;
+/* What the config in force complained about while loading, or "". A complaint
+ * is not a failure: an include that is not there, a shader nobody has heard of,
+ * a binding that does not parse — the rest of the file applied and the session
+ * is running (D9). But dropping it on the floor is how a mistyped theme name
+ * turns into ten minutes of wondering, so it is kept for whoever can say it out
+ * loud. The front end toasts it; the log gets it either way. */
+static char CFG_COMPLAINT[256];
 
 static void ensure_config(void) {
   if (CFG_LOADED) return;
@@ -35,14 +42,23 @@ static void ensure_config(void) {
   if (!config_load(&CFG, path, err, sizeof err)) {
     /* A missing file is the normal case; a broken one is worth a line in the
      * log, and in both the compiled-in defaults stand (fail open). */
-    if (access(path, R_OK) == 0)
+    if (access(path, R_OK) == 0) {
       fprintf(stderr, "sl0ppty: %s: %s\n", path, err[0] ? err : "parse error");
+      snprintf(CFG_COMPLAINT, sizeof CFG_COMPLAINT, "%s",
+               err[0] ? err : "config parse error");
+    }
   } else if (err[0]) {
     fprintf(stderr, "sl0ppty: %s: %s\n", path, err);
+    snprintf(CFG_COMPLAINT, sizeof CFG_COMPLAINT, "%s", err);
   }
   CFG_LOADED = true;
 }
 
+const char *app_config_complaint(void) {
+  ensure_config();
+  return CFG_COMPLAINT;
+}
+
 bool app_reload_config(char *err, size_t errcap) {
   config_t fresh;
   config_defaults(&fresh);
@@ -54,6 +70,8 @@ bool app_reload_config(char *err, size_t errcap) {
   config_free(&CFG);
   CFG = fresh;
   CFG_LOADED = true;
+  /* Whatever the new file had to say about itself, including nothing. */
+  snprintf(CFG_COMPLAINT, sizeof CFG_COMPLAINT, "%s", err && err[0] ? err : "");
   return true;
 }
 
diff --git a/src/app.h b/src/app.h
index 9e6c820..31587a5 100644
--- a/src/app.h
+++ b/src/app.h
@@ -42,6 +42,10 @@ bool app_reload_config(char *err, size_t errcap);
 /* The files the config in force was read from: the one that was loaded and
  * everything it included, existing or not. What the watcher watches. */
 size_t app_config_files(const char **out, size_t max);
+/* What the config in force complained about while loading, or "". A complaint is
+ * not a failure -- an include that is not there, a shader nobody has heard of --
+ * so the session is running either way; this is for saying so. */
+const char *app_config_complaint(void);
 void app_resize(app_t *a, uint16_t cols, uint16_t rows);
 /* The attached client's cell size in pixels, passed on to every pane's pty and
  * terminal. Images need it: a placement that does not say how many cells it
diff --git a/src/cmd.c b/src/cmd.c
index 758586b..5c3792a 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -278,7 +278,20 @@ static char *cmd_json(app_t *a, screen_t *s, input_parser_t *in,
     }
     app_resize(a, s->cols, s->rows); /* geometry may have moved */
     s->force_full = true;
-    app_toast(a, "config reloaded");
+    /* A complaint is not a failure: the file applied and one line of it did
+     * not, so the reply is ok with a reason attached rather than an error. A
+     * script that cares can look; the session says it out loud either way. */
+    const char *why = app_config_complaint();
+    app_toast(a, why && *why ? why : "config reloaded");
+    if (why && *why) {
+      json_t j;
+      json_init(&j);
+      json_obj_open(&j, NULL);
+      json_bool(&j, "ok", true);
+      json_str(&j, "warning", why, strlen(why));
+      json_obj_close(&j);
+      return j.buf;
+    }
     return jok_int(NULL, 0);
   }
   if (strcmp(cmd, "alive") == 0) {
diff --git a/src/config.c b/src/config.c
index a31496f..a6c63a9 100644
--- a/src/config.c
+++ b/src/config.c
@@ -1018,7 +1018,12 @@ static void include_path(const char *base_file, const char *ref, char *out,
  *
  * A file that will not load is a line and no more (D9): the rest of the config
  * still applies, because losing your keybindings over a mistyped theme name
- * would be a worse answer than a session that says so. */
+ * would be a worse answer than a session that says so.
+ *
+ * The complaint travels up whether or not the include itself loaded. A file that
+ * applied while *its* include did not still has something to say, and the first
+ * version only looked at the return value — so a bad file two levels down was
+ * reported by nobody, because the file in the middle had parsed fine. */
 static void apply_includes(config_t *c, const kdl_node_t *root, const char *path,
                            int depth, char *err, size_t errcap) {
   for (size_t i = 0; i < root->nkids; i++) {
@@ -1034,10 +1039,9 @@ static void apply_includes(config_t *c, const kdl_node_t *root, const char *path
       /* Its own buffer: an include's failure must not overwrite a message the
        * file that included it has already produced. */
       char ierr[256] = {0};
-      if (!load_into(c, resolved, depth + 1, ierr, sizeof ierr) && err &&
-          !err[0])
-        snprintf(err, errcap, "line %d: %s", n->line,
-                 ierr[0] ? ierr : "cannot include it");
+      if (!load_into(c, resolved, depth + 1, ierr, sizeof ierr) && !ierr[0])
+        snprintf(ierr, sizeof ierr, "cannot include it");
+      if (ierr[0] && err && !err[0]) snprintf(err, errcap, "%s", ierr);
     }
   }
 }
@@ -1064,11 +1068,20 @@ size_t config_files(const config_t *c, const char **out, size_t max) {
   return n;
 }
 
+/* The name to put in front of a complaint: the file it happened in, without its
+ * directory. Enough to know which file to open when several are involved, and
+ * short enough to fit in a toast. */
+static const char *file_label(const char *path) {
+  const char *slash = strrchr(path, '/');
+  return slash ? slash + 1 : path;
+}
+
 static bool load_into(config_t *c, const char *path, int depth, char *err,
                       size_t errcap) {
   if (depth > INCLUDE_MAX_DEPTH) {
     if (err && errcap)
-      snprintf(err, errcap, "%s: includes nested too deep (a cycle?)", path);
+      snprintf(err, errcap, "%s: includes nested too deep (a cycle?)",
+               file_label(path));
     return false;
   }
   remember_file(c, path);
@@ -1077,6 +1090,13 @@ static bool load_into(config_t *c, const char *path, int depth, char *err,
 
   apply_includes(c, root, path, depth, err, errcap);
 
+  /* Whether the complaint below, if any, came from further down. What this file
+   * has to say about itself gets this file's name in front of it; what an
+   * include had to say already carries the name of the file it happened in, and
+   * prefixing it again would build a chain that truncates before the part that
+   * matters. */
+  bool inherited = err && err[0];
+
   c->gap = (uint16_t)kdl_arg_int(kdl_child(root, "gap"), 0, c->gap);
   c->gap_aspect =
       (uint16_t)kdl_arg_int(kdl_child(root, "gap_aspect"), 0, c->gap_aspect);
@@ -1299,5 +1319,11 @@ static bool load_into(config_t *c, const char *path, int depth, char *err,
   }
 
   kdl_free(root);
+
+  if (!inherited && err && err[0]) {
+    char own[256];
+    snprintf(own, sizeof own, "%s: %s", file_label(path), err);
+    snprintf(err, errcap, "%s", own);
+  }
   return true;
 }
diff --git a/src/server.c b/src/server.c
index 0420ffe..59b3c9c 100644
--- a/src/server.c
+++ b/src/server.c
@@ -275,6 +275,14 @@ int server_run(const char *name, const char *const argv[], uint16_t cols,
 
   app_compose(s.app, &s.screen); /* a click resolves against a painted frame */
 
+  /* A config that loaded with a complaint in it still started the session, so
+   * the complaint has nowhere to go but here. Once, at startup: the log line is
+   * written where nobody attached will see it. */
+  {
+    const char *why = app_config_complaint();
+    if (why && *why) app_toast(s.app, why);
+  }
+
   /* Watch the config's *directory*, not the file. Editors overwhelmingly save
    * by writing a temporary file and renaming it over the target, which swaps
    * the inode out from under a watch on the file itself: it fires once and
@@ -477,7 +485,11 @@ int server_run(const char *name, const char *const argv[], uint16_t cols,
         /* The set of files can have changed with the config that named them:
          * an include added, pointed somewhere else, or taken out. */
         watch_config(&watches);
-        app_toast(s.app, "config reloaded");
+        /* A complaint rather than "config reloaded": the file applied, and one
+         * line of it did not. Saying only the good half is how a mistyped
+         * include becomes ten minutes of wondering. */
+        const char *why = app_config_complaint();
+        app_toast(s.app, why && *why ? why : "config reloaded");
       } else {
         app_toast(s.app, err[0] ? err : "config reload failed");
       }
diff --git a/tests/test_include.py b/tests/test_include.py
index 42baa89..b2f5200 100755
--- a/tests/test_include.py
+++ b/tests/test_include.py
@@ -123,7 +123,8 @@ def test_anything_can_be_included_not_only_a_theme():
 
 def test_a_missing_include_costs_a_line_and_no_more():
     """Losing your keybindings over a mistyped theme name would be a worse
-    answer than a session that carries on without the theme (D9)."""
+    answer than a session that carries on without the theme (D9) -- but silence
+    would be its own kind of wrong, so it is a line and not nothing."""
     root = tree({"config.kdl": 'include "themes/nope.kdl"\ngap 2\n'})
     with Session(SH, cols=60, rows=14, config=root + "/config.kdl") as s:
         s.settle()
@@ -131,6 +132,23 @@ def test_a_missing_include_costs_a_line_and_no_more():
         check("and the rest of the file applied anyway", s.pane()["x"] == 4,
               str(s.pane()))
 
+        reply = s.api("reload")
+        check("the reload succeeds", reply.get("ok"), str(reply))
+        check("...and says which line it could not honour",
+              "nope.kdl" in reply.get("warning", ""), str(reply))
+        check("the session says it out loud too",
+              "nope.kdl" in s.snapshot().screen(),
+              repr(s.snapshot().screen()[-200:]))
+
+
+def test_a_cycle_says_so():
+    root = tree({"a.kdl": 'include "b.kdl"\n', "b.kdl": 'include "a.kdl"\n'})
+    with Session(SH, cols=70, rows=14, config=root + "/a.kdl") as s:
+        s.settle()
+        reply = s.api("reload")
+        check("a cycle is reported rather than followed",
+              "too deep" in reply.get("warning", ""), str(reply))
+
 
 def test_a_cycle_stops_rather_than_spinning():
     """A config that includes itself is a mistake, and the depth limit is what
@@ -195,6 +213,7 @@ if __name__ == "__main__":
     test_a_leading_tilde_is_a_home_directory()
     test_anything_can_be_included_not_only_a_theme()
     test_a_missing_include_costs_a_line_and_no_more()
+    test_a_cycle_says_so()
     test_a_cycle_stops_rather_than_spinning()
     test_an_include_is_picked_up_by_a_reload()
     test_every_contrib_theme_can_be_included()