Skip to content

Narrow the dist/nginx un-ignore rule to the two tracked confs - #132

Merged
jgruberf5 merged 2 commits into
stagingfrom
fix/nginx-conf-glob-narrowing
Aug 18, 2026
Merged

jgruberf5 merged 2 commits into
stagingfrom
fix/nginx-conf-glob-narrowing

Conversation

@jgruberf5

Copy link
Copy Markdown
Collaborator

Lands a review fix from #121 that never reached staging.

Why this was orphaned

#121 merged at 2026-08-10T21:00:19Z. This commit (4c9fead0) was authored at 22:29:39Z — 89 minutes after the merge — in response to that PR's own review round, and was pushed to a branch that had already been merged. Nothing has carried it since, so staging still has the broad glob.

Cherry-picked onto current staging rather than re-proposing the original branch, which has drifted ~43 files behind and would revert unrelated work.

The problem

dist/.gitignore uses an ignore-everything-then-un-ignore pattern. The rule was:

!nginx/*.conf

dist/nginx/ is a directory that receives generated configs at runtime. *.conf un-ignores those too, so a generated conf would show up as an untracked file ready to be committed — the precise outcome the surrounding * rule exists to prevent.

Verified behaviour

Dropped two would-be generated files into dist/nginx/ and asked git directly:

File staging today with this PR
frontend.local.conf (tracked) tracked tracked
proxy.local.conf (tracked) tracked tracked
generated.local.conf not ignored ignored
upstream.conf not ignored ignored

Both tracked files keep working; only the generated ones change behaviour.

Also confirmed the explicit list is still complete — git ls-tree -r staging dist/ shows exactly two tracked .conf files, the two named here, with none added since August.

jgruber and others added 2 commits August 17, 2026 21:16
…ng *.conf

Self-review finding on this PR. `!nginx/*.conf` un-ignored ANY .conf dropped
into dist/nginx/, including a generated one — which is precisely what the `*`
rule exists to stop. Verified: dist/nginx/generated.conf was trackable before
this change and is ignored after it, while both real files stay tracked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012BDw71NbXZjH25q7FyMwo5
Two issues from the review of #132.

The comment justified the change by saying a GENERATED conf gets dropped in
dist/nginx/. Nothing does that. The only writer is the `dist` target, and it
copies exactly the two files that are tracked. The narrowing is still worth
having -- a glob auto-tracks whatever appears, and the leading `*` in this file
exists to stop precisely that -- but the stated reason described a scenario
that does not occur. Rewritten to say what is actually true.

Naming the files also introduces a failure the glob did not have: a conf added
later is silently untracked, so it survives on the author's disk and is missing
from a fresh clone. The two places that must agree are dist/.gitignore and the
`dist` target's cp lines, and neither pointed at the other. Both now do.

Verified after the edits: the two tracked confs stay tracked, a third file in
dist/nginx/ is ignored, and `make -n dist` still parses.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014KxvfrjQMigLtgt35REbQH
@jgruberf5

Copy link
Copy Markdown
Collaborator Author

Review of #132

Reviewed the change on its merits rather than as a formality — a one-line .gitignore edit still has a blast radius, and the justification turned out to be partly wrong. Two issues found and fixed in 2f4ac31; two more findings are real but belong outside this PR and are filed separately.


1. The stated rationale described something that does not happen (fixed)

The comment claimed the glob "would also un-ignore a GENERATED conf dropped in this directory". Nothing drops one. The only writer to dist/nginx/ is the dist target:

cp proxy/nginx.local.conf     dist/nginx/proxy.local.conf;
cp frontend-v2/nginx.local.conf dist/nginx/frontend.local.conf;

— and it writes exactly the two files that are tracked. The narrowing is still worth having (the leading * in this file exists to keep the tracked set deliberate, and a glob concedes that), but shipping a justification for a scenario that does not occur is how a rule outlives its reason. Rewritten to state what is actually true.

2. The change introduces a failure the glob did not have (fixed)

!nginx/*.conf tracked any conf automatically. Naming two files means a third one added later is silently untracked — it lives on the author's disk, passes review because the diff looks complete, and is simply absent from a fresh clone. Nothing pointed the two coupled places at each other.

dist/.gitignore now records the constraint, and the dist target carries a note at the exact lines someone would edit to add a conf. Not a mechanism, but the two places now name each other.


Verified

Check Result
frontend.local.conf, proxy.local.conf (tracked) still tracked
generated.local.conf, upstream.conf (would-be generated) ignored — on staging today they are not, and would be committed by accident
Tracked .conf files under dist/ exactly the two named — none added since August, so the explicit list is complete
make -n dist parses cleanly after the Makefile note

Behaviour was checked with git check-ignore against real files on disk, before and after, rather than by reading the pattern.


Out of scope, filed separately

Two things surfaced while establishing what this directory is for. Neither is caused by this PR and neither belongs in it.

dist/VERSION also reads 3.0.1 against a repo at 3.1.6; it is display-only (image tags come from $BNK_FORGE_VERSION), and it is covered in #134.

@mwiget mwiget left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review — narrow-dist-nginx-ignore @ 676dfcd

Small, self-contained, and the claim is the kind that is cheap to check rather than reason about — so I checked it. Ran git check-ignore on both branches with two would-be generated files dropped into dist/nginx/:

                                    staging        this PR
frontend.local.conf (tracked)       not ignored    not ignored
proxy.local.conf    (tracked)       not ignored    not ignored
generated.local.conf                not ignored    IGNORED
upstream.conf                       not ignored    IGNORED

Exactly as described: the two tracked confs are unaffected, only unlisted files change behaviour. Confirmed independently that git ls-tree -r staging dist/ yields precisely those two .conf files, and that the dist target copies precisely those two — so the explicit list is complete as of this commit.

The !nginx/ line above keeps the re-include legal (gitignore cannot re-include through an excluded parent directory), which is the part that would silently break this pattern if someone tightened the directory rule later.

On the trade

Naming files explicitly moves the failure mode from "tracks too much silently" to "tracks too little silently", and the second one is quieter — a conf missing from a fresh clone shows up as a broken install, not as a dirty git status. The PR takes that seriously in the right place: the note sits in the dist target where someone adding a cp will actually be looking, not only in .gitignore.

If you want it enforced rather than remembered, the dist target could assert that everything it copies into dist/nginx/ is tracked (git ls-files --error-unmatch) and fail the build otherwise. Not worth holding this PR for — the current comment is proportionate to a two-file list.

Approving. Behaviour verified on both branches, CI green across all 25 checks.

@jgruberf5
jgruberf5 merged commit 481efc8 into staging Aug 18, 2026
25 checks passed
@jgruberf5
jgruberf5 deleted the fix/nginx-conf-glob-narrowing branch August 18, 2026 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants