Patch-stack rework: content-based trigger, richer reporting, no quiltimport - #43
Open
adunstan wants to merge 3 commits into
Open
Patch-stack rework: content-based trigger, richer reporting, no quiltimport#43adunstan wants to merge 3 commits into
adunstan wants to merge 3 commits into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three commits.
1. Add
PGBuild::PatchSeriesThe code that reads a quilt-style patch repository — resolving a
seriesentryto the patch it actually names, parsing
series, materializing the result —existed as two private copies, in
PGBuild/Modules/PatchStack.pmandcheck_patch_stack.pl. They had already drifted: the same symlink fix had toland twice (9039004 and cb961c8), and the two
seriesparsers disagreed aboutan 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
seriesnames a patch in another branch's subdirectory, as../master/foo.patch, was not rebuilt when that patch changed. The trigger wasthe git tree SHA of the branch's own subdirectory, and that does not move in
this case — the
seriesblob still holds the same text. The series was appliedevery 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.loggains a format marker, the patches-repo commit that was used,and a blob SHA per patch.
check_patch_stack.plreads the stack through theshared module and gains
--manifest, which checks that every series entryresolves 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 quiltimportThree problems with it, each reproduced rather than assumed:
a branch could build and report a green result with a patch missing from its
stack.
git amwrapper its name suggests. It uses$GIT_DIR/rebase-applyas its own scratch directory and does not remove itwhen it fails, leaving a repository git reports as mid-rebase which can be
neither continued nor aborted.
log_id()writes a headrefcaptured at checkout, never a fresh
rev-parse.The series is now walked directly, applying each patch with
git applyandstopping 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.
cleanuprestores the tree withreset --hardandclean -fd.check_patch_stack.pl --sequentialcalls the same code, so it predicts a realrun rather than approximating one. It consequently stops falling back from
-p1to-p0; the default mode keeps that, since it checks each patch againstthe pristine base in isolation.
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
Needs PGBuildFarm/server-code#20 first — the reporting half is inert until a
server can render what the client sends.
Not yet exercised on a real animal.