Improve pre-push checks and correct historic mistakes []

.agents/skills/edg-changes-example/SKILL.md

@@ -141,7 +141,11 @@ Steps:
    in. `edg-exec` runs it in the dev-env container, which has `aspell`.) For
    misspellings that are really identifiers or terms, add them to an
    `//allowed_words:` line at the very top of the entry, as the failure
-   message shows; fix real misspellings instead.
+   message shows; fix real misspellings instead. `edgspelling.preprocess`
+   leaves an identifier like `getXValue` whole because it contains a
+   one-letter part. If aspell reports such an identifier whole, add the
+   whole identifier to `//allowed_words:`; adding only its parts (`get`,
+   `X`, `Value`) will not suppress the warning.
 5. Optionally preview the result without changing anything:
    `edg-collect-changes --dry-run src/Changes src/pending_changes`.
 

.agents/skills/edg-spelling-allowlist/SKILL.md

@@ -0,0 +1,126 @@
+---
+name: edg-spelling-allowlist
+description: >-
+  How the pre-push spell check (edg-check-file-changes, aspell, edgspelling)
+  decides what to report, and how to add words to the hierarchical spelling
+  allowlist (dev_tools/spelling_allowlist.json). Use when the pre-push hook
+  or edg-collect-changes --check reports misspelled words, when choosing
+  where a new allowed word belongs, or when editing the allowlist file.
+---
+
+# Spell check and the spelling allowlist
+
+## How the check works
+
+The pre-push hook (`.git-hooks/pre-push.py`) runs
+`dev_tools/bin/edg-check-file-changes` on every changed file, through
+`edg-exec` when docker is preferred.  Its spelling pass:
+
+- Runs `aspell list` on the text.  For code files (`.c`, `.cpp`, `.h`,
+  `.hpp`) only the comments are checked (extracted with `get-comments`).
+  Other files are checked whole.
+- First splits identifiers into words (`edgspelling.preprocess`): `fooBar`
+  becomes `foo Bar`.  Underscores already split words, so `foo_bar` is
+  checked as `foo` and `bar`.
+- Reports only words that are **new** relative to the old version of the
+  file, minus the allowlist words for that file's path.
+- For pending Changes entries (`*/pending_changes/*.txt`), also accepts
+  the entry's `//allowed_words:` directives and any word already in the
+  target `Changes` file (see skill **edg-changes-example**).  CI runs the
+  same check with `edg-collect-changes --check`.
+
+The hook skips some paths entirely (`EXCLUDES_PATH_PREFIXES` in
+`.git-hooks/pre-push.py`), including `AGENTS.md`, `.agents/`, and the
+allowlist itself.
+
+## The allowlist file
+
+`dev_tools/spelling_allowlist.json` is a tree of nodes.  Each node may have:
+
+| Key | Meaning |
+| --- | --- |
+| `words` | Sorted lowercase words, allowed in any capitalization |
+| `names` | Sorted words allowed only with exactly this capitalization |
+| `paths` | Object mapping one path component (directory or file name) to a child node |
+
+A file's allowed words are the union of the nodes on the way down to it, so a
+word in the root node is allowed everywhere.  The format is documented in
+`dev_tools/pylibs/edgspelling/__init__.py`.
+
+Rules that `edgspelling.Allowlist` enforces when the file loads (a bad
+file makes the check refuse to run and fails `ShippedAllowlistTests` in CI):
+
+- Each list is sorted by code point and has no duplicates.
+- `words` entries are lowercase.  Use `names` when the capitalization must
+  match (proper nouns like `EDG` or `Coverity`).
+- Only `words`, `names`, and `paths` keys are allowed.
+- Each entry is one word.  `foo_bar` or `fooBar` would never match, because
+  identifiers are split before lookup.  Add the parts instead.
+
+## Choosing where a word goes
+
+Put a word in the **narrowest node** that covers the places it's used
+often (see "Prompting is the default" below):
+
+- **Root:** terms used across the project (C++ terminology, `src`, `utf`).
+- **A directory node** (`src`, `dev_tools`, `dev_tools/pylibs/edgtest`,
+  `doc/source`, …): jargon used only there.  Common Python names like
+  `isinstance` and `pathlib`, and the pylib package names (`edgutil`, …),
+  live in `dev_tools`.  The rare uses elsewhere (`dev-init.py`,
+  `.git-hooks/`, `doc/source/conf.py`) are left to prompt.
+
+A word used in only one file almost never earns an entry.  The check
+reports only words that are new to a file, so the entry would save a prompt
+only when the word shows up in a second file.  A word used only in a
+`Changes` file never needs one, because pending entries already accept
+every word in their target `Changes` file.
+- **A file node:** a one-off term in a single file.
+
+### Prompting is the default
+
+Each allowed word is a blind spot: a typo that happens to match it is never
+reported anywhere the entry applies.  A broader node makes the blind spot
+bigger.  Being prompted again costs little, because the check reports only
+words that are new to a file.  Weigh the two:
+
+- Prefer leaving a word out and letting people be prompted.  Add it only when
+  people would otherwise be prompted for it very often (for example, a term
+  used constantly across a directory or the project).
+- When a word does earn an entry, choose the narrowest node that stops the
+  frequent prompts, even if rare uses elsewhere still prompt.  For example,
+  the Python names are in `dev_tools` rather than at the root.
+- Be most careful with short words and words that are one letter away from
+  a common English word (`len`, `str`, `dir`).  They're the likeliest to hide
+  a real typo.
+
+Don't add a real misspelling.  Fix the text.  For a Changes entry, prefer an
+`//allowed_words:` directive over a new allowlist word unless the term will
+recur.
+
+## Editing the file
+
+Keep the existing formatting (one-space JSON indent, UTF-8, non-ASCII kept as
+is).  A Python round trip keeps it exact and keeps the lists sorted:
+
+```python
+import json, pathlib
+path = pathlib.Path('dev_tools/spelling_allowlist.json')
+tree = json.loads(path.read_text(encoding = 'utf-8'))
+tree['words'] = sorted(set(tree['words']) | {'newword'})
+# For a nested node: tree['paths']['src']['words'] = ...
+path.write_text(
+  json.dumps(tree, indent = 1, ensure_ascii = False) + '\n',
+  encoding = 'utf-8'
+)
+```
+
+Then check that the file still loads, and run the tests:
+
+```sh
+PYTHONPATH=dev_tools/pylibs python3 -c 'import edgspelling; edgspelling.Allowlist()'
+PYTHONPATH=dev_tools/pylibs python3 -m unittest discover -s dev_tools/tests \
+  -p 'test_edg_check_file_changes.py'
+```
+
+To see what aspell rejects in some text:
+`printf '%s\n' word1 word2 | aspell list`.

.git-hooks/pre-push.py

@@ -40,6 +40,7 @@
 import subprocess
 import sys
 import tempfile
+import time
 from pathlib import Path
 
 # Git invokes this hook with the ambient environment, which does not
@@ -50,8 +51,36 @@
 )
 
 import edgchanges
+import edgshell
 import edgutil
 
+START_TIME = time.monotonic()
+
+# The push isn't allowed to proceed until the hook has run for this long, so
+# that there's time to read the target of the push and cancel it (e.g., with
+# Ctrl-C) before it happens.
+MIN_RUN_SECONDS = 3.0
+
+def read_push_refs():
+  '''Return the lines git supplied on standard input describing what is to be
+  pushed.  A relaunched delegate (see below) can't read them from standard
+  input, so its parent passes them in the environment.'''
+  if os.environ.get('EDG_PREPUSH_DELEGATE') == '1':
+    refs_text = os.environ.get('EDG_PREPUSH_REFS', '')
+  elif sys.stdin.isatty():
+    # Run by hand rather than by git.
+    refs_text = ''
+  else:
+    refs_text = sys.stdin.read()
+  return refs_text.splitlines()
+
+# Git supplies the remote name and location as arguments (both are the
+# location if the push doesn't use a named remote).  Read the refs before
+# standard input is replaced below.
+PUSH_REMOTE_NAME = sys.argv[1] if len(sys.argv) > 1 else None
+PUSH_REMOTE_LOCATION = sys.argv[2] if len(sys.argv) > 2 else None
+PUSH_REFS = read_push_refs()
+
 if platform.system() == 'Windows':
   # The standard input of this hook carries the push ref list supplied by
   # git, so it cannot be used to prompt the user.  When a console is
@@ -67,10 +96,15 @@
     delegate_proc = subprocess.run(
       [
         sys.executable,
-        __file__
+        __file__,
+        *sys.argv[1:]
       ],
       creationflags = subprocess.CREATE_NEW_CONSOLE,
-      env = {**os.environ, 'EDG_PREPUSH_DELEGATE': '1'}
+      env = {
+        **os.environ,
+        'EDG_PREPUSH_DELEGATE': '1',
+        'EDG_PREPUSH_REFS': '\n'.join(PUSH_REFS)
+      }
     )
     sys.exit(delegate_proc.returncode)
 else:
@@ -94,16 +128,43 @@
   'src/error_msg.txt',
   # Don't check AGENT files.
   'AGENTS.md',
-  '.agents/'
+  '.agents/',
+  # Don't check the spelling allowlist; it is made of words aspell rejects.
+  'dev_tools/spelling_allowlist.json'
 ]
 
 def _is_excluded_file_path(path_str: str) -> bool:
   return any(
     path_str.startswith(prefix) for prefix in EXCLUDES_PATH_PREFIXES
   )
 
+def pause_delegate_window() -> None:
+  '''When running as a relaunch delegate, wait for the user before the
+  standalone console window closes so that failure output can be read.'''
+  if os.environ.get('EDG_PREPUSH_DELEGATE') != '1':
+    return
+
+  try:
+    input('Push checks failed; press Enter to close this window. ')
+  except EOFError:
+    pass
+
 TOOL_PATH_GIT = shutil.which('git')
 
+def find_dev_tool(name):
+  '''Return the path of the dev tool called name on PATH, or exit with an
+  error if the environment lacks it (e.g., direnv isn't applied).'''
+  tool_path = shutil.which(name)
+  if tool_path is None:
+    print(
+      f"error: the environment is missing the required tool '{name}'; "
+      'put dev_tools/bin on PATH (e.g., via direnv) and push again.',
+      file = sys.stderr
+    )
+    pause_delegate_window()
+    sys.exit(1)
+  return tool_path
+
 def run_git(args, *, check = True, text = True):
   '''Run a git command and return the completed process.'''
   return subprocess.run(
@@ -134,6 +195,37 @@ def git_show_blob(revision_path: str) -> bytes:
     return b''
   return completed_process.stdout
 
+ZERO_OBJECT_NAME_CHARS = set('0')
+
+def describe_push_ref(line):
+  '''Return a description of one of the ref lines git supplies to the hook
+  (<local ref> <local name> <remote ref> <remote name>), or None if line
+  isn't of that form.'''
+  fields = line.split()
+  if len(fields) != 4:
+    return None
+  local_ref, local_name, remote_ref, remote_name = fields
+  if set(local_name) <= ZERO_OBJECT_NAME_CHARS:
+    return f"delete {remote_ref}"
+  description = f"{local_ref} -> {remote_ref}"
+  if set(remote_name) <= ZERO_OBJECT_NAME_CHARS:
+    description += ' (new)'
+  return description
+
+def print_push_target():
+  '''Print where the push is targeted.'''
+  if PUSH_REMOTE_NAME is None:
+    return
+  target = PUSH_REMOTE_NAME
+  if PUSH_REMOTE_LOCATION not in (None, PUSH_REMOTE_NAME):
+    target += f" ({PUSH_REMOTE_LOCATION})"
+  print(f"Pushing to {target}:\n")
+  for line in PUSH_REFS:
+    description = describe_push_ref(line)
+    if description is not None:
+      print(f"  {description}")
+  print('')
+
 class PrePushChecker:
   '''Runs EDG pre-push policy checks against new commits and changed files.'''
 
@@ -230,9 +322,20 @@ def check_changed_files(self, first_new_commit):
     remote_head = f"{first_new_commit}~1"
     print('Checking changed files:')
 
-    changed_files = git_output(
-      ['diff', '--no-commit-id', '--name-only', '-r', f"{remote_head}..HEAD"]
-    ).splitlines()
+    # Use -z so that git doesn't C-quote paths with unusual characters (e.g.,
+    # non-ASCII ones): the quoted form isn't a usable path.
+    changed_files = [
+      path for path in run_git(
+        ['diff', '--no-commit-id', '--name-only', '-r', '-z',
+         f"{remote_head}..HEAD"]
+      ).stdout.split('\0') if path
+    ]
+
+    # Binary files have no text to check, so don't extract them.  Ask git
+    # rather than sniffing the contents so that .gitattributes is honored.
+    binary_files = edgshell.list_binary_files_in_git_diff(
+      f"{remote_head}..HEAD"
+    )
 
     mono_repo_dir = edgutil.find_mono_repo_or_exit()
     containing_tmp_dir = mono_repo_dir / '.tmp'
@@ -242,6 +345,8 @@ def check_changed_files(self, first_new_commit):
       for file_path_str in changed_files:
         if _is_excluded_file_path(file_path_str):
           continue
+        if file_path_str in binary_files:
+          continue
 
         file_path = Path(file_path_str)
         named_tmp_args = {
@@ -276,7 +381,7 @@ def check_changed_files(self, first_new_commit):
       if use_docker:
         file_change_check_proc = subprocess.run(
           [
-            shutil.which('edg-exec'),
+            find_dev_tool('edg-exec'),
             '--',
             'edg-check-file-changes',
             str(input_file.relative_to(mono_repo_dir).as_posix()),
@@ -288,7 +393,7 @@ def check_changed_files(self, first_new_commit):
       else:
         file_change_check_proc = subprocess.run(
           [
-            shutil.which('edg-check-file-changes'),
+            find_dev_tool('edg-check-file-changes'),
             str(input_file),
             str(output_file)
           ],
@@ -394,6 +499,7 @@ def check_changed_files(self, first_new_commit):
 
   def run(self) -> int:
     '''Execute the full pre-push check sequence.  Return the exit status.'''
+    print_push_target()
     first_new_commit = self.check_commits()
 
     # Skip per file checks if we didn't actually detect a new commit.
@@ -402,22 +508,27 @@ def run(self) -> int:
 
     return 0 if (self.errors == 0 and not self.abort) else 1
 
-def pause_delegate_window() -> None:
-  '''When running as a relaunch delegate, wait for the user before the
-  standalone console window closes so that failure output can be read.'''
-  if os.environ.get('EDG_PREPUSH_DELEGATE') != '1':
-    return
+def wait_for_minimum_run_time() -> None:
+  '''Sleep until the hook has been running for MIN_RUN_SECONDS.'''
+  remaining = MIN_RUN_SECONDS - (time.monotonic() - START_TIME)
+  if remaining > 0:
+    print(f"Pushing in {remaining:.0f}s (Ctrl-C to cancel)...")
+    time.sleep(remaining)
 
-  try:
-    input('Push checks failed; press Enter to close this window. ')
-  except EOFError:
-    pass
+def wait_for_minimum_run_time() -> None:
+  '''Sleep until the hook has been running for MIN_RUN_SECONDS.'''
+  remaining = MIN_RUN_SECONDS - (time.monotonic() - START_TIME)
+  if remaining > 0:
+    print(f"Pushing in {remaining:.0f}s (Ctrl-C to cancel)...")
+    time.sleep(remaining)
 
 def main():
   checker = PrePushChecker()
   returncode = checker.run()
   if returncode != 0:
     pause_delegate_window()
+  else:
+    wait_for_minimum_run_time()
   sys.exit(returncode)
 
 main()