# Readability Review — darcsweb
## Summary
The codebase is mostly readable once you get down to individual helpers: comments are better than average, the tests are fairly direct, and the security-sensitive path checks are usually explained. The main friction is structural, not algorithmic: `app/Main.hs` and `src/DarcsWeb/Html.hs` carry too many responsibilities in single files, while a few dense idioms force the reader to mentally decompile otherwise simple logic. The generated `gen/*.hs` modules are predictably noisy, but the handwritten wrappers do not always hide that noise well enough.
## Confusing constructs
- `app/Main.hs:74` uses `foldl (flip id) defaultOpts o`, which hides a simple "apply parsed option updates to defaults" step behind a classic but opaque idiom. Plain rewrite: `foldl (\opts applyOpt -> applyOpt opts) defaultOpts o` or a named helper such as `applyParsedOptions`.
- `app/Main.hs:257`, `app/Main.hs:258`, `app/Main.hs:299`, `app/Main.hs:300`, `app/Main.hs:313`, `app/Main.hs:314` pull regex captures out with `pathParam "1"` and `pathParam "2"`. Plain rewrite: wrap each regex route in a helper that returns named values like `(repoName, subPath)` so readers do not have to map numbers back to the regex.
- `app/Main.hs:451` defines `decodeHdr = (>>= eitherToMaybe . TE.decodeUtf8') . flip lookup hdrs`, which compresses a straightforward lookup-and-decode path into point-free composition. Plain rewrite: `decodeHeader name = case lookup name hdrs of ...`.
- `src/DarcsWeb/Config.hs:24`, `src/DarcsWeb/Config.hs:26`, `src/DarcsWeb/Config.hs:33` make the tiny config parser read like a puzzle, with a guarded `head` and a `reverse . dropWhile ... . reverse` trim. Plain rewrite: pattern-match on the stripped line and use `dropWhileEnd isSpace`.
- `src/DarcsWeb/Darcs.hs:177`, `src/DarcsWeb/Darcs.hs:180`, `src/DarcsWeb/Darcs.hs:181` encode the jail check with nested `if`s plus `last` and `init`, so the exact-directory case is hard to spot. Plain rewrite: bind `jailRoot` once and test `canonical == jailRoot || addTrailingSlash jailRoot \`isPrefixOf\` canonical`.
- `src/DarcsWeb/Html.hs:369`, `src/DarcsWeb/Html.hs:370`, `src/DarcsWeb/Html.hs:371` rely on `init` and `last` after `T.splitOn "/" subPath`; the invariant may be true, but it is invisible at the use site. Plain rewrite: pattern-match on the segment list once and bind `dirParts` and `fileName` explicitly.
- `test/Properties/Csp.hs:38` uses `result !! length name` just to assert that a space follows the directive name. Plain rewrite: `case drop (length name) result of ' ':_ -> True; _ -> False`.
## Naming
- `src/DarcsWeb/Config.hs:14` and `app/Main.hs:94` use `CfgMap` and `cfgMap` next to `DarcsWebConfig`. Suggested names: `ConfigMap` and `configMap`, so the difference between raw parsed key-value data and validated runtime config is obvious.
- `app/Main.hs:239` and `src/DarcsWeb/Darcs.hs:81` use `mRepoInfo` and `mInfo`. Suggested names: `maybeRepoInfo` and `maybeBasicInfo`.
- `src/DarcsWeb/Darcs.hs:255` and `src/DarcsWeb/Darcs.hs:265` use `piap`, which is hard to decode even with the type on screen. Suggested names: `patchInfoAnd` or `patchWithInfo`.
- `src/DarcsWeb/Html.hs:78`, `src/DarcsWeb/Html.hs:163`, `src/DarcsWeb/Html.hs:193`, `src/DarcsWeb/Html.hs:217` use `ri` and `ps` throughout the largest handwritten module. Suggested names: `repo` and `patch`, so the rendering code reads like the page it produces.
## Structure
- `app/Main.hs:85` through `app/Main.hs:120` pack argument parsing, config loading, path canonicalisation, logger selection, daemonisation, and server startup into one `main`. A smaller top-level flow such as `loadSettings`, `mkLogger`, and `startServer` would fit on one screen.
- `app/Main.hs:217` through `app/Main.hs:342` register every route inline and repeat the same `withRepo` / load / render-404 pattern. Splitting this into route groups plus a tiny `respondNotFound` helper would make the route table scanable.
- `app/Main.hs:344` through `app/Main.hs:406` and `app/Main.hs:419` through `app/Main.hs:440` repeat the same "canonicalise, jail-check, existence-check, then 404" structure across static files, repos, and clone files. A shared helper would turn three nested `if` ladders into declarative routing code.
- `src/DarcsWeb/Html.hs:31` through `src/DarcsWeb/Html.hs:467` mix page layout, repo summary pages, tree/blob views, breadcrumb construction, diff highlighting, and date/author formatting in one module. Splitting by responsibility would let a reader change one kind of output without paging through the entire renderer.
- `src/DarcsWeb/Html.hs:297` through `src/DarcsWeb/Html.hs:308` and `src/DarcsWeb/Html.hs:367` through `src/DarcsWeb/Html.hs:381` duplicate nearly the same breadcrumb builder. One shared helper would remove a lot of local recursion noise.
- `src/DarcsWeb/Darcs.hs:121` through `src/DarcsWeb/Darcs.hs:164` and `src/DarcsWeb/Darcs.hs:196` through `src/DarcsWeb/Darcs.hs:250` repeat "run Darcs action and swallow exceptions" plus deep validation ladders. Small helpers like `runRepoJobOr` and `guardRepoPath` would flatten the module.
## Comments & docs
- `app/Main.hs:4` has no module header describing the overall request flow or where path/jail invariants are enforced. That context matters because `Main` is the operational entrypoint and one of the largest reader-facing modules.
- `src/DarcsWeb/Darcs.hs:1` and `src/DarcsWeb/Darcs.hs:7` would benefit from a top-of-file note explaining why the Darcs integration needs advanced extensions and where the boundary lies between Darcs-library access, plain filesystem reads, and Coq-extracted path checks.
- `src/DarcsWeb/Html.hs:3` should say up front that it intentionally hand-builds HTML `Text` and relies on `esc` for safety. That design choice shapes how every function in the module should be read.
- `src/DarcsWeb/Types.hs:1` has field comments, but no module or type-level note explaining why `PatchSummary` carries `psLog`, `psSummary`, and `psDiff`, or why `RepoInfo` stores a filesystem path. A short overview would help new readers orient themselves.
- `test/Spec.hs:1` needs a brief note that the test runner intentionally performs proof verification before QuickCheck. That sequencing is project-specific and not obvious from the filename alone.
## Small wins
- `app/Main.hs:1`, `app/Main.hs:169`, `app/Main.hs:171` bring raw FFI into the executable entrypoint just to open `/dev/null`. If the project can use `System.Posix.IO.openFd`, that would remove the `ForeignFunctionInterface` extension entirely; if not, move the FFI to a tiny helper module so `Main` stays at the HTTP/application level.
- `src/DarcsWeb/Darcs.hs:69` is a top-level binding without a signature. Adding `checkRepo :: FilePath -> String -> IO [RepoInfo]` would match the otherwise explicit style.
- `src/DarcsWeb/Html.hs:347`, `src/DarcsWeb/Html.hs:350`, `src/DarcsWeb/Html.hs:430`, `src/DarcsWeb/Html.hs:441` use raw size/time constants (`1024`, `1048576`, `604800`, `2419200`). Naming them as `kib`, `mib`, `week`, and `fourWeeks` would make policy read as policy instead of arithmetic.
- `src/DarcsWeb/Config.hs:39` should use `fromMaybe def` instead of `maybe def id`; it reads as data lookup rather than a mini combinator exercise.
- `src/DarcsWeb/Html.hs:109` should use `not (null recentPatches)` instead of `length recentPatches > 0`; the latter makes the reader wonder whether the exact length matters.
- `app/Main.hs:21` is one of the few non-explicit imports in the handwritten modules. Matching the surrounding explicit import style would make `Main` easier to skim.
## Extracted code note
- `gen/HtmlPure.hs:1`, `gen/CspPure.hs:1`, and `gen/PathPure.hs:1` are clearly extraction artefacts: warnings are globally suppressed, helper names are mechanical, and simple logic expands into case-heavy recursion. These are extracted and cannot be edited directly; the right fix is to keep them quarantined behind well-named handwritten wrappers.
- `gen/PathPure.hs:70`, `gen/PathPure.hs:92`, and `gen/PathPure.hs:99` encode important path rules in dense boolean nests. Because the source of truth is the verified Coq/Rocq side, the readable improvement belongs there, or in better wrapper-side documentation at `src/DarcsWeb/Darcs.hs:185` and `app/Main.hs:410`, not in hand-cleaning the generated Haskell.
- `gen/HtmlPure.hs:16` and `gen/CspPure.hs:33` expose low-level generated names directly to Haskell readers. A thin handwritten wrapper layer with domain names like `escapeHtml` and `buildCspHeader` would keep the extraction naming scheme out of the rest of the codebase.
