docs: Fix three broken copy-paste snippets in Git Workflow Guide - #1138
docs: Fix three broken copy-paste snippets in Git Workflow Guide#1138noahbjohnson wants to merge 5 commits into
Conversation
All three blocks were syntactically valid but failed when a reader ran them, which matters for a guide whose value is that people copy from it. 1. CI workflow checksum step. The asset was downloaded as glx.tar.gz but sha256sum -c was asked to verify glx_Linux_x86_64.tar.gz, which was no longer on disk. Under Actions' `bash -eo pipefail` the exit 1 failed the job on every run — a permanently red gate that reads like a rejected checksum. The asset now keeps its published name through download, verification, and extraction. 2. Merge-conflict Person snippet. A top-level `names:` list sat beside `properties:`; the Person schema is additionalProperties: false, so glx validate rejects it with "additional properties 'names' not allowed". The guide wires glx validate into a pre-commit hook a few sections earlier, so a reader who resolved the conflict as shown then could not commit. The name now sits under properties.name with value/fields, matching the Quickstart; conflict markers stay around occupation so the teaching point survives. 3. Research Cycle git add. Staging entity directories by name fails with "fatal: pathspec 'persons/' did not match any files" (exit 128) on a fresh clone: glx init creates them empty and Git does not track empty directories. It worked for the author and failed for the collaborator — the exact workflow the Collaboration section encourages. Now `git add -A`. The scaffold behavior is tracked separately in #1136. Each fix was reproduced against merged main and re-verified after. Fixes #1137 Signed-off-by: Noah B Johnson <noah@noahbjohnson.net>
Deploying genealogix with
|
| Latest commit: |
a3fa26d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://62be0d98.genealogix.pages.dev |
| Branch Preview URL: | https://docs-fix-git-workflow-snippe.genealogix.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
isaacschepp
left a comment
There was a problem hiding this comment.
Overview
Three independent runtime fixes to docs/guides/git-workflow.md, plus a matching ## [Unreleased] → Fixed CHANGELOG entry. I verified each against the repo rather than taking the description on faith:
- Checksum step —
.goreleaser.yml'sname_template({{ .ProjectName }}_{{ title .Os }}_x86_64foramd64) does resolve toglx_Linux_x86_64.tar.gz, andchecksum.name_templateischecksums.txt. Downloading under the published name makesgrep ' glx_Linux_x86_64.tar.gz$' checksums.txt | sha256sum -c -resolve against a file that exists, andtar -xzf … -C .bin glxstill extracts the root-level binary (archives.filesonly adds LICENSE/NOTICE/README/CHANGELOG alongside it). Correct. - Person snippet —
specification/schema/v1/person.schema.jsonis"additionalProperties": falsewith onlypropertiesandnotesallowed, so the old top-levelnames:was genuinely rejected. The new shape matchesdocs/quickstart.md:69-77exactly (minustype/sex), and bothnameandoccupationare registered inspecification/5-standard-vocabularies/person-properties.glx. Correct, and the right form to show here — no reservations on the "Review focus" question. git add -A—git add persons/on a fresh clone does fail with exit 128 whenglx init's empty directories were never tracked. Correct.
I also checked the fixes are complete rather than local: git grep finds no other - given: / top-level names: person examples anywhere in docs/ or specification/, and no other release-asset download block. The one remaining git add persons/ (specification/3-archive-organization.md:446) is a deliberate commit-by-entity-type example that runs after files exist, so it is not the same defect.
All 30+ checks pass; branch is mergeable; no prior reviews on the PR.
Requested change
One factual correction in the CHANGELOG (inline). Everything else below is optional.
Non-blocking suggestions
git add -Avs the "one source examined, one commit" discipline the same section teaches — see inline note. Also, the description says this matches the Setting Up section, which actually usesgit add .;-Ais the better choice for a guide (it is repo-root-scoped regardless of cwd), so I would keep-Aand not chase the wording.- The checksum block verifies integrity, not authenticity — worth a follow-up issue rather than a change here; see inline.
Risks
None. Documentation-only, no schema or code surface touched, and the reproduce-then-verify testing in the description is consistent with what I found in the repo.
|
|
||
| ### Fixed | ||
|
|
||
| - **Git Workflow Guide: three copy-paste snippets that failed when run** — All three blocks in `docs/guides/git-workflow.md` were syntactically valid but broke for a reader who actually ran them, which matters for a guide whose value is that people copy from it. (1) The CI workflow's install step downloaded the release asset as `glx.tar.gz` but asked `sha256sum -c` to verify `glx_Linux_x86_64.tar.gz`, which was no longer on disk — under GitHub Actions' `bash -eo pipefail` the resulting exit 1 failed the job on every run, giving anyone who copied the workflow a permanently red validation gate that looked like a rejected checksum; the asset now keeps its published name through download, verification, and extraction. (2) The merge-conflict example put a top-level `names:` list beside `properties:`, which `glx validate` rejects (`additional properties 'names' not allowed`, the Person schema being `additionalProperties: false`) — doubly awkward because the same guide wires `glx validate` into a pre-commit hook a few sections earlier, so a reader who resolved the conflict as shown then could not commit; the name now sits under `properties.name` with `value`/`fields`, matching the Quickstart, with the conflict markers still around `occupation`. (3) The Research Cycle staged entity directories by name (`git add persons/ events/ …`), which fails with `fatal: pathspec 'persons/' did not match any files` (exit 128) on a fresh clone, because `glx init` creates those directories empty and Git does not track empty directories — it worked on the author's machine and failed for the collaborator, the exact workflow the guide's Collaboration section encourages; now `git add -A`. The underlying scaffold behavior is tracked separately in #1136. Fixes #1137. |
There was a problem hiding this comment.
The shell attribution here is off. A run: step with no explicit shell: key — which is what the guide's snippet is — runs under GitHub Actions' default bash -e {0}; pipefail is only added when the step sets shell: bash explicitly (bash --noprofile --norc -eo pipefail {0}).
The conclusion is unaffected: sha256sum is the last command in the pipeline, so its exit 1 is the pipeline's status and -e fails the step either way. But this entry ships as documentation on the rendered changelog page, and a reader who takes the pipefail claim as a statement about default run: behavior will carry a wrong mental model into their own workflows.
Suggest dropping the flag name, e.g. "…the resulting exit 1 failed the job on every run under GitHub Actions' error-on-exit default". (The same phrasing appears in the PR description — worth fixing there too, though only the CHANGELOG actually ships.)
| ```bash | ||
| glx validate | ||
| git add persons/ events/ citations/ sources/ | ||
| git add -A |
There was a problem hiding this comment.
Correct fix — git add persons/ really does exit 128 on a fresh clone, and I confirmed no other doc in docs/ or specification/ stages entity directories this way except the deliberate commit-by-entity-type example at specification/3-archive-organization.md:446.
Optional: -A stages the entire working tree, which sits a little awkwardly against this section's own "one source examined, one commit" advice — a researcher with unrelated edits in flight will sweep them into the census commit. A half-sentence after the block would keep both the working command and the teaching point, e.g. "git add -A stages everything in the archive; if you have unrelated work in progress, stage those paths explicitly instead."
Take it or leave it — the command as written is correct and copy-pasteable, which is the point of the PR.
| curl -fsSL -o checksums.txt "$base/checksums.txt" | ||
| grep ' glx_Linux_x86_64.tar.gz$' checksums.txt | sha256sum -c - | ||
| tar -xzf glx.tar.gz -C .bin glx | ||
| tar -xzf glx_Linux_x86_64.tar.gz -C .bin glx |
There was a problem hiding this comment.
Not a change request for this PR — flagging as a follow-up, since this PR is what makes the block actually run.
The block now verifies the tarball against checksums.txt, but checksums.txt itself arrives over the same unauthenticated channel as the tarball. That catches corruption and truncated downloads; it does not catch a substituted release, which is what the comment's "never pipe an unverified download straight into a shell" is warning about.
The repo already publishes what closes this: .goreleaser.yml's signs: block runs cosign sign-blob over artifacts: checksum, producing checksums.txt.sigstore.json. A cosign verify-blob --bundle checksums.txt.sigstore.json --certificate-identity … --certificate-oidc-issuer https://token.actions.githubusercontent.com checksums.txt step ahead of the sha256sum line would make the guide's security claim real, at the cost of an extra install step in the example. Happy to file the issue if you want it tracked separately.
…workflow-snippets # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Pull request overview
Fixes three runnable-snippet regressions in the Git Workflow Guide so copy/paste examples work as intended, and records the user-facing docs fix in the changelog.
Changes:
- Update the Research Cycle snippet to use
git add -Aso it works on fresh clones where scaffold directories may not exist. - Fix the CI workflow example to keep the release asset filename consistent through download, checksum verification, and extraction.
- Reshape the merge-conflict Person example to a valid GLX Person shape (
properties.name), matching the Quickstart pattern.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/guides/git-workflow.md | Corrects three copy/paste blocks so they succeed when executed (git add, CI install/checksum, valid Person conflict example). |
| CHANGELOG.md | Adds an Unreleased → Fixed entry documenting the docs correction and referencing the tracking issue. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What and why
Fixes the three broken copy-paste snippets in
docs/guides/git-workflow.mdthat #1084 merged with. Each block is syntactically valid and looks right on the page, but fails when a reader actually runs it — which matters more than usual here, since the guide's value is that people copy these blocks verbatim.1. The CI workflow's checksum step failed on every run. The release asset was downloaded as
glx.tar.gz, butsha256sum -cwas asked to verifyglx_Linux_x86_64.tar.gz, which was no longer on disk. GitHub Actions runsrun:steps underbash -eo pipefail, so the exit 1 failed the job regardless of version or network — and the failure presents as a rejected checksum, inviting exactly the wrong diagnosis. The asset now keeps its published name through download, verification, and extraction.2. The merge-conflict Person snippet was invalid GLX. A top-level
names:list sat besideproperties:; the Person schema isadditionalProperties: false. Doubly awkward because the guide wiresglx validateinto a pre-commit hook a few sections earlier, so a reader who resolved the conflict exactly as shown then couldn't commit the result. The name now sits underproperties.namewithvalue/fields, matching the shape used in the Quickstart — conflict markers stay aroundoccupationso the teaching point is unchanged.3. The Research Cycle's
git addfailed on a fresh clone.git add persons/ events/ citations/ sources/exits 128 withfatal: pathspec 'persons/' did not match any files, becauseglx initcreates those directories empty and Git doesn't track empty directories. It works on the author's machine and fails for the collaborator — the exact clone-and-collaborate workflow the guide's Collaboration section encourages. Nowgit add -A, matching the Setting Up section. The underlying scaffold behavior is aglx initchange tracked separately in #1136; this is the docs-side fix that stands on its own.Related issues
Fixes #1137
Follow-up from review feedback on #1084, which merged before these were addressed.
Review focus
The reshaped Person snippet in "When They Happen Anyway" — whether
properties.namewithvalue/fields.given/fields.surnameis the form you want shown here. I matcheddocs/quickstart.md:69-77, which uses the sameperson-john-smithID, minus thetype: "birth"field andsex(not relevant to the conflict being illustrated).Otherwise trivial: two one-line command changes.
Testing
Each fix was reproduced as broken against merged
main, then re-verified after the change.sha256sum: glx_Linux_x86_64.tar.gz: No such file or directory→FAILED open or read, exit 1. After:glx_Linux_x86_64.tar.gz: OK, exit 0.glx validate→additional properties 'names' not allowed, structural validation failed. After (markers resolved): passes structural and semantic validation,occupationincluded.git add— before, on a realgit cloneof aglx initarchive:fatal: pathspec 'persons/' did not match any files, exit 128. After:git add -Aexits 0 and stages the entity file.Also run:
npm run build(VitePress) — completes, no dead linksnpx markdownlint-cli2on both changed files — clean (the one repo-wide finding is a pre-existing MD040 indocs/superpowers/specs/, untouched here)make check-linksis Linux-only on this machine (the script needs GNU awk's 3-argmatch(); BSD awk errors out), so it's left to CI. No links were changed.Breaking changes
None — documentation only.