Skip to content

Harden config-dir check-ignore and trim docstrings#1725

Merged
bdraco merged 1 commit into
mainfrom
version-history-check-ignore-tidy
Jun 28, 2026
Merged

Harden config-dir check-ignore and trim docstrings#1725
bdraco merged 1 commit into
mainfrom
version-history-check-ignore-tidy

Conversation

@bdraco

@bdraco bdraco commented Jun 27, 2026

Copy link
Copy Markdown
Member

What does this implement/fix?

Addresses a late Copilot review on #1720 (it landed seconds after merge). Two small things in the version-history git adoption code; no behaviour change for normal config dirs.

Pass -- to git check-ignore in _enclosing_repo_ignores_config_dir so a config_dir whose path begins with - is read as a path rather than an option. Trim the helper docstring and the new test's docstring to terse contracts per the repo style (drop the rationale block and the issue-number cross-reference; the why lives in #1720 and its commit).

Related issue or feature (if applicable):

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Frontend coordination

  • No frontend change needed

Checklist

  • The code change is tested and works locally.
  • Pre-commit hooks pass (ruff, codespell, yaml/json/python checks).
  • Tests have been added or updated under tests/ where applicable.
  • components.index.json / definitions/components/*.json have not been hand-edited (regenerate via script/sync_components.py if a sync is needed).
  • Architecture-level changes are reflected in docs/ARCHITECTURE.md and/or docs/API.md.

Pass `--` to `git check-ignore` so a config_dir starting with `-` is
read as a path, not an option; trim the helper and test docstrings to
terse contracts per the repo style.
@github-actions github-actions Bot added the maintenance Maintenance / chores label Jun 27, 2026
@codspeed-hq

codspeed-hq Bot commented Jun 28, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 27 untouched benchmarks


Comparing version-history-check-ignore-tidy (6301666) with main (83653d9)

Open in CodSpeed

@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.54%. Comparing base (83653d9) to head (6301666).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1725   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files         227      227           
  Lines       18174    18174           
=======================================
  Hits        18092    18092           
  Misses         82       82           
Flag Coverage Δ
py3.12 99.52% <ø> (ø)
py3.14 99.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ce_builder/controllers/version_history/git_repo.py 100.00% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bdraco bdraco marked this pull request as ready for review June 28, 2026 00:01
Copilot AI review requested due to automatic review settings June 28, 2026 00:01
@esphbot

esphbot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

PR Review — Harden config-dir check-ignore and trim docstrings

Clean, correctly-scoped maintenance follow-up. Merge-ready.

  • The -- separator in git check-ignore is the right hardening: _run (git_repo.py:710) invokes git via an argv list with no shell, so passing -- before str(self.config_dir) cleanly disambiguates a config_dir path beginning with - from a git option — no injection surface, no behaviour change for normal dirs.
  • Docstring trims on both the helper (_enclosing_repo_ignores_config_dir) and the test match the repo's terse one-line-contract style; the dropped rationale and [Bug] Commits config to unrelated git repo in parent directory #1718 cross-ref correctly live in the originating PR/commit per the documented convention.
  • The inline # ``--`` so a config_dir starting with ``-`` isn't read as an option. comment justifies a non-obvious flag — exactly the case where a comment is warranted.
  • Diff matches the PR description precisely; no scope creep, coverage unchanged (line already covered by the existing nested-repo test), single maintenance label is appropriate.


Checklist

  • Command argument handling safe (no shell injection, options disambiguated)
  • Docstrings match repo style conventions
  • Diff aligns with PR description, no scope creep
  • Test coverage maintained

Automated review by Kōan (Claude) HEAD=6301666 24s

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens a small edge case in the version-history “enclosing git repo adoption” logic by ensuring git check-ignore treats the config directory as a path even when it begins with -, and aligns docstrings with the repo’s terse contract style.

Changes:

  • Pass -- to git check-ignore in _enclosing_repo_ignores_config_dir to prevent option injection/misparse for leading-dash config dir paths.
  • Trim docstrings in the helper and its test to match the repo’s “terse contracts” docstring convention.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
esphome_device_builder/controllers/version_history/git_repo.py Harden git check-ignore invocation with -- and shorten the helper docstring.
tests/controllers/version_history/test_git_repo.py Shorten the test docstring to a single-line contract.

Comment thread esphome_device_builder/controllers/version_history/git_repo.py
@bdraco bdraco merged commit 146cc8d into main Jun 28, 2026
23 checks passed
@bdraco bdraco deleted the version-history-check-ignore-tidy branch June 28, 2026 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Maintenance / chores

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants