Skip to content

Patch-stack rework: content-based trigger, richer reporting, no quiltimport - #43

Open
adunstan wants to merge 6 commits into
mainfrom
patch-stack-rework
Open

Patch-stack rework: content-based trigger, richer reporting, no quiltimport#43
adunstan wants to merge 6 commits into
mainfrom
patch-stack-rework

Conversation

@adunstan

@adunstan adunstan commented Aug 12, 2026

Copy link
Copy Markdown
Member

Six commits.

1. Add PGBuild::PatchSeries

The code that reads a quilt-style patch repository — resolving a series entry
to the patch it actually names, parsing series, materializing the result —
existed as two private copies, in PGBuild/Modules/PatchStack.pm and
check_patch_stack.pl. They had already drifted: the same symlink fix had to
land twice (9039004 and cb961c8), and the two series parsers disagreed about
an indented entry, which the buildfarm silently dropped and the checker
honoured. That last one is fixed here.

2. Trigger on resolved series content

A branch whose series names a patch in another branch's subdirectory, as
../master/foo.patch, was not rebuilt when that patch changed. The trigger was
the git tree SHA of the branch's own subdirectory, and that does not move in
this case — the series blob still holds the same text. The series was applied
every run regardless, so a stack that stopped applying was still reported; what
did not happen was rebuilding and retesting the branch against the changed
patch.

It is now a digest over the resolved blob SHA of every patch the series names.
Patches a branch does not name contribute nothing, so a push touching one
branch's stack still does not rebuild the others.

patch_stack.log gains a format marker, the patches-repo commit that was used,
and a blob SHA per patch. check_patch_stack.pl reads the stack through the
shared module and gains --manifest, which checks that every series entry
resolves to a file that is actually present — it needs no buildroot, so it is
cheap to run before pushing a stack change.

3. Replace git quiltimport

Three problems with it, each reproduced rather than assumed:

  • It skips a series entry whose patch file is absent and exits zero, so
    a branch could build and report a green result with a patch missing from its
    stack.
  • It is not the git am wrapper its name suggests. It uses
    $GIT_DIR/rebase-apply as its own scratch directory and does not remove it
    when it fails, leaving a repository git reports as mid-rebase which can be
    neither continued nor aborted.
  • The commit it creates per patch reached nothing: log_id() writes a headref
    captured at checkout, never a fresh rev-parse.

The series is now walked directly, applying each patch with git apply and
stopping at the first entry that is missing or fails. No commits are created
and no rebase state is written, so the stranded rebase cannot recur.
cleanup restores the tree with reset --hard and clean -fd.

check_patch_stack.pl --sequential calls the same code, so it predicts a real
run rather than approximating one. It consequently stops falling back from
-p1 to -p0; the default mode keeps that, since it checks each patch against
the pristine base in isolation.

4. Stop reading a strip level out of a series line's comment

parse_series() scanned every token on a series line for a -pN, so a level
written after the # that starts a comment was taken as real. The security
stacks keep a redmine id there, and a -p2 in that text became the strip level
the patch was applied at — a level neither quilt nor git quiltimport would
use, since both treat everything past the marker as comment.

Strip levels do not contribute to the series digest, so this moves no branch's
identity and triggers no rebuilds. Checked against the live security stacks: no
series line has that shape today, so the change is inert there and only stops
the divergence appearing later.

5. Apply the series with the patches repo's own driver

Per Noah Misch's point on the thread: the tool that applies the stack should
live in the patches repo and be called by the client, not be part of the
client, so that what an animal tests is what a release wrap produces rather
than something resembling it. A patch git apply takes and git am does not —
one with no usable From: or Subject: — passes on every animal and fails on
wrap day.

The client now looks for import-series.pl at the top of the patches repo and
hands it the resolved series when it is there. It gets the materialized copy,
so a shared ../master/foo.patch entry or a symlink is an ordinary file by the
time the script sees one — the same thing it sees on wrap day against a real
checkout. Choosing branches, fetching, resetting to upstream and deciding
whether to run at all stay on the client side.

The driver applies with git am, so HEAD moves and a commit is created per
patch. The client records the commit the tree started on, resets to that in
cleanup, and first clears any git am state a failed entry left behind,
which a reset does not remove. It supplies a committer identity when git cannot
find one of its own, since an animal's git is frequently unconfigured and git's
guess from the host name fails outright on a host with no domain; a configured
identity is left alone, and a patch's own From: wins either way.

The name comes from a new driver setting, defaulting to import-series.pl.
A name set in the config has to be there — naming one says the repo has an
applier of its own, and falling back from a name that turned out to be wrong
would apply the stack with something other than what the config asked for and
still report green. Only the default name may be absent, and then the built-in
loop runs. That is not a transition measure: PatchStack is a general module,
and a quilt-style repo that no release wrap consumes has no reason to carry a
driver. patch_stack.log records which of the two ran, as
patch_stack_applier; PatchStackLog.pm skips keys it does not know, so an
existing server ignores it.

6. Check the driver's reading of series against ours

Two programs parse that file — the client because the rebuild digest and the
list of blobs to materialize come out of it, the driver because its reading is
what gets applied — and nothing noticed when they differed. They did: commit 4
above is exactly that case.

The driver is run with --list before anything is applied. It prints its own
reading and exits without touching the tree: one line per entry, in series
order, name and strip level separated by a tab, - where the line gave no
level. A disagreement is reported with both readings and nothing applied. A
driver that does not understand --list is reported the same way rather than
skipping the check — a check that quietly passes when the other side is
unfamiliar is the failure it exists to prevent.

Verified by reverting commit 4 and re-running: the comment-strip case turns from
"applied at -p2 and reported green" into a reported disagreement.

Deploying this

Safe to run before the driver exists. With no import-series.pl in the patches
repo and no driver in the config, the lookup fails and the built-in git apply loop runs, exactly as it did before commits 5 and 6. The day the script
lands in the quilt repo, animals already on this client pick it up on their next
run with no config change, while animals on an older client carry on with git apply.

The driver itself is not in this PR — it belongs in the patches repo. Drafts
of it, and of the wrap-day scripts that call it, are written and tested but not
yet proposed.

One behaviour change worth arguing about

How much drift in surrounding context a patch may have now depends on where it
came from. A patch stored in a branch's own subdirectory is applied with full
context: it was written for that branch, so if it no longer applies exactly,
upstream has moved beneath the stack and that is worth reporting rather than
absorbing. An entry reaching into another branch's subdirectory keeps the
looser matching, since a patch written against one branch and applied to
another has an obvious reason to find its surroundings a little different.

Ordering

PGBuildFarm/server-code#20 has merged, so the reporting half has a server that
renders it and this is the remaining piece.

Not yet exercised on a real animal.

PGBuild::Modules::PatchStack and check_patch_stack.pl each carried a
private copy of the code that reads a quilt-style patch repository:
resolving a series entry to the patch it actually names, parsing the
series file, and materializing the result as plain files. The copies
had already drifted. The same symlink fix had to land twice, in
9039004 and cb961c8, and their series parsers disagreed about an
indented entry, which the buildfarm silently dropped and the checker
honoured.

Collect it in one module, so the tool that checks a stack and the
client that applies it cannot diverge again. The parser now strips
leading whitespace before splitting, which is the indented-entry fix.

Two routines are new rather than extracted: series_manifest(), which
resolves every entry to the blob it names and digests the ordered
result, and apply_series(), which applies a resolved series with
git apply. The manifest also records whether an entry resolved outside
its own subdirectory, which is what lets apply_series() decide how much
context drift to tolerate. The commits that follow are their callers.
A branch whose series names a patch in another branch's subdirectory,
as ../master/foo.patch, was not rebuilt when that patch changed. The
trigger was the git tree SHA of the branch's own subdirectory, and that
does not move in this case: the series blob still holds the same text.
The series was applied on every run regardless, so a stack that stopped
applying was still reported, but nothing rebuilt or retested the branch
against the changed patch, and no report recorded which stack content
had been exercised.

Use a digest over the resolved blob SHA of every patch the series
names. Patches a branch does not name contribute nothing to its
digest, so a push touching one branch's stack still does not rebuild
the others. The subdirectory tree SHA is still read, but only to tell
whether the branch has a stack at all.

patch_stack.log gains a format marker, the patches-repo commit that was
used, and a blob SHA per patch, so a report can identify what was
tested and can distinguish a modified patch from an added or removed
one. The server side is already in place.

check_patch_stack.pl reads the stack through the shared module too, and
gains --manifest, which checks that every series entry resolves to a
patch that is actually present. It reads only the patches repo, so
unlike the other modes it needs no buildroot or source tree. A series
naming a file that is not there means that patch is not tested on that
branch, which is easy to do and easy to miss; this catches it before
the push rather than after a build cycle.
git quiltimport skips a series entry whose patch file is absent and
exits zero, so a branch could build and report a green result with a
patch missing from its stack -- a silent failure, and the one that
prompted this. It is also not the git-am wrapper its name suggests: it
uses $GIT_DIR/rebase-apply as its own scratch directory and does not
remove it when it fails, leaving a repository git reports as mid-rebase
which can be neither continued nor aborted. The commit it creates per
patch reached nothing either, since log_id() writes a headref captured
at checkout rather than a fresh rev-parse.

Walk the series directly instead, applying each patch with git apply
and stopping at the first entry that is missing or fails to apply.
--index is what lets the cleanup reset remove a file a patch added;
without it the file is untracked and survives into later runs, still
being compiled after that patch leaves the series.

Context tolerance now depends on where a patch came from, rather than
being the single value quiltimport used for everything. A patch stored
in the branch's own subdirectory was written for that branch and is
applied with full context: if it no longer applies exactly, upstream
has moved beneath the stack, and that is worth reporting rather than
absorbing. An entry reaching into another branch's subdirectory was
written against a different branch, so drift in the surrounding code is
expected and it keeps the older tolerance. Note git reduces context
progressively and only as far as it must, so this is a floor in each
case, not a fixed amount.

No commits are created and no rebase state is written under $GIT_DIR,
so the stranded rebase cannot recur. cleanup restores the tree with
reset --hard and clean -fd rather than rewinding past imported commits.

check_patch_stack.pl --sequential calls the same code, so what it
reports is what an animal will do rather than an approximation. It
consequently stops falling back from -p1 to -p0; the default mode keeps
that, since it checks each patch against the pristine base in isolation
rather than predicting a real run.

run_log() takes an optional log directory, without which the shared
module could not use it from a standalone tool: the path it infers
comes from globals that only run_build.pl sets.
@adunstan
adunstan force-pushed the patch-stack-rework branch from 4530e4c to 4ee0fde Compare August 31, 2026 12:46
parse_series() scanned every whitespace-separated token on a series
line for a -pN, so a level written after the "#" that starts a comment
was taken as real. The security stacks keep a redmine id there, and a
-p2 in that text became the strip level the patch was applied at -- a
level neither quilt nor git quiltimport would use, since both treat
everything past the marker as comment. The patch would have gone in
somewhere no release wrap would put it, with nothing reporting the
disagreement.

Stop scanning at the first token beginning with "#". Strip levels do
not contribute to the series digest, so no branch's identity moves and
this triggers no rebuilds.

Claude-Session: https://claude.ai/code/session_01Xt53nR5qt8cLHwqEHN4s2c
An animal applies a security stack with our own git apply loop, while
the release wrap that ships the same stack applies it with a script of
its own. Two appliers means what the farm tests is only something
resembling what a wrap produces. A patch git apply takes and git am
does not -- one with no usable From: or Subject:, say -- passes here
and fails on wrap day, which is the most expensive day to find out.

Look for a script named import-series.pl at the top of the patches
repository and hand the series to it when it is there. It takes the
directory holding a series file as its only argument, and gets the
materialized copy, so a shared "../master/foo.patch" entry or a symlink
is an ordinary file by the time it sees one -- the same thing it sees
on wrap day against a real checkout. Everything the two callers do
differently, choosing branches, fetching, resetting to upstream and
deciding whether to run at all, stays on this side.

The driver applies with git am, so HEAD moves and a commit is created
per patch. Record the commit the tree started on and reset to that in
cleanup, clearing first any git am state a failed entry left behind,
which a reset does not remove. Supply a committer identity when git
cannot find one of its own: an animal's git is frequently unconfigured,
and git's guess from the host name fails outright on a host with no
domain. An owner who has configured one keeps it, and a patch's own
From: wins either way.

The name comes from the new driver setting, defaulting to
import-series.pl. A driver named in the config has to be there, because
naming one says the repository has an applier of its own, and falling
back from a name that turned out to be wrong would apply the stack with
something other than what the config asked for and still report green.
Only the default name may be absent, and then the built-in loop runs.
That is not a transition measure: PatchStack is a general module, and a
quilt-style repository that no release wrap consumes has no reason to
carry a driver. patch_stack.log records which of the two ran, as
patch_stack_applier; a server that does not know the key ignores it,
as it does any other key it does not know.

Per a suggestion from Noah Misch.

Claude-Session: https://claude.ai/code/session_01Xt53nR5qt8cLHwqEHN4s2c
Two programs parse the series file. We parse it because the digest that
decides whether a branch rebuilds, and the list of blobs to materialize,
both come out of that reading; the driver parses it because its reading
is what gets applied. Nothing noticed if the two read a line
differently, and they did: until the previous commit a strip level
written past the "#" that carries a redmine id was a level to us and
comment text to the driver, so the farm would have applied a patch at a
level no wrap would use and reported a green build for it.

Run the driver with --list before anything is applied. It prints its own
reading and exits without touching the tree: one line per entry, in
series order, the name and the strip level separated by a tab, "-" where
the line gave no level. It is run against the materialized copy, which
holds a byte copy of the series blob we parsed, so the two are reading
the same text. A disagreement is reported with both readings in full and
nothing applied.

A driver that does not understand --list is reported the same way rather
than skipping the check. A check that quietly passes when the other side
is unfamiliar is the failure it exists to prevent, and --list is part of
the interface from the first driver onwards.

Claude-Session: https://claude.ai/code/session_01Xt53nR5qt8cLHwqEHN4s2c
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.

1 participant