From 18158a60673c6332bd6c6318c7857030cecb22ba Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Wed, 5 Aug 2026 10:21:40 +0200 Subject: [PATCH 1/2] docs(git-workflow): warn about destructive checkout and identical hunks Three lessons from one session, all reference-only: `git checkout -- ` overwrites the working tree with no warning and no reflog entry. Used to restore a file after measuring a baseline, it silently discarded an uncommitted fix, and the following commit went out without it. `git show :` already documented just above is the non-destructive answer. Chaining `git push` behind a filtered test run with `&&` pushes on a red suite, because grep exits 0 on matching the word FAILURES. Two independent PRs that both need the same hunk can carry it byte-identically, which git merges once in any order. That removes the merge-order constraint instead of stacking the PRs. Signed-off-by: Sebastian Mendel --- .../git-workflow/references/advanced-git.md | 48 ++++++++++++++++++- .../references/pull-request-workflow.md | 42 ++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/skills/git-workflow/references/advanced-git.md b/skills/git-workflow/references/advanced-git.md index e334fdf..2f06ad0 100644 --- a/skills/git-workflow/references/advanced-git.md +++ b/skills/git-workflow/references/advanced-git.md @@ -909,6 +909,53 @@ To compare a file across branches without checkout, use `git diff -- ` (or `git diff ... -- ` for the merge-base–relative diff). +### Never `git checkout -- ` while the change is uncommitted + +`git checkout -- ` overwrites the working-tree file **without +warning and without a reflog entry** — there is nothing to recover from. The +pattern that bites is comparing current behaviour against a baseline: + +```bash +git checkout develop -- src/Service.php # measure the old behaviour +# … run the probe … +git checkout HEAD -- src/Service.php # "restore" +``` + +That last line restores the file as it is **committed**. If the fix you were +measuring was still only in the working tree, it is now gone, silently, and the +next `git add` commits the file without it. Observed cost: a commit pushed +without the fix it was created for, discovered only because the test output was +read line by line afterwards. + +Use `git show :` (above) to read the baseline instead — it never +touches the working tree. Where a baseline must genuinely be *on disk* (an +autoloader, a bundler), commit or stash first, or check the baseline out into a +throwaway worktree: + +```bash +git worktree add /tmp/baseline develop +``` + +### Never chain `git push` behind a test run with `&&` + +```bash +composer test | grep -E 'OK|FAILURES' && git add -A && git commit -s -m … && git push +``` + +This pushes on a failing suite. `&&` propagates the exit status of the **last** +command in the pipeline, and `grep` exits 0 as soon as it matches *anything* — +including the word `FAILURES`. Filtering test output for readability discards +the very status the chain depends on. + +Run the suite as its own command, read the result, and only then commit and +push. If it must be one invocation, gate on the runner rather than the filter +(`set -o pipefail`, or `composer test > out.txt; rc=$?; grep … out.txt; [ $rc -eq 0 ] || exit 1`). +Afterwards verify what actually landed, on the remote rather than locally: + +```bash +git fetch fork && git show FETCH_HEAD: | grep -c '' +``` + ## Troubleshooting ### Common Issues @@ -975,4 +1022,3 @@ gh api repos/OWNER/REPO/compare/... \ Verify against `origin` (or the compare API) before deleting a tag, choosing a release version, or concluding two lines forked — not against local refs. - diff --git a/skills/git-workflow/references/pull-request-workflow.md b/skills/git-workflow/references/pull-request-workflow.md index f349892..58b844a 100644 --- a/skills/git-workflow/references/pull-request-workflow.md +++ b/skills/git-workflow/references/pull-request-workflow.md @@ -828,6 +828,48 @@ jobs: ## Conflict Resolution +### Two independent PRs needing the same hunk — make it byte-identical + +When two PRs split out of one piece of work both need the same addition (a +`catch` block both paths now reach, an enum case both use, the same import), +there are three options and only one of them is good: + +| Approach | Cost | +|---|---| +| Stack the second PR on the first | Second PR can no longer merge on its own | +| Put the hunk in one PR only | The other PR is broken until that one lands — a merge-order trap | +| **Put the identical hunk in both** | **Git merges it once, in any order** | + +Git treats the same change at the same place as *one* change, so both PRs merge +cleanly and the hunk appears exactly once in the result. That removes the +ordering constraint entirely, which matters because maintainers merge in +whatever order they review. + +It only works if the text matches **byte for byte** — same wording, same +indentation, same position, and the same import placement (run the project's +formatter in both branches, or the import sorter will reorder one of them). Copy +it mechanically rather than retyping: + +```bash +git show : | sed -n '//,//p' > /tmp/hunk +diff <(sed -n '/…/,/…/p' ) /tmp/hunk && echo identical +``` + +Then prove the property instead of assuming it — merge every order in a +throwaway clone and check the result: + +```bash +git clone -q --shared /tmp/mergetest && cd /tmp/mergetest +for order in "A B" "B A"; do + git reset -q --hard + for b in $order; do git merge -q --no-edit "$b" || echo "CONFLICT in $order"; done +done +grep -c '' # must be 1, not 2 +``` + +State in both PR bodies that the hunk is identical and why, so a reviewer does +not "clean up" the duplicate in one of them. + ### Before Merging ```bash From bdce98d742ca062164af1bf9b47330bdac791cd9 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Wed, 5 Aug 2026 11:02:08 +0200 Subject: [PATCH 2/2] docs(git-workflow): address review on the checkout and hunk sections - The merge-order loop continued after a conflict, leaving the repo mid-merge and every later result meaningless. Abort and exit instead. - grep -c counts lines, so a multi-line pattern never matches: say to use one distinctive line, and -F to avoid regex surprises in code. - set -o pipefail is not in POSIX sh; lead with the portable rc capture and note where pipefail applies. - "nothing to recover from" was absolute; uncommitted content never reaches the object store, but an editor history or snapshot may hold it. - Replace the hardcoded develop with , and use consistent placeholders in the sed extraction. Signed-off-by: Sebastian Mendel --- .../git-workflow/references/advanced-git.md | 33 ++++++++++++++----- .../references/pull-request-workflow.md | 20 +++++++---- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/skills/git-workflow/references/advanced-git.md b/skills/git-workflow/references/advanced-git.md index 2f06ad0..ff4794e 100644 --- a/skills/git-workflow/references/advanced-git.md +++ b/skills/git-workflow/references/advanced-git.md @@ -912,13 +912,15 @@ for the merge-base–relative diff). ### Never `git checkout -- ` while the change is uncommitted `git checkout -- ` overwrites the working-tree file **without -warning and without a reflog entry** — there is nothing to recover from. The -pattern that bites is comparing current behaviour against a baseline: +warning and without a reflog entry**. Uncommitted content is never written to +the object store, so there is usually nothing in Git to recover from — only an +editor's local history or a filesystem snapshot can still hold it. The pattern +that bites is comparing current behaviour against a baseline: ```bash -git checkout develop -- src/Service.php # measure the old behaviour +git checkout -- src/Service.php # measure the old behaviour # … run the probe … -git checkout HEAD -- src/Service.php # "restore" +git checkout HEAD -- src/Service.php # "restore" ``` That last line restores the file as it is **committed**. If the fix you were @@ -933,7 +935,7 @@ autoloader, a bundler), commit or stash first, or check the baseline out into a throwaway worktree: ```bash -git worktree add /tmp/baseline develop +git worktree add /tmp/baseline ``` ### Never chain `git push` behind a test run with `&&` @@ -948,12 +950,25 @@ including the word `FAILURES`. Filtering test output for readability discards the very status the chain depends on. Run the suite as its own command, read the result, and only then commit and -push. If it must be one invocation, gate on the runner rather than the filter -(`set -o pipefail`, or `composer test > out.txt; rc=$?; grep … out.txt; [ $rc -eq 0 ] || exit 1`). -Afterwards verify what actually landed, on the remote rather than locally: +push. If it must be one invocation, gate on the runner rather than the filter — +capture the runner's status explicitly, which works in any POSIX shell: ```bash -git fetch fork && git show FETCH_HEAD: | grep -c '' +composer test > out.txt; rc=$? +grep -E 'OK|FAILURES' out.txt # read it, but don't gate on it +[ "$rc" -eq 0 ] || exit 1 +``` + +(`set -o pipefail` does the same in bash, zsh and ksh, but it is not in POSIX +`sh` — a script with `#!/bin/sh` under dash will fail on it.) + +Afterwards verify what actually landed, on the remote rather than locally. +Match one unique line from the change — `grep -c` counts matching *lines*, so a +multi-line pattern will not match at all; `-F` avoids regex surprises in code: + +```bash +git fetch +git show FETCH_HEAD: | grep -cF '' ``` ## Troubleshooting diff --git a/skills/git-workflow/references/pull-request-workflow.md b/skills/git-workflow/references/pull-request-workflow.md index 58b844a..11cca7d 100644 --- a/skills/git-workflow/references/pull-request-workflow.md +++ b/skills/git-workflow/references/pull-request-workflow.md @@ -848,23 +848,29 @@ whatever order they review. It only works if the text matches **byte for byte** — same wording, same indentation, same position, and the same import placement (run the project's formatter in both branches, or the import sorter will reorder one of them). Copy -it mechanically rather than retyping: +it mechanically rather than retyping, and compare the two extracts. `` +and `` are the first and last lines of the hunk, used as `sed` +address patterns: ```bash -git show : | sed -n '//,//p' > /tmp/hunk -diff <(sed -n '/…/,/…/p' ) /tmp/hunk && echo identical +git show : | sed -n '//,//p' > /tmp/hunk +diff <(sed -n '//,//p' ) /tmp/hunk && echo identical ``` Then prove the property instead of assuming it — merge every order in a -throwaway clone and check the result: +throwaway clone and check the result. Abort the run on the first conflict: +continuing would leave the repo mid-merge and every later result meaningless. ```bash git clone -q --shared /tmp/mergetest && cd /tmp/mergetest -for order in "A B" "B A"; do +for order in " " " "; do git reset -q --hard - for b in $order; do git merge -q --no-edit "$b" || echo "CONFLICT in $order"; done + for b in $order; do + git merge -q --no-edit "$b" || { echo "CONFLICT: $b in order [$order]"; git merge --abort; exit 1; } + done done -grep -c '' # must be 1, not 2 +# counts matching LINES, so use one distinctive line of the hunk, not the hunk +grep -cF '' # must be 1, not 2 ``` State in both PR bodies that the hunk is identical and why, so a reviewer does