Raise the cognitive-complexity threshold to 30 and clear the rest

.clang-tidy

@@ -110,7 +110,7 @@ CheckOptions:
   - key:             readability-implicit-bool-conversion.AllowPointerConditions
     value:           '1'
   - key:             readability-function-cognitive-complexity.Threshold
-    value:           '25'
+    value:           '30'
   - key:             readability-braces-around-statements.ShortStatementLines
     value:           '1'
   - key:             readability-identifier-length.IgnoredLoopCounterNames

mbo/diff/impl/diff_myers.cc

@@ -181,6 +181,10 @@ void DiffMyers::Loop() {
   }
 }
 
+// The forward/reverse D-path search is one algorithm from Myers' paper, and the bookkeeping (the two
+// diagonal windows, the parity rule and the overlap test) only makes sense read together. Splitting
+// it into helpers would hide the correspondence to the paper without making any part simpler.
+// NOLINTNEXTLINE(readability-function-cognitive-complexity)
 DiffMyers::Snake DiffMyers::FindMiddleSnake(const Span& span) {
   // The span was trimmed: both sides are non-empty and neither the first nor
   // the last lines match, so the minimal cost is >= 2 and the first overlap

mbo/diff/internal/output.cc

@@ -74,6 +74,10 @@ void AppendUnified(
   }
 }
 
+// As with AppendSideBySide: the branching enumerates the context format's cases (change blocks
+// showing '!' on both sides versus lone '-' / '+' runs), which is the specification this function
+// implements rather than incidental complexity.
+// NOLINTNEXTLINE(readability-function-cognitive-complexity)
 void AppendContext(
     std::string& output,
     const DiffOptions& options,
@@ -185,6 +189,11 @@ void AppendNormal(
   }
 }
 
+// The branching is the side-by-side format itself: the four row kinds and their gutter markers,
+// plus padding and truncation per cell. The emitted text is the contract these functions exist to
+// produce, so splitting on a complexity metric would trade a readable format description for
+// helpers that can only be understood together.
+// NOLINTNEXTLINE(readability-function-cognitive-complexity)
 void AppendSideBySide(
     std::string& output,
     const DiffOptions& options,

mbo/mope/mope.cc

@@ -437,6 +437,9 @@ std::pair<std::size_t, std::size_t> Template::MaybeExpandWhiteSpace(
                                                                  : std::make_pair(tag_pos, tag.start.length());
 }
 
+// A single scan over the output that dispatches on each tag kind it meets; at 32 it is barely over
+// the threshold, and the tag handling reads as one sequence rather than as separable steps.
+// NOLINTNEXTLINE(readability-function-cognitive-complexity)
 absl::Status Template::ExpandInternal(Context& ctx, std::string& output) const {
   std::string_view pos = output;
   while (true) {

mbo/mope/mope_main.cc

@@ -61,32 +61,44 @@ struct Options {
 };
 
 namespace {
+
+// Applies a single `--set=<key>=<value>` argument. The key may be a ':' separated section path, in
+// which case the value is set on the addressed (possibly nested) section; without a path it becomes
+// a global context value. No path component may be empty.
+absl::Status ApplySetFlag(
+    std::string_view set_kv,
+    mbo::mope::Template& mope_template,
+    absl::flat_hash_map<std::string, std::string>& context_data) {
+  static constexpr std::string_view kEmptyPartError =
+      "No part of the key in `--set=<key>=<value>` may be empty if split by ':'.";
+  const auto [names, value] = std::pair<std::string_view, std::string_view>(absl::StrSplit(set_kv, '='));
+  std::vector<std::string_view> section_names = absl::StrSplit(names, ':');
+  const std::string_view key = section_names.back();
+  if (key.empty()) {
+    return absl::InvalidArgumentError(kEmptyPartError);
+  }
+  section_names.pop_back();
+  if (section_names.size() == 1 && section_names[0].empty()) {
+    context_data[key].assign(value);  // Global context_data
+    return absl::OkStatus();
+  }
+  auto* section = &mope_template;
+  for (const std::string_view section_name : section_names) {
+    if (section_name.empty()) {
+      return absl::InvalidArgumentError(kEmptyPartError);
+    }
+    MBO_ASSIGN_OR_RETURN(section, section->AddSection(section_name));
+  }
+  return section->SetValue(key, value);
+}
+
 absl::Status Process(const Options& opts) {
   auto input = mbo::file::Artefact::Read(opts.template_name);
   mbo::mope::Template mope_template;
   // Add `--set` flag values.
   absl::flat_hash_map<std::string, std::string> context_data;
   for (const auto& set_kv : absl::GetFlag(FLAGS_set)) {
-    const auto [names, value] = std::pair<std::string_view, std::string_view>(absl::StrSplit(set_kv, '='));
-    std::vector<std::string_view> section_names = absl::StrSplit(names, ':');
-    std::string_view key = section_names.back();
-    if (key.empty()) {
-      return absl::InvalidArgumentError("No part of the key in `--set=<key>=<value>` may be empty if split by ':'.");
-    }
-    section_names.pop_back();
-    if (section_names.size() == 1 && section_names[0].empty()) {
-      context_data[key].assign(value);  // Global context_data
-    } else {
-      auto* section = &mope_template;
-      for (std::string_view section_name : section_names) {
-        if (section_name.empty()) {
-          return absl::InvalidArgumentError(
-              "No part of the key in `--set=<key>=<value>` may be empty if split by ':'.");
-        }
-        MBO_ASSIGN_OR_RETURN(section, section->AddSection(section_name));
-      }
-      MBO_RETURN_IF_ERROR(section->SetValue(key, value));
-    }
+    MBO_RETURN_IF_ERROR(ApplySetFlag(set_kv, mope_template, context_data));
   }
   // Read `--ini` file if present.
   const std::string ini_filename = absl::GetFlag(FLAGS_ini);

tools/clang_tidy.sh

@@ -109,7 +109,35 @@ done
 # Nothing to check (pre-commit may invoke with no matching files).
 [ "${#}" -gt 0 ] || exit 0
 
+# Checks that carry no meaning in test code, disabled for `*_test.cc` only. Doing
+# it here rather than with a NOLINT per test keeps one statement of the rule
+# instead of a comment repeated on every test, and new tests are covered without
+# anyone remembering to annotate them.
+#   * readability-function-cognitive-complexity: a gtest TestBody's score comes
+#     from ASSERT_*/EXPECT_* macros expanding to branches, not from logic that
+#     could be refactored. Test bodies reached 122 against a threshold of 30.
+# `--checks` APPENDS to the `Checks` in .clang-tidy (it does not replace it), so
+# every other check still applies to tests.
+readonly TEST_DISABLED_CHECKS='-readability-function-cognitive-complexity'
+
+declare -a SOURCES=()
+declare -a TESTS=()
+for FILE in "${@}"; do
+  case "${FILE}" in
+    *_test.cc | *_test.cpp | *_test.cxx) TESTS+=("${FILE}") ;;
+    *) SOURCES+=("${FILE}") ;;
+  esac
+done
+
 # Report only: --header-filter restricts diagnostics to this repo's own headers
 # (not the toolchain's force-included / system headers), -p points at the compile
 # DB. WarningsAsErrors in .clang-tidy makes any finding a non-zero exit.
-exec "${CLANG_TIDY}" --header-filter='(^|/)mbo/' -p . "${@}"
+# Both groups must run, and a finding in either has to fail, so no `exec` here.
+STATUS=0
+if [ "${#SOURCES[@]}" -gt 0 ]; then
+  "${CLANG_TIDY}" --header-filter='(^|/)mbo/' -p . "${SOURCES[@]}" || STATUS=1
+fi
+if [ "${#TESTS[@]}" -gt 0 ]; then
+  "${CLANG_TIDY}" --header-filter='(^|/)mbo/' --checks="${TEST_DISABLED_CHECKS}" -p . "${TESTS[@]}" || STATUS=1
+fi
+exit "${STATUS}"