Skip to content

Don't adopt an enclosing git repo that ignores the config dir#1720

Merged
bdraco merged 1 commit into
mainfrom
decline-adopt-ignored-config-dir
Jun 27, 2026
Merged

Don't adopt an enclosing git repo that ignores the config dir#1720
bdraco merged 1 commit into
mainfrom
decline-adopt-ignored-config-dir

Conversation

@bdraco

@bdraco bdraco commented Jun 27, 2026

Copy link
Copy Markdown
Member

What does this implement/fix?

When the config dir sits inside an unrelated git checkout that gitignores it (e.g. an esphome/esphome clone with config/ in its .gitignore), version history walked up, found the parent work tree, and adopted it. That repo can never track our YAML (git ignores the dir), so no backup is ever created; meanwhile we still wrote our managed block into the parent's .git/info/exclude, mutating an unrelated repo.

This declines to adopt an enclosing repo that ignores the config dir and creates a config-local repo instead, mirroring the existing decline path for the Device Builder source checkout. Detection is a single git check-ignore of the config dir; a non-ignored subdir (the normal /config case) still adopts as before.

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
  • Companion frontend PR: esphome/device-builder-frontend#

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.

When a config dir sits inside an unrelated checkout that gitignores it,
version history adopted that parent; git could never track the YAML so
no backup was created, and we still wrote our managed block into the
unrelated repo's info/exclude. Decline to adopt when the enclosing repo
ignores the config dir and create a config-local repo instead, mirroring
the existing own-source-checkout decline.
@github-actions github-actions Bot added the bugfix Bug fix label Jun 27, 2026
@codspeed-hq

codspeed-hq Bot commented Jun 27, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 27 untouched benchmarks


Comparing decline-adopt-ignored-config-dir (8c82cdb) with main (ce7db7a)

Open in CodSpeed

@codecov

codecov Bot commented Jun 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.54%. Comparing base (ce7db7a) to head (8c82cdb).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1720   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files         227      227           
  Lines       18170    18174    +4     
=======================================
+ Hits        18088    18092    +4     
  Misses         82       82           
Flag Coverage Δ
py3.12 99.52% <100.00%> (+<0.01%) ⬆️
py3.14 99.54% <100.00%> (+<0.01%) ⬆️

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% <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 27, 2026 21:01
Copilot AI review requested due to automatic review settings June 27, 2026 21:01
@esphbot

esphbot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

PR Review — Don't adopt an enclosing git repo that ignores the config dir

Tight, correct bugfix — declines to adopt an enclosing repo that gitignores the config dir, mirroring the existing own-source decline path. Merge-ready.

Strengths:

  • Detection is a single git check-ignore -q, the simplest possible signal, and reuses the existing _run(..., check=False) harness so non-zero/error exits (returncode 128 when something is off) safely fall through to the existing adopt behavior rather than crashing.
  • Short-circuit ordering is right: _enclosing_repo_ignores_config_dir() only runs when toplevel is not None, so the normal no-repo path pays nothing, and the normal /config-is-its-own-repo case returns not-ignored and still adopts.
  • The test is the strong part: it asserts the positive outcomes (nested .git, kitchen.yaml actually committed, seed commit present) AND the critical negative — the unrelated parent's .git/info/exclude is left untouched — which is exactly the mutation [Bug] Commits config to unrelated git repo in parent directory #1718 was about.
  • The merged log message now derives the reason string, keeping both decline causes (own-source vs ignored) in one branch.

Nitpicks (non-blocking):

  • _encloses_own_source(toplevel) is evaluated twice (condition + reason string), but it's a pure path comparison with no subprocess, so the cost is nil.

  • No correctness, security, or coverage concerns. Codecov reports 100% on the changed lines.



Checklist

  • Correctness of new decline condition
  • Error/edge handling (check-ignore non-zero exits)
  • Test coverage incl. parent-repo-untouched assertion
  • No command injection / unsafe subprocess use

Automated review by Kōan (Claude) HEAD=8c82cdb 57s

@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.

@bdraco bdraco merged commit 8ab3d31 into main Jun 27, 2026
23 checks passed
@bdraco bdraco deleted the decline-adopt-ignored-config-dir branch June 27, 2026 21:05

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

Prevents the version-history feature from adopting an enclosing Git worktree when that enclosing repo ignores the configured config directory, avoiding both “no backups created” and unintended mutation of an unrelated repository’s .git/info/exclude.

Changes:

  • Declines adoption of an enclosing repo when git check-ignore indicates the config directory is ignored, and initializes a nested repo in the config dir instead.
  • Improves the info log message to reflect why adoption was declined (own source checkout vs ignored config dir).
  • Adds a regression test covering the “enclosing repo ignores config dir” scenario (issue #1718).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
esphome_device_builder/controllers/version_history/git_repo.py Adds ignore-detection to avoid adopting an enclosing repo that can’t track the config directory.
tests/controllers/version_history/test_git_repo.py Adds a test ensuring an ignored config dir gets its own nested history repo and the parent repo is not modified.

Comment on lines +245 to +250
"""A config dir gitignored by its enclosing repo gets a nested repo, not adoption.

Adopting an enclosing checkout that ``.gitignore``s the config dir backs up
nothing (git refuses the ignored YAML) while still writing our managed block
into that unrelated repo (#1718).
"""
Comment on lines +245 to +251
def _enclosing_repo_ignores_config_dir(self) -> bool:
"""Whether the enclosing work tree ignores ``config_dir`` itself.

A ``/config`` inside an unrelated checkout that ``.gitignore``s it can
never track our YAML, so adopting would back up nothing while still
writing into that repo; init a config-local repo instead.
"""
Comment on lines +252 to +256
result = self._run(
["check-ignore", "-q", str(self.config_dir)],
cwd=self.config_dir,
check=False,
)
@exciton

exciton commented Jun 27, 2026

Copy link
Copy Markdown

Thanks!

@bdraco

bdraco commented Jun 27, 2026

Copy link
Copy Markdown
Member Author

Copilot's review landed just after merge; addressed the three points in #1725: pass -- to git check-ignore, and trim the helper and test docstrings to terse contracts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Commits config to unrelated git repo in parent directory

4 participants