From 8afd49ff27e785f8c8bcc28735392f30e1fb30a3 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Tue, 11 Aug 2026 12:09:31 -0400 Subject: [PATCH 1/6] Add PGBuild::PatchSeries for shared patch-stack reading 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. --- Makefile | 2 +- PGBuild/PatchSeries.pm | 390 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 391 insertions(+), 1 deletion(-) create mode 100644 PGBuild/PatchSeries.pm diff --git a/Makefile b/Makefile index e1af172..03e7eb5 100644 --- a/Makefile +++ b/Makefile @@ -17,7 +17,7 @@ PERLFILES = run_build.pl run_web_txn.pl run_branches.pl \ update_personality.pl setnotes.pl manage_alerts.pl \ build-farm.conf.sample \ PGBuild/SCM.pm PGBuild/Options.pm PGBuild/WebTxn.pm PGBuild/Utils.pm \ - PGBuild/Log.pm PGBuild/VSenv.pm \ + PGBuild/Log.pm PGBuild/VSenv.pm PGBuild/PatchSeries.pm \ PGBuild/Modules/Skeleton.pm \ PGBuild/Modules/TestUpgrade.pm \ PGBuild/Modules/FileTextArrayFDW.pm PGBuild/Modules/BlackholeFDW.pm \ diff --git a/PGBuild/PatchSeries.pm b/PGBuild/PatchSeries.pm new file mode 100644 index 0000000..cfdc3a0 --- /dev/null +++ b/PGBuild/PatchSeries.pm @@ -0,0 +1,390 @@ + +=pod + +Copyright (c) 2003-2026, Andrew Dunstan + +See accompanying License file for license details + +=head1 PGBuild::PatchSeries + +Shared reading of a "quilt-style" patch-stack repository: resolving a +C entry to the patch content it actually names, parsing the +C file, and computing a stable identity for a branch's series. + +Used by both C (which applies the series) +and C (which checks that it applies). Those two +previously carried separate copies of this logic and drifted apart. + +A C entry may name a patch in another branch's subdirectory by +relative path, commonly C<../master/foo.patch>, so that a patch shared +unchanged across branches is stored once. Symlinks are also resolved, +but are no longer used in practice; the support is retained so existing +stacks do not regress. + +Resolution goes through git's tree and blob data rather than the +checked-out working tree. That is what makes it work on platforms where +a checked-out symlink is a plain text file holding its target path, and +it is also why paths are normalized first: C +silently returns empty content rather than erroring. + +=cut + +package PGBuild::PatchSeries; + +use strict; +use warnings; + +use Digest::SHA qw(sha1_hex); +use File::Path qw(mkpath); +use File::Basename qw(dirname); + +use PGBuild::Utils qw($devnull run_log); + +our ($VERSION); $VERSION = 'REL_21'; + +use Exporter qw(import); +our (@EXPORT_OK); +@EXPORT_OK = qw(normalize_git_path git_entry git_blob resolve_patch_path + parse_series series_manifest materialize_series apply_series); + +# Collapse "." and ".." segments in a git-style forward-slash path +# without touching the filesystem -- the target may not exist as a real +# file on this platform. +sub normalize_git_path +{ + my $path = shift; + my @out; + foreach my $part (split(m{/+}, $path)) + { + next if $part eq '' || $part eq '.'; + if ($part eq '..') { pop @out; } + else { push @out, $part; } + } + return join('/', @out); +} + +# Return (mode, sha) for a path in the patches repo at HEAD, or ('','') +# if it is not there. "git ls-tree" prints " \t"; +# the blob SHA is what identifies the patch content, and is what the +# callers below hash and report. +sub git_entry +{ + my ($repo, $path) = @_; + + my $line = `git -C "$repo" ls-tree HEAD -- "$path" 2>$devnull`; + chomp $line; + return ('', '') unless $line; + my ($mode, $type, $sha) = split(/\s+/, $line); + return (defined $mode ? $mode : '', defined $sha ? $sha : ''); +} + +# Raw content of the blob at a path in the patches repo at HEAD. +sub git_blob +{ + my ($repo, $path) = @_; + + return `git -C "$repo" show "HEAD:$path" 2>$devnull`; +} + +# Resolve a path to the blob it actually names, following symlink +# entries (mode 120000) by hand. Returns ($final_path, $sha), or an +# empty list if the path does not resolve. +sub resolve_patch_path +{ + my ($repo, $path) = @_; + + $path = normalize_git_path($path); + + foreach my $hop (1 .. 5) + { + my ($mode, $sha) = git_entry($repo, $path); + return () if $mode eq ''; + if ($mode eq '120000') + { + my $target = git_blob($repo, $path); + $target =~ s/\s+$//; + my ($dir) = $path =~ m{^(.*)/[^/]*$}; + $dir = defined $dir ? $dir : ''; + $path = normalize_git_path("$dir/$target"); + next; + } + return ($path, $sha); + } + + # symlink chain too deep + return (); +} + +# Parse series file content (a string, not a filename) per the quilt +# convention: one patch per line, blank lines and "#" comments skipped, +# an optional -pN token giving that patch's strip level. +# +# Leading and trailing whitespace is stripped before splitting. Without +# the leading strip, split(/\s+/, " 0001.patch") yields an empty first +# field and the entry is silently dropped; the trailing strip keeps a +# CRLF series file from producing names with a stray carriage return. +sub parse_series +{ + my $content = shift; + + my @out; + return @out unless defined $content; + + foreach my $line (split(/\n/, $content)) + { + $line =~ s/^\s+//; + $line =~ s/\s+$//; + next if $line =~ /^(#|$)/; + + my @tok = split(/\s+/, $line); + my $name = shift @tok; + next unless defined $name && $name ne ''; + + my $strip; + foreach my $t (@tok) + { + $strip = $1 if $t =~ /^-p(\d+)$/; + } + push(@out, { name => $name, strip => $strip }); + } + return @out; +} + +# Build the manifest for one branch's subdirectory: what every entry in +# the series resolves to, in series order, plus a digest over the lot. +# Returns undef when the subdirectory has no resolvable series file. +# +# The digest is what the buildfarm compares to decide whether a branch's +# stack has changed. It is computed from resolved blob SHAs rather than +# from the subdirectory's tree SHA, so that a patch shared from another +# branch by a "../master/foo.patch" entry triggers this branch when its +# content changes. A subdirectory tree SHA does not move in that case, +# because the series blob still holds the same text -- which is why such +# branches were not being rebuilt. +# +# Only patches this branch actually names contribute, so an unrelated +# change elsewhere in the patches repo does not cause a build here. +sub series_manifest +{ + my ($repo, $subdir) = @_; + + my ($series_path, $series_sha) = + resolve_patch_path($repo, "$subdir/series"); + return unless defined $series_path; + + my @entries; + my $blob = git_blob($repo, $series_path); + my @parsed = parse_series($blob); + + foreach my $e (@parsed) + { + my ($path, $sha) = resolve_patch_path($repo, "$subdir/$e->{name}"); + + # An entry is shared when it resolves outside this branch's own + # subdirectory -- a "../master/foo.patch" reference, or a symlink + # leading there. apply_series() allows such a patch more slack + # when matching context, since it was written against a different + # branch; a patch stored here was written for here and should + # apply exactly. + my $shared = + (defined $path && $path ne "$subdir/$e->{name}") ? 1 : 0; + + push( + @entries, + { + name => $e->{name}, + strip => $e->{strip}, + path => $path, + sha => $sha, + shared => $shared, + missing => (defined $path ? 0 : 1), + } + ); + } + + # Canonical text behind the digest. Series order is preserved, so a + # reordering registers as a change. An entry that does not resolve + # contributes the literal "missing" rather than an empty field, so + # the digest moves again once the patch appears. + my $canon = "series\t$series_sha\n"; + foreach my $e (@entries) + { + $canon .= + "$e->{name}\t" . ($e->{missing} ? 'missing' : $e->{sha}) . "\n"; + } + + return { + series_path => $series_path, + series_sha => $series_sha, + entries => \@entries, + id => sha1_hex($canon), + }; +} + +# Materialize a plain-file copy of a resolved series into $dest: the +# series file itself plus every entry the manifest resolved, with any +# symlinks already followed by series_manifest(). $dest is created if it +# does not exist, but is not cleared first -- callers that need a clean +# destination (e.g. because a previous run left stale files behind) are +# responsible for that themselves. +# +# The series file is written from $manifest->{series_path}, not from a +# path built out of $subdir, because the series file itself may be +# shared and its literal path a symlink whose blob is just the target +# name. +# +# An entry marked missing is skipped rather than written, and its name +# is collected into the returned arrayref. Reporting is left to the +# caller: this module is shared between the buildfarm client and a +# command-line tool, which prefix and route their output differently, +# so it returns what happened rather than deciding where it goes. +# +# A present entry's name may itself be a relative path out of the +# destination (a shared "../master/foo.patch" style reference), so its +# parent directory may not exist yet. +sub materialize_series +{ + my ($repo, $manifest, $dest) = @_; + + mkpath($dest); + + open(my $sfh, '>', "$dest/series") or die "writing $dest/series: $!\n"; + binmode $sfh; + print $sfh git_blob($repo, $manifest->{series_path}); + close $sfh; + + my @skipped; + foreach my $e (@{ $manifest->{entries} }) + { + if ($e->{missing}) + { + push(@skipped, $e->{name}); + next; + } + + my $target_file = "$dest/$e->{name}"; + mkpath(dirname($target_file)); + open(my $pfh, '>', $target_file) or die "writing $target_file: $!\n"; + binmode $pfh; + print $pfh git_blob($repo, $e->{path}); + close $pfh; + } + return \@skipped; +} + +# Apply a materialized series to a source tree, in series order, +# stopping at the first entry that is missing or fails to apply. +# +# This replaces "git quiltimport", which had three defects. It skipped +# an entry whose file was absent and still exited 0, so a stack could +# silently omit a patch while the farm reported green. It created a +# commit per patch that nothing consumed -- PGBuild::SCM::log_id writes +# a headref captured at checkout, never a fresh rev-parse. And it used +# $GIT_DIR/rebase-apply as its own scratch directory without removing +# it on the failure path, leaving a repository git reported as +# mid-rebase which could be neither continued nor aborted. +# +# Patches are applied with "git apply --index", plus a context setting +# that depends on where the patch came from: +# +# --index so a file added by a patch is staged, and "reset --hard" +# therefore removes it. Without it the file is untracked and +# survives into later runs. +# -C3 for a patch stored in this branch's own subdirectory. It +# was written for this branch, so it should apply exactly; +# if it no longer does, upstream has moved beneath the stack +# and that is worth reporting rather than absorbing. +# -C1 for an entry resolving into another branch's subdirectory, +# a "../master/foo.patch" reference. That patch was written +# against a different branch, so some drift in the code +# around it is expected, and it gets the tolerance +# quiltimport applied to everything. +# +# Note -C is a floor, not a setting: git tries the full context first +# and reduces only as far as it must, so a patch whose context matches +# in full is applied in full under either value. +# +# No commits are created and HEAD never moves, so no rebase state is +# written under $GIT_DIR and the corruption above cannot recur. +# +# Commands run through run_log, which merges stdout and stderr; the +# combined text is returned per entry rather than logged here, because +# this module is shared between the buildfarm client and a command-line +# tool that route output differently. $logdir is passed through to +# run_log and may be undef on the farm. +sub apply_series +{ + my ($manifest, $patchdir, $srcdir, $logdir) = @_; + + my @applied; + + foreach my $e (@{ $manifest->{entries} }) + { + my $file = "$patchdir/$e->{name}"; + + if ($e->{missing} || !-f $file) + { + return { + ok => 0, + applied => \@applied, + failure => { + name => $e->{name}, + reason => 'missing', + detail => "no patch file for this series entry\n", + }, + }; + } + + # Mirror quiltimport: pass -pN only when the series line gave + # one. Deliberately no -p1 -> -p0 fallback; guessing a strip + # level during a real apply is what this change exists to stop. + my $strip = defined $e->{strip} ? "-p$e->{strip} " : ''; + + # Context tolerance depends on where the patch came from. A + # patch stored in this branch's own subdirectory was written for + # this branch and should apply exactly; if it no longer does, + # upstream has moved under it and the stack wants rebasing, + # which is worth being told about rather than papering over. An + # entry reaching into another branch's subdirectory was written + # against that branch, so some drift in the surrounding code is + # expected and it gets the slack quiltimport gave everything. + # + # git reduces context progressively and only as far as it must, + # so -C is a floor rather than a setting: a patch whose context + # matches in full is applied in full either way. + my $context = $e->{shared} ? '-C1' : '-C3'; + + my @out = run_log( + qq{git -C "$srcdir" apply --index $context $strip-- "$file"}, + $logdir); + my $status = $? >> 8; + my $text = join('', @out); + + if ($status) + { + return { + ok => 0, + applied => \@applied, + failure => { + name => $e->{name}, + reason => 'apply-failed', + detail => $text, + }, + }; + } + + push( + @applied, + { + name => $e->{name}, + sha => $e->{sha}, + path => $e->{path}, + output => $text, + } + ); + } + + return { ok => 1, applied => \@applied }; +} + +1; From bb1b88bb6810a56c896f381f9b4df2480dc5e718 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Tue, 11 Aug 2026 12:09:42 -0400 Subject: [PATCH 2/6] Trigger patch-stack runs 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 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. --- PGBuild/Modules/PatchStack.pm | 345 +++++++++++++++------------------- check_patch_stack.pl | 259 ++++++++++--------------- docs/patch-stack.md | 99 ++++++++-- 3 files changed, 330 insertions(+), 373 deletions(-) diff --git a/PGBuild/Modules/PatchStack.pm b/PGBuild/Modules/PatchStack.pm index 54bb607..b2bf148 100644 --- a/PGBuild/Modules/PatchStack.pm +++ b/PGBuild/Modules/PatchStack.pm @@ -25,28 +25,58 @@ files must carry C and C headers (i.e. be produced by C or equivalent) so that C can extract the author. Bare diffs will not import. -A patch shared unchanged across branches may be referenced rather -than copied: either as a C entry using a relative path into -another branch's subdirectory (commonly C<../master/foo.patch>), or -as a symlink into another branch's subdirectory. Both forms are -resolved via git's own tree/blob data, not the checked-out working -tree, so it works regardless of the platform's filesystem symlink -support (and of whether C<..> in the path has been normalized -- a -raw C silently returns empty content for an -unnormalized path instead of erroring, so paths are normalized -before being resolved). A patch listed in a branch's C with -no entry at all in the patches branch (not even by relative path or -symlink) is a broken patch stack for that branch: it is logged and -left out of the import, which then fails loudly rather than silently -applying a substitute from elsewhere. +A patch shared unchanged across branches may be referenced rather than +copied, as a C entry giving a relative path into another +branch's subdirectory -- commonly C<../master/foo.patch>. Symlinks into +another branch's subdirectory are also resolved, but are no longer used +in practice; the support remains so that existing stacks do not +regress. + +Both forms are resolved via git's own tree and blob data, not the +checked-out working tree, so they work regardless of the platform's +filesystem symlink support (and of whether C<..> in the path has been +normalized -- a raw C silently returns empty +content for an unnormalized path instead of erroring, so paths are +normalized before being resolved). A patch listed in a branch's +C with no entry at all in the patches branch is a broken patch +stack for that branch: it is logged and left out of the import, which +then fails loudly rather than silently applying a substitute from +elsewhere. =head2 RUN TRIGGER -The module forces a run whenever the patch-stack subdirectory tree -identifier (the git tree SHA of C<< I:I >>) -differs from the value recorded on the previous run. This is in -addition to the usual upstream-branch trigger, so a build kicks off -when either the upstream branch or the patch series moves. +The module forces a run whenever the identity of this branch's patch +series differs from the value recorded on the previous run. That +identity is a digest over the resolved blob SHA of every patch the +series names, in order -- not the git tree SHA of the branch's +subdirectory. + +The distinction matters when a patch is shared between branches. A +series entry may name a patch in another branch's subdirectory, as +C<../master/foo.patch>. Editing that patch does not change the +referring branch's subdirectory tree, because the C blob still +holds the same text, so a tree-SHA trigger never fired and the branch +was neither rebuilt nor retested against the changed patch. + +The series was still applied on every run -- the C hook fires +unconditionally, and C declines to prune branches when +this module is configured -- so a patch that had stopped applying was +still reported as C. What was missing was any +verification that the patched tree still built and passed its tests, +and any record of which stack content had been exercised. + +Digesting resolved content instead tracks what the branch would +actually apply. Patches the branch does not name contribute nothing, so +an unrelated change elsewhere in the patches repo still does not cause +a build here. + +This is in addition to the usual upstream-branch trigger, so a build +kicks off when either the upstream branch or the patch series moves. + +The identity changed shape when content digests replaced subdirectory +tree SHAs. On the first run after upgrading from an earlier client, +the recorded value is a tree SHA and the computed one is a digest, so +each configured branch rebuilds once and then settles. =head2 CONFIGURATION @@ -74,10 +104,10 @@ package PGBuild::Modules::PatchStack; use PGBuild::Options; use PGBuild::SCM; -use PGBuild::Utils qw(:DEFAULT $st_prefix $branch_root $devnull); +use PGBuild::Utils qw(:DEFAULT $st_prefix $branch_root $devnull); +use PGBuild::PatchSeries qw(series_manifest materialize_series); -use File::Path qw(mkpath); -use File::Basename qw(dirname); +use File::Path qw(mkpath); use strict; use warnings; @@ -129,6 +159,8 @@ sub setup local_repo => $local_repo, applied => 0, patches_id => '', + stack_commit => '', + manifest => undef, pre_apply_sha => '', }; bless($self, $class); @@ -181,13 +213,13 @@ sub _fetch_or_clone return; } -# Stable identifier for "the patches as they exist right now" for -# this Postgres branch — the git tree SHA of the per-branch -# subdirectory. Empty string if the subdirectory doesn't exist. -# Using the subdirectory tree SHA rather than the patches repo's -# commit SHA means that a change to another branch's subdirectory -# does not trigger a rebuild for this branch. -sub _patches_id +# Tree SHA of this branch's subdirectory in the patches branch, or empty +# string if the subdirectory is not there at all. Used only to decide +# whether this branch has a stack; the identity that decides whether the +# stack has *changed* is the content digest from series_manifest(), +# because a subdirectory tree SHA does not move when a patch reached by +# a "../master/foo.patch" series entry is edited. +sub _subdir_tree { my $self = shift; my $local = $self->{local_repo}; @@ -198,40 +230,30 @@ sub _patches_id return $id; } -# Log the patch series we are about to import, one line per patch: -# the file name (as listed in series) followed by the subject. We -# derive the subject the same way quiltimport does for the commit it -# creates -- via "git mailinfo", which unwraps the header and strips -# any "[PATCH ...]" prefix -- falling back to the file name minus a -# trailing ".patch" when the patch carries no Subject: header. +# Log the patch series from the manifest computed in checkout(), rather +# than a fresh read of series, one line per patch: the file name (as +# listed in series) followed by the subject. We derive the subject the +# same way quiltimport does for the commit it creates -- via +# "git mailinfo", which unwraps the header and strips any "[PATCH ...]" +# prefix -- falling back to the file name minus a trailing ".patch" when +# the patch carries no Subject: header. # -# Returns the parsed list as an arrayref of { name => , subject => } -# hashrefs, so callers can reuse it (e.g. to write patch_stack.log) -# without re-deriving the subjects. +# Returns the parsed list as an arrayref of { name => , sha => , +# subject => } hashrefs, so callers can reuse it (e.g. to write +# patch_stack.log) without re-deriving the subjects. sub _log_series { my $self = shift; my $log = shift; my $patchdir = shift; - open(my $fh, '<', "$patchdir/series") or return []; - my @patches; - while (my $line = <$fh>) - { - chomp $line; - - # mirror quiltimport's parsing: skip blanks and comments, and - # take the first whitespace-delimited token as the file name. - next if $line =~ /^\s*(#|$)/; - my ($name) = split(/\s+/, $line); - push(@patches, $name) if defined $name && $name ne ''; - } - close $fh; + my $entries = $self->{manifest} ? $self->{manifest}{entries} : []; - push(@$log, "$MODULE: series (" . scalar(@patches) . " patches):\n"); + push(@$log, "$MODULE: series (" . scalar(@$entries) . " patches):\n"); my @parsed; - foreach my $name (@patches) + foreach my $e (@$entries) { + my $name = $e->{name}; my $file = "$patchdir/$name"; my $subject = ''; if (-f $file) @@ -247,155 +269,33 @@ sub _log_series { ($subject = $name) =~ s/\.patch$//; } - push(@$log, " $name: $subject\n"); - push(@parsed, { name => $name, subject => $subject }); - } - return \@parsed; -} - -# Return the git mode ('100644', '120000', ...) of a path in the -# patches repo at HEAD, or '' if it doesn't exist there. -sub _git_mode -{ - my $self = shift; - my $path = shift; - my $local = $self->{local_repo}; - - my $line = `git -C $local ls-tree HEAD -- "$path" 2>$devnull`; - chomp $line; - return '' unless $line; - my ($mode) = split(/\s+/, $line); - return $mode // ''; -} - -# Return the raw content of the blob at the given path in the patches -# repo at HEAD. -sub _git_blob -{ - my $self = shift; - my $path = shift; - my $local = $self->{local_repo}; - - return `git -C $local show "HEAD:$path" 2>$devnull`; -} -# Collapse "." and ".." segments in a git-style forward-slash path -# without touching the filesystem -- the target may not exist as a -# real file on this platform (e.g. behind an unmaterialized symlink). -sub _normalize_git_path -{ - my $path = shift; - my @out; - foreach my $part (split(m{/+}, $path)) - { - next if $part eq '' || $part eq '.'; - if ($part eq '..') { pop @out; } - else { push @out, $part; } + my $short = substr(defined $e->{sha} ? $e->{sha} : '', 0, 7); + push(@$log, " $name [$short]: $subject\n"); + push(@parsed, { name => $name, sha => $e->{sha}, subject => $subject }); } - return join('/', @out); -} - -# Resolve a path in the patches repo to its real file content, following -# git symlinks (mode 120000) by hand via git's tree/blob data rather -# than the checked-out working tree. Patches repos may share an -# unmodified patch across branches via a symlink; Windows without -# core.symlinks enabled checks such a symlink out as a plain text file -# containing the link target, which is useless to quiltimport. Reading -# through git's plumbing instead sidesteps that platform limitation -# entirely, and works the same way regardless of how (or whether) the -# platform materializes real filesystem symlinks. -sub _resolve_patch_content -{ - my $self = shift; - my $log = shift; - my $path = shift; - - # A series entry may reference a patch in another branch's - # subdirectory via a relative path (e.g. "../master/foo.patch") - # rather than a symlink. "git ls-tree" resolves "." / ".." path - # segments itself, but "git show HEAD:" does not -- it - # silently returns empty content for an unnormalized path instead - # of erroring, which would otherwise materialize a blank, useless - # patch file. Normalize up front so both plumbing calls agree on - # the same path. - $path = _normalize_git_path($path); - - for (1 .. 5) - { - my $mode = $self->_git_mode($path); - return undef if $mode eq ''; - if ($mode eq '120000') - { - my $target = $self->_git_blob($path); - $target =~ s/\s+$//; - my ($dir) = $path =~ m{^(.*)/[^/]*$}; - $dir //= ''; - $path = _normalize_git_path("$dir/$target"); - next; - } - return $self->_git_blob($path); - } - push(@$log, "$MODULE: symlink chain too deep resolving $path\n"); - return undef; + return \@parsed; } # Materialize a plain-file copy of the patch series with any symlinks -# resolved to their real content (see _resolve_patch_content), so -# quiltimport and our own mailinfo parsing operate on real patch +# resolved to their real content (see PGBuild::PatchSeries::series_manifest), +# so quiltimport and our own mailinfo parsing operate on real patch # content regardless of the platform's symlink support. sub _build_resolved_dir { my $self = shift; my $log = shift; my $sub = $self->{subdir}; + my $local = $self->{local_repo}; + my $manifest = $self->{manifest}; - my $dest = "$self->{local_repo}.resolved/$sub"; - rmtree($dest) if -d $dest; - mkpath($dest); - - my $series_content = $self->_resolve_patch_content($log, "$sub/series"); - die "resolving $sub/series\n" unless defined $series_content; - open(my $sfh, '>', "$dest/series") or die "writing $dest/series: $!\n"; - binmode $sfh; - print $sfh $series_content; - close $sfh; - - # Derive the patch list from the just-resolved series content, not - # a fresh read of the checked-out series file: if the series file - # itself is shared via symlink (as this module allows -- see the - # module docs), the two can disagree on a platform without - # filesystem symlink support, where the raw checkout is just a - # text file holding the link target. - my @names; - foreach my $line (split /\n/, $series_content) - { - next if $line =~ /^\s*(#|$)/; - my ($name) = split(/\s+/, $line); - push(@names, $name) if defined $name && $name ne ''; - } - - foreach my $name (@names) - { - my $content = $self->_resolve_patch_content($log, "$sub/$name"); - unless (defined $content) - { - push(@$log, "$MODULE: could not resolve $sub/$name, skipping\n"); - next; - } + die "resolving $sub/series\n" unless $manifest; - # $name may itself be a relative path out of this branch's - # subdirectory (e.g. "../master/foo.patch", used to share a - # patch unchanged across branches without a symlink), so its - # parent directory may not be $dest itself and may not exist - # yet. - my $target_file = "$dest/$name"; - mkpath(dirname($target_file)); - open(my $pfh, '>', $target_file) - or die "writing $target_file: $!\n"; - binmode $pfh; - print $pfh $content; - close $pfh; - } + my $dest = "$local.resolved/$sub"; + rmtree($dest) if -d $dest; + my $skipped = materialize_series($local, $manifest, $dest); + push(@$log, "$MODULE: could not resolve $sub/$_, skipping\n") + foreach @$skipped; return $dest; } @@ -496,21 +396,54 @@ sub _apply_patches # process well before cleanlogs() would run -- so that path writes the # log directly, right before send_result(), instead of relying on the # hook. -sub _write_patch_stack_log +# Build the contents of patch_stack.log as a list of lines. Split out +# from _write_patch_stack_log so the format can be exercised without a +# lastrun-logs directory to write into. +# +# Header lines must not contain a tab. A server that does not know a key +# skips it precisely because it matches neither the "key: value" pattern +# nor the tab-delimited patch pattern, so a tab in a header would be +# parsed as a bogus patch entry by older servers. +sub _patch_stack_log_lines { my $self = shift; my @lines; - push(@lines, "patch_stack_id: " . ($self->{patches_id} // '') . "\n"); + push(@lines, "patch_stack_format: 2\n"); + push(@lines, + "patch_stack_id: " + . (defined $self->{patches_id} ? $self->{patches_id} : '') + . "\n"); + push(@lines, + "patch_stack_commit: " + . (defined $self->{stack_commit} ? $self->{stack_commit} : '') + . "\n"); push(@lines, "patch_stack_source: $self->{patches_branch}:$self->{subdir}\n"); push(@lines, - "patch_stack_status: " . ($self->{series_status} // '') . "\n"); - foreach my $p (@{ $self->{series_patches} // [] }) + "patch_stack_status: " + . (defined $self->{series_status} ? $self->{series_status} : '') + . "\n"); + + foreach my $p (@{ $self->{series_patches} || [] }) { - push(@lines, "$p->{name}\t$p->{subject}\n"); + push( + @lines, + join("\t", + $p->{name}, + (defined $p->{sha} ? $p->{sha} : ''), + (defined $p->{subject} ? $p->{subject} : '')) + . "\n" + ); } - writelog('patch_stack', \@lines); + return \@lines; +} + +sub _write_patch_stack_log +{ + my $self = shift; + + writelog('patch_stack', $self->_patch_stack_log_lines()); return; } @@ -531,13 +464,13 @@ sub checkout send_result("$MODULE-fetch", 1, $savescmlog); } - $self->{patches_id} = $self->_patches_id(); + my $subdir_tree = $self->_subdir_tree(); push(@$savescmlog, "$MODULE: $self->{patches_branch}:$self->{subdir} = " - . ($self->{patches_id} || '(absent)') + . ($subdir_tree || '(absent)') . "\n"); - unless ($self->{patches_id}) + unless ($subdir_tree) { print time_str(), "$MODULE: subdirectory '$self->{subdir}' absent in" @@ -551,6 +484,22 @@ sub checkout exit 0; } + # Compute the series identity before applying anything, so that a + # series which fails to apply still has an identity to report. + my $manifest = series_manifest($self->{local_repo}, $self->{subdir}); + $self->{manifest} = $manifest; + $self->{patches_id} = $manifest ? $manifest->{id} : ''; + + my $commit = + `git -C $self->{local_repo} rev-parse --verify --quiet HEAD 2>$devnull`; + chomp $commit; + $self->{stack_commit} = $commit; + + push(@$savescmlog, + "$MODULE: patches commit $commit, series id " + . ($self->{patches_id} || '(none)') + . "\n"); + my $ok = $self->_apply_patches($savescmlog); unless ($ok) diff --git a/check_patch_stack.pl b/check_patch_stack.pl index 9026b05..4c6a7ac 100755 --- a/check_patch_stack.pl +++ b/check_patch_stack.pl @@ -36,19 +36,20 @@ =head1 check_patch_stack.pl reported as skipped. A series entry may reference a patch shared unchanged with another -branch's subdirectory rather than a copy: either as a relative path -(commonly C<../master/foo.patch>) or as a symlink. Both are resolved -via C/C against the patch-stack repo's C -rather than the checked-out working tree, mirroring how -C resolves them -- so this script sees -exactly what a buildfarm animal would apply, including on platforms -where a checked-out symlink is just a text file holding its target -path. For this reason C<< >> must itself be a git -repository (a plain directory of files is not enough). +branch's subdirectory rather than a copy, by giving a relative path -- +commonly C<../master/foo.patch>. Symlinks are also resolved, but are no +longer used in practice and are supported only so existing stacks do +not regress. Both are resolved via git plumbing against the patch-stack +repo's C rather than the checked-out working tree, mirroring +C -- so this script sees exactly what a +buildfarm animal would apply, including on platforms where a +checked-out symlink is just a text file holding its target path. For +this reason C<< >> must itself be a git repository +(a plain directory of files is not enough). =head1 USAGE - check_patch_stack.pl [options] + check_patch_stack.pl [options] [] Options: @@ -59,6 +60,10 @@ =head1 USAGE (default 1) --sequential apply patches cumulatively in series order (in a scratch worktree), stopping at the first failure + --manifest check that every series entry resolves to a patch + that is present, printing what each resolves to; + reads only the patches repo, so is not + required. Exit 1 if any entry is missing --verbose show git apply diagnostics for failing patches --help this message @@ -71,22 +76,20 @@ =head1 USAGE use warnings; use Getopt::Long; -use File::Spec; -use File::Temp qw(tempdir); -use File::Path qw(mkpath); -use File::Basename qw(dirname); -use Cwd qw(abs_path); +use File::Temp qw(tempdir); +use Cwd qw(abs_path); -# Platform-correct null device (e.g. 'nul' on Windows), matching how -# PGBuild::Utils derives $devnull -- hardcoding '/dev/null' would defeat -# the point of resolving patches via git plumbing for Windows support. -my $devnull = File::Spec->devnull; +use FindBin; +use lib $FindBin::RealBin; +use PGBuild::PatchSeries qw(series_manifest materialize_series); +use PGBuild::Utils qw($devnull); my @only_branches; my @map_args; my $default_strip = 1; my $sequential = 0; my $verbose = 0; +my $manifest_only = 0; my $help = 0; GetOptions( @@ -95,23 +98,31 @@ =head1 USAGE 'strip=i' => \$default_strip, 'sequential' => \$sequential, 'verbose' => \$verbose, + 'manifest' => \$manifest_only, 'help' => \$help, ) or usage(2); usage(0) if $help; my ($repo, $buildroot) = @ARGV; -usage(2) unless defined $repo && defined $buildroot; -$repo = abs_path($repo) // fail("no such patch-stack repo: $ARGV[0]\n"); -$buildroot = abs_path($buildroot) // fail("no such buildroot: $ARGV[1]\n"); +# --manifest reads only the patches repo, so a buildroot is not needed. +usage(2) unless defined $repo; +usage(2) unless defined $buildroot || $manifest_only; +$repo = abs_path($repo) // fail("no such patch-stack repo: $ARGV[0]\n"); fail("patch-stack repo is not a directory: $repo\n") unless -d $repo; -fail("buildroot is not a directory: $buildroot\n") unless -d $buildroot; + +if (defined $buildroot) +{ + $buildroot = abs_path($buildroot) // fail("no such buildroot: $ARGV[1]\n"); + fail("buildroot is not a directory: $buildroot\n") unless -d $buildroot; +} # Symlinked/shared-path patches are resolved via git plumbing against -# HEAD (see resolve_patch_content), so the repo must actually be a git -# checkout -- a plain directory of files isn't enough. +# HEAD (see PGBuild::PatchSeries::resolve_patch_path), so the repo must +# actually be a git checkout -- a plain directory of files isn't +# enough. system(qq{git -C "$repo" rev-parse --git-dir >$devnull 2>&1}) == 0 or fail("patch-stack repo is not a git repository: $repo\n"); @@ -138,6 +149,44 @@ =head1 USAGE fail("no series subdirectories found under $repo\n") unless @subdirs; +if ($manifest_only) +{ + my $bad = 0; + foreach my $sub (@subdirs) + { + next if %only && !$only{$sub}; + + print "=== series: $sub ===\n"; + my $manifest = series_manifest($repo, $sub); + unless ($manifest) + { + print " BROKEN: cannot resolve $sub/series\n\n"; + $bad = 1; + next; + } + + printf " %-28s %s\n", 'series', substr($manifest->{series_sha}, 0, 7); + foreach my $e (@{ $manifest->{entries} }) + { + if ($e->{missing}) + { + printf " %-28s %s\n", $e->{name}, '(missing)'; + $bad = 1; + next; + } + + # Show where an entry resolved only when that differs from + # the entry itself, so shared patches stand out. + my $where = + ($e->{path} eq "$sub/$e->{name}") ? '' : " -> $e->{path}"; + printf " %-28s %s%s\n", $e->{name}, substr($e->{sha}, 0, 7), + $where; + } + print " patch_stack_id: $manifest->{id}\n\n"; + } + exit($bad ? 1 : 0); +} + # Shared destination for resolved (symlink-free) copies of each series # tested below, so relative-path series entries that point at another # subdirectory (e.g. "../master/foo.patch") resolve to a sibling @@ -215,122 +264,29 @@ =head1 USAGE #--------------------------------------------------------------------- -# Return the git mode ('100644', '120000', ...) of a path in the -# patch-stack repo at HEAD, or '' if it doesn't exist there. -sub git_mode -{ - my ($repo, $path) = @_; - - my $line = `git -C "$repo" ls-tree HEAD -- "$path" 2>$devnull`; - chomp $line; - return '' unless $line; - my ($mode) = split(/\s+/, $line); - return $mode // ''; -} - -# Return the raw content of the blob at the given path in the -# patch-stack repo at HEAD. -sub git_blob -{ - my ($repo, $path) = @_; - - return `git -C "$repo" show "HEAD:$path" 2>$devnull`; -} - -# Collapse "." and ".." segments in a git-style forward-slash path -# without touching the filesystem -- the target may not exist as a -# real file on this platform (e.g. behind an unmaterialized symlink). -sub normalize_git_path -{ - my $path = shift; - my @out; - foreach my $part (split(m{/+}, $path)) - { - next if $part eq '' || $part eq '.'; - if ($part eq '..') { pop @out; } - else { push @out, $part; } - } - return join('/', @out); -} - -# Resolve a path in the patch-stack repo to its real file content, -# following git symlinks (mode 120000) by hand via git's tree/blob -# data rather than the checked-out working tree. Mirrors -# PGBuild::Modules::PatchStack::_resolve_patch_content: a patches repo -# may share an unmodified patch across branches via a symlink, and on -# platforms without filesystem symlink support (e.g. Windows without -# core.symlinks) a checked-out symlink is just a text file containing -# the link target, which git apply cannot use as patch content. Reading -# through git's plumbing instead works the same way regardless of how -# (or whether) the platform materializes real filesystem symlinks. -sub resolve_patch_content -{ - my ($repo, $path) = @_; - - $path = normalize_git_path($path); - - for (1 .. 5) - { - my $mode = git_mode($repo, $path); - return undef if $mode eq ''; - if ($mode eq '120000') - { - my $target = git_blob($repo, $path); - $target =~ s/\s+$//; - (my $dir = $path) =~ s{/[^/]*$}{}; - $path = normalize_git_path("$dir/$target"); - next; - } - return git_blob($repo, $path); - } - return undef; # symlink chain too deep -} - # Materialize a plain-file copy of a series subdirectory's series file # and the patches it lists, with any symlinks resolved to their real -# content (see resolve_patch_content), so the same checks run below -# operate on real patch content regardless of the platform's symlink -# support -- exactly what PatchStack.pm's quiltimport will actually -# see. Dies if the series file itself can't be resolved; a patch entry -# that can't be resolved is left out of $dest so the caller's normal -# "file not found" [MISS] handling reports it, mirroring how a broken -# patch stack fails quiltimport rather than silently substituting -# something else. Returns (resolved_dir, \@patches). +# content (see PGBuild::PatchSeries::series_manifest), so the same +# checks run below operate on real patch content regardless of the +# platform's symlink support -- exactly what PatchStack.pm's +# quiltimport will actually see. Dies if the series file itself can't +# be resolved; a patch entry that can't be resolved is left out of +# $dest so the caller's normal "file not found" [MISS] handling +# reports it, mirroring how a broken patch stack fails quiltimport +# rather than silently substituting something else. Returns +# (resolved_dir, \@patches). sub build_resolved_dir { - my ($repo, $sub, $resolved_root) = @_; - my $dest = "$resolved_root/$sub"; - mkpath($dest) unless -d $dest; - - my $series_content = resolve_patch_content($repo, "$sub/series"); - die "cannot resolve $sub/series\n" unless defined $series_content; + my ($stack_repo, $sub, $dest_root) = @_; + my $dest = "$dest_root/$sub"; - open(my $sfh, '>', "$dest/series") or die "writing $dest/series: $!\n"; - binmode $sfh; - print $sfh $series_content; - close $sfh; + my $manifest = series_manifest($stack_repo, $sub); + die "cannot resolve $sub/series\n" unless $manifest; - my @patches = parse_series("$dest/series"); - - foreach my $p (@patches) - { - my $name = $p->{name}; - my $content = resolve_patch_content($repo, "$sub/$name"); - next unless defined $content; - - # $name may itself be a relative path out of this subdirectory - # (e.g. "../master/foo.patch", used to share a patch unchanged - # across branches without a symlink), so its parent directory - # may not be $dest itself and may not exist yet. - my $target_file = "$dest/$name"; - mkpath(dirname($target_file)); - open(my $pfh, '>', $target_file) - or die "writing $target_file: $!\n"; - binmode $pfh; - print $pfh $content; - close $pfh; - } - return ($dest, \@patches); + # Unresolvable entries are simply left out of $dest; the caller's + # existing "file not found" handling reports them as [MISS]. + materialize_series($stack_repo, $manifest, $dest); + return ($dest, $manifest->{entries}); } # Default mode: dry-run each patch independently against the pristine @@ -456,34 +412,6 @@ sub print_diag return; } -sub parse_series -{ - my $path = shift; - open(my $fh, '<', $path) or die "cannot read $path: $!\n"; - my @out; - while (my $line = <$fh>) - { - chomp $line; - $line =~ s/^\s+//; - - # quilt convention: skip blanks and comments - next if $line =~ /^(#|$)/; - my @tok = split(/\s+/, $line); - my $name = shift @tok; - next unless defined $name && $name ne ''; - - # an optional -pN token sets this patch's strip level - my $strip; - foreach my $t (@tok) - { - $strip = $1 if $t =~ /^-p(\d+)$/; - } - push(@out, { name => $name, strip => $strip }); - } - close $fh; - return @out; -} - # Try git apply --check at the requested strip level. If the series # line gave no explicit level and -p1 fails, also try -p0, since plain # quilt diffs are sometimes generated without a/ b/ prefixes. @@ -520,7 +448,7 @@ sub usage my $code = shift // 0; my $fh = $code ? \*STDERR : \*STDOUT; print $fh <<'EOT'; -Usage: check_patch_stack.pl [options] +Usage: check_patch_stack.pl [options] [] Report which patches in which series apply cleanly to the matching //pgsql source tree. Each patch is dry-run @@ -535,6 +463,9 @@ sub usage (default 1) --sequential apply patches cumulatively in series order (in a scratch worktree), stopping at the first failure + --manifest print each series' resolved blob SHAs and the + identity the buildfarm uses to detect changes, + then exit; is not required --verbose show git apply diagnostics for failing patches --help this message diff --git a/docs/patch-stack.md b/docs/patch-stack.md index 41c508e..18cb42f 100644 --- a/docs/patch-stack.md +++ b/docs/patch-stack.md @@ -122,9 +122,17 @@ git commit -m "Add REL_17_STABLE stack skeleton" git push origin quilt ``` -Animals detect the change by comparing the git tree SHA of the subdirectory -against their recorded value. Any push that changes the subdirectory tree -triggers a rebuild automatically. +Animals detect the change by comparing an identity computed from the patches +themselves — the blob SHA of every patch the branch's `series` names, in +order, plus the blob SHA of the `series` file itself — against their +recorded value. Folding in the `series` blob is what makes a reordering or +a comment-only edit to `series` register, even though no patch file +changed. Any push that changes the content a branch would apply triggers +a rebuild of that branch automatically, +including when the changed patch lives in another branch's subdirectory and +is reached by a relative path (see "Sharing a patch between branches" +below). Patches a branch does not list contribute nothing, so a push +touching only one branch's stack does not rebuild the others. ## Removing or reordering patches @@ -175,23 +183,92 @@ git push origin quilt ``` +### Sharing a patch between branches + +When a patch applies unchanged to more than one branch, you can store it +once and reference it from the other branches' `series` files by relative +path, instead of keeping duplicate copies in sync: + +``` +# REL_16_STABLE/series +0001-local-fix.patch +../master/0002-cve.patch +``` + +The referenced patch is read from `master/` at build time, so editing it +updates every branch that names it, and every one of those branches is +rebuilt on the next run. Keeping copies instead means remembering to update +each one. + +Symlinks into another branch's subdirectory also work and are still +supported, but relative paths are preferred: they are visible in the +`series` file itself, and they behave identically on platforms without +filesystem symlink support. + + ## Viewing what was applied on the web dashboard Each run writes a `patch_stack.log` artifact — separate from the main -checkout log — recording the series identity, whether it applied -cleanly, and the filename/subject of every patch it attempted. This -travels to the server the same way `githead.log` does, and the server -renders it as its own table on the build's report page (with an -added/removed diff against the previous run when the series changed), -instead of it being buried in the raw `SCM-checkout` log text. There's +checkout log — recording the series identity, the patches-repo commit that +was used, whether the series applied cleanly, and the filename, content +SHA, and subject of every patch it attempted. The server renders it as its +own table on the build's report page, with a diff against the previous run +showing patches added, removed, **and modified** — a patch whose content +changed under an unchanged filename is now visible, which it was not +before. The stack identity and patches-repo commit are shown alongside, so +a given run can be tied to an exact revision of the patches repo. There's nothing to configure for this — it's automatic whenever `PatchStack` is enabled — but it means the `series` file's patch order and each patch's `Subject:` line are now user-visible, so keep them meaningful. ## Verifying the repository locally -Before pushing, you can verify that the series applies cleanly by running -`git quiltimport` in a throw-away clone of PostgreSQL: +Before pushing, `check_patch_stack.pl` (in the buildfarm client repo) +reports whether each series applies to the matching source tree: + +```sh +check_patch_stack.pl --sequential /path/to/patches.git /path/to/buildroot +``` + +`--manifest` instead checks that every `series` entry resolves to a patch +that is actually there, and prints what each one resolves to. It reads only +the patches repository, so unlike the modes above it needs no buildroot and +no PostgreSQL source tree: + +```sh +check_patch_stack.pl --manifest /path/to/patches.git +``` + +``` +=== series: REL_16_STABLE === + series 4f2a9c1 + 0001-local-fix.patch 8b3e77d + ../master/0002-cve.patch a91c204 -> master/0002-cve.patch + patch_stack_id: 3d9f1ab400112233445566778899aabbccddeeff +``` + +The per-patch SHAs above are shown truncated to 7 characters, as `git log` +does; `patch_stack_id` is printed in full (40 characters), since that is +the exact value animals compare and there is no shorter form of it to +recognize. + +An entry whose patch file is missing is reported as `(missing)` and the exit +status is 1. That is what this mode is for: a `series` naming a file that +is not there means the patch is not tested on that branch, and the mistake +is easy to make and easy to miss — a patch renamed but not renamed in +`series`, or added to one branch's `series` under the wrong name. Run it +before pushing and the farm never sees the mistake. + +The `->` column shows where an entry resolved when that differs from the +entry itself, so shared patches are visible at a glance, and two branches +carrying the same blob SHA are demonstrably testing the same content. + +`--manifest` reads the patches repo at its current `HEAD`, so uncommitted +changes there are invisible to it — including right before you push, which +is exactly when you are most likely to run it. Commit first, then run +`--manifest`, to be sure it's reporting what you're about to push. + +You can also apply the series by hand in a throw-away clone: ```sh git clone --branch REL_17_STABLE https://git.postgresql.org/git/postgresql.git /tmp/pg-test From 4ee0fde841bc9fdd417f47e863eadd1560b2740a Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Tue, 11 Aug 2026 12:09:54 -0400 Subject: [PATCH 3/6] Apply the patch series with git apply, not quiltimport 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. --- PGBuild/Modules/PatchStack.pm | 117 ++++++++++++++------------------- PGBuild/Utils.pm | 8 ++- check_patch_stack.pl | 120 +++++++++++++++++----------------- docs/patch-stack.md | 39 ++++++++--- 4 files changed, 144 insertions(+), 140 deletions(-) diff --git a/PGBuild/Modules/PatchStack.pm b/PGBuild/Modules/PatchStack.pm index b2bf148..718ec75 100644 --- a/PGBuild/Modules/PatchStack.pm +++ b/PGBuild/Modules/PatchStack.pm @@ -19,11 +19,16 @@ git repo, with a per-Postgres-branch subdirectory holding a C file and the patch files referenced by it (one per line, applied in order). -Patches are imported with C, which creates a real -commit per patch and preserves authorship. This means the patch -files must carry C and C headers (i.e. be produced -by C or equivalent) so that C can -extract the author. Bare diffs will not import. +Patches are applied with C, one at a time in series order, +stopping at the first entry that is missing or fails to apply. No +commits are created and C never moves; the working tree is +restored after the run. + +Patch files should still carry C and C headers (i.e. +be produced by C or equivalent), because the subject +is extracted with C for the build report. Unlike +C, which this replaced, a bare diff will now apply -- +it will simply be reported under its file name. A patch shared unchanged across branches may be referenced rather than copied, as a C entry giving a relative path into another @@ -38,10 +43,11 @@ filesystem symlink support (and of whether C<..> in the path has been normalized -- a raw C silently returns empty content for an unnormalized path instead of erroring, so paths are normalized before being resolved). A patch listed in a branch's -C with no entry at all in the patches branch is a broken patch -stack for that branch: it is logged and left out of the import, which -then fails loudly rather than silently applying a substitute from -elsewhere. +C with no entry in the patches branch is a broken patch stack +for that branch: the run stops there and reports C, +naming the entry. C, used before this, skipped such +an entry and exited zero, so a branch could build and report a green +result with a patch missing from its stack. =head2 RUN TRIGGER @@ -105,7 +111,7 @@ package PGBuild::Modules::PatchStack; use PGBuild::Options; use PGBuild::SCM; use PGBuild::Utils qw(:DEFAULT $st_prefix $branch_root $devnull); -use PGBuild::PatchSeries qw(series_manifest materialize_series); +use PGBuild::PatchSeries qw(series_manifest materialize_series apply_series); use File::Path qw(mkpath); @@ -161,7 +167,6 @@ sub setup patches_id => '', stack_commit => '', manifest => undef, - pre_apply_sha => '', }; bless($self, $class); @@ -326,57 +331,32 @@ sub _apply_patches $self->{series_patches} = $self->_log_series($log, $patchdir); - # Capture the upstream HEAD before importing so cleanup (and - # error recovery below) can rewind past the commits quiltimport - # is about to create. - my $sha = `git -C $srcdir rev-parse --verify --quiet HEAD 2>$devnull`; - chomp $sha; - if ($? >> 8 || $sha eq '') - { - push(@$log, "$MODULE: cannot determine HEAD of $srcdir\n"); - $self->{series_status} = 'broken'; - return 0; - } - $self->{pre_apply_sha} = $sha; - - # quiltimport uses git-am internally; abort any interrupted state - # left by a previous run before starting fresh. A rebase-apply - # directory left behind by a run that failed before git-am finished - # writing its full state (e.g. an empty-patch mailsplit error) can - # be incomplete enough that "am --abort" itself silently fails to - # remove it, so fall back to forcibly clearing it -- otherwise the - # next quiltimport's internal git-am fails at mkdir because the - # directory still exists. - if (-d "$srcdir/.git/rebase-apply") - { - push(@$log, "$MODULE: aborting stale rebase-apply state\n"); - run_log("git -C $srcdir am --abort"); - if (-d "$srcdir/.git/rebase-apply") - { - push(@$log, - "$MODULE: am --abort left rebase-apply state behind," - . " removing it directly\n"); - rmtree("$srcdir/.git/rebase-apply"); - } - } + # A partially applied series must still be cleaned up, so record + # that the tree has been touched before the first patch lands. + $self->{applied} = 1; - push(@$log, "$MODULE: importing patch series from $patchdir\n"); + push(@$log, "$MODULE: applying patch series from $patchdir\n"); - my @out = run_log(qq{git -C $srcdir quiltimport --patches "$patchdir"}); - my $status = $? >> 8; - push(@$log, "------ quiltimport (status=$status) ------\n", @out); + my $result = apply_series($self->{manifest}, $patchdir, $srcdir, undef); - if ($status) + # git apply reports context reduction on a SUCCESSFUL apply, so + # carry the output of every entry, not just a failing one. + foreach my $a (@{ $result->{applied} }) { - # quiltimport leaves a partial commit history on failure; - # rewind to a known state so a later cleanup or rerun starts - # from the upstream tip rather than a half-applied series. - run_log("git -C $srcdir reset --hard --quiet $sha"); + push(@$log, " $a->{name}\n"); + push(@$log, $a->{output}) if defined $a->{output} && $a->{output} ne ''; + } + + unless ($result->{ok}) + { + my $f = $result->{failure}; + push(@$log, + "$MODULE: $f->{reason}: $sub/$f->{name}\n", + defined $f->{detail} ? $f->{detail} : ''); $self->{series_status} = 'broken'; return 0; } - $self->{applied} = 1; $self->{series_status} = 'applied'; return 1; } @@ -554,27 +534,26 @@ sub cleanup { my $self = shift; - return unless $self->{applied}; - # When rm_worktrees is on the END block has already wiped the - # worktree files; running git reset here would just resurrect them. - # The imported commits left at HEAD are harmless: the next run's - # SCM update (PGBuild::SCM::_update_target) restores the worktree - # with "git checkout ." and then "git reset --hard origin/", - # which discards them and returns the tree to pristine upstream - # before any build happens. + # worktree files; running git commands here would just resurrect + # them. The next run's SCM update restores the tree anyway. return if $self->{bfconf}->{rm_worktrees}; my $srcdir = $self->{srcdir}; return unless -d "$srcdir/.git"; - # Reset to the upstream tip captured before quiltimport so the - # imported commits are discarded and the worktree is back to - # pristine upstream state for the next run. - my $target = $self->{pre_apply_sha} || 'HEAD'; - print time_str(), "$MODULE: resetting $srcdir to $target\n" - if $verbose > 1; - run_log("git -C $srcdir reset --hard --quiet $target"); + return unless $self->{applied}; + + # HEAD never moved, so this restores pristine upstream. Because the + # patches were applied with --index, it also removes files they + # added. The clean sweeps anything that escaped: the buildfarm never + # builds in the source tree, so nothing untracked there is ours to + # keep, and one leaked file would otherwise be compiled on every + # subsequent run. -fd rather than -fdx: ignored files are left + # alone. + print time_str(), "$MODULE: restoring $srcdir\n" if $verbose > 1; + run_log("git -C $srcdir reset --hard --quiet HEAD"); + run_log("git -C $srcdir clean -qfd"); return; } diff --git a/PGBuild/Utils.pm b/PGBuild/Utils.pm index e7fbfad..f0a74c0 100644 --- a/PGBuild/Utils.pm +++ b/PGBuild/Utils.pm @@ -68,10 +68,16 @@ sub send_result # something like IPC::RUN but without requiring it, as some installations # lack it. +# $filedir is optional. Omitted, the location is inferred from the +# buildfarm run's globals, which is what every in-tree caller wants. +# Passing it explicitly lets code that runs outside a buildfarm run -- +# check_patch_stack.pl, via PGBuild::PatchSeries -- use run_log at all: +# $branch_root, $st_prefix and $logdirname are set by run_build.pl and +# are undef otherwise, so the inferred path would collapse to "/". sub run_log { my $command = shift; - my $filedir = "$branch_root/$st_prefix$logdirname"; + my $filedir = shift || "$branch_root/$st_prefix$logdirname"; mkpath($filedir); my $file = "$filedir/lastcommand.log"; my $stfile = "$filedir/laststatus"; diff --git a/check_patch_stack.pl b/check_patch_stack.pl index 4c6a7ac..87a5fba 100755 --- a/check_patch_stack.pl +++ b/check_patch_stack.pl @@ -25,15 +25,23 @@ =head1 check_patch_stack.pl Because every patch is checked against the unpatched base, a patch that only applies on top of an earlier patch in the series will be reported as failing here even though it would apply in a real -sequential C. +sequential run. With C<--sequential> the patches are instead applied cumulatively, in series order, inside a throwaway C checked out from the tree's HEAD (so the real source tree is never touched and the base is -pristine regardless of the working tree's state). Application stops at -the first patch that fails to apply -- mirroring C, which -also halts on the first bad patch -- and the remaining patches are -reported as skipped. +pristine regardless of the working tree's state). That mode calls +C, the same code the buildfarm +itself uses, so what it reports is what an animal will do rather than +an approximation of it. Application stops at the first patch that is +missing or fails to apply, and the remaining patches are reported as +skipped. + +Note that C<--sequential> therefore does not fall back from C<-p1> to +C<-p0> the way the default mode does: C applies at the +strip level the C line gives and does not guess. The default +mode keeps the fallback, since it is checking each patch in isolation +rather than predicting a real run. A series entry may reference a patch shared unchanged with another branch's subdirectory rather than a copy, by giving a relative path -- @@ -81,7 +89,7 @@ =head1 USAGE use FindBin; use lib $FindBin::RealBin; -use PGBuild::PatchSeries qw(series_manifest materialize_series); +use PGBuild::PatchSeries qw(series_manifest materialize_series apply_series); use PGBuild::Utils qw($devnull); my @only_branches; @@ -233,7 +241,7 @@ =head1 USAGE $tot_series++; - my ($resolved_dir, $patches) = + my ($resolved_dir, $patches, $manifest) = eval { build_resolved_dir($repo, $sub, $resolved_root) }; if ($@) { @@ -245,7 +253,7 @@ =head1 USAGE my ($clean, $fail, $miss) = $sequential - ? test_sequential($tree, $resolved_dir, \@patches) + ? test_sequential($tree, $resolved_dir, \@patches, $manifest) : test_independent($tree, $resolved_dir, \@patches); printf " %d patches: %d clean, %d failed, %d missing\n\n", @@ -268,13 +276,13 @@ =head1 USAGE # and the patches it lists, with any symlinks resolved to their real # content (see PGBuild::PatchSeries::series_manifest), so the same # checks run below operate on real patch content regardless of the -# platform's symlink support -- exactly what PatchStack.pm's -# quiltimport will actually see. Dies if the series file itself can't -# be resolved; a patch entry that can't be resolved is left out of -# $dest so the caller's normal "file not found" [MISS] handling -# reports it, mirroring how a broken patch stack fails quiltimport -# rather than silently substituting something else. Returns -# (resolved_dir, \@patches). +# platform's symlink support -- exactly what PatchStack.pm will +# actually apply. Dies if the series file itself can't be resolved; a +# patch entry that can't be resolved is left out of $dest so the +# caller's normal "file not found" [MISS] handling reports it, which +# is also what apply_series does with a missing entry: stop and say so, +# rather than silently substituting something else or skipping past it. +# Returns (resolved_dir, \@patches, $manifest). sub build_resolved_dir { my ($stack_repo, $sub, $dest_root) = @_; @@ -286,7 +294,7 @@ sub build_resolved_dir # Unresolvable entries are simply left out of $dest; the caller's # existing "file not found" handling reports them as [MISS]. materialize_series($stack_repo, $manifest, $dest); - return ($dest, $manifest->{entries}); + return ($dest, $manifest->{entries}, $manifest); } # Default mode: dry-run each patch independently against the pristine @@ -326,15 +334,17 @@ sub test_independent return ($clean, $fail, $miss); } -# --sequential mode: apply the patches cumulatively, in order, inside a -# throwaway worktree checked out from the tree's HEAD. Stops at the -# first failure (or missing file), the way quiltimport does, and marks -# the rest skipped. Returns (clean, failed, missing); a stop counts the -# offending patch as failed/missing and the rest as neither. +# --sequential: apply the series cumulatively into a throwaway worktree, +# using the same apply_series() the buildfarm uses, so what this +# validates is exactly what an animal will do. Returns +# (clean, failed, missing). +# +# Unlike test_independent this does NOT fall back from -p1 to -p0: +# apply_series deliberately does not guess strip levels, and the point +# of this mode is to match the farm rather than to be forgiving. sub test_sequential { - my ($tree, $dir, $patches) = @_; - my ($clean, $fail, $miss) = (0, 0, 0); + my ($tree, $dir, $patches, $manifest) = @_; my $scratch = tempdir("patchstack.XXXXXX", TMPDIR => 1, CLEANUP => 1); my $wt = "$scratch/wt"; @@ -346,53 +356,41 @@ sub test_sequential return (0, 0, 0); } - my $stopped = 0; - foreach my $p (@$patches) - { - my $name = $p->{name}; - - if ($stopped) - { - printf " %-7s %s\n", '[SKIP]', "$name (earlier patch failed)"; - next; - } + my $logdir = "$scratch/log"; + my $result = apply_series($manifest, $dir, $wt, $logdir); - my $strip = defined $p->{strip} ? $p->{strip} : $default_strip; - my $file = "$dir/$name"; + my ($clean, $fail, $miss) = (0, 0, 0); + my %done; + foreach my $a (@{ $result->{applied} }) + { + $done{ $a->{name} } = 1; + $clean++; + printf " %-7s %s\n", '[ ok ]', $a->{name}; + print_diag($a->{output}) + if $verbose && defined $a->{output} && $a->{output} ne ''; + } - unless (-f $file) + if (!$result->{ok}) + { + my $f = $result->{failure}; + $done{ $f->{name} } = 1; + if ($f->{reason} eq 'missing') { - printf " %-7s %s\n", '[MISS]', "$name (file not found)"; + printf " %-7s %s\n", '[MISS]', "$f->{name} (file not found)"; $miss++; - $stopped = 1; - next; - } - - # pick the working strip level without mutating the tree, then - # apply for real at that level so later patches build on it. - my ($ok, $used_strip, $err) = check_apply($wt, $file, $strip); - unless ($ok) - { - printf " %-7s %s\n", '[FAIL]', $name; - $fail++; - $stopped = 1; - print_diag($err); - next; } - - my $aerr = `git -C "$wt" apply -p$used_strip -- "$file" 2>&1`; - if (($? >> 8) != 0) + else { - printf " %-7s %s\n", '[FAIL]', "$name (apply failed)"; + printf " %-7s %s\n", '[FAIL]', $f->{name}; $fail++; - $stopped = 1; - print_diag($aerr); - next; + print_diag($f->{detail}); } + } - my $note = $used_strip == $strip ? '' : " (-p$used_strip)"; - printf " %-7s %s%s\n", '[ ok ]', $name, $note; - $clean++; + foreach my $p (@$patches) + { + next if $done{ $p->{name} }; + printf " %-7s %s\n", '[SKIP]', "$p->{name} (earlier patch failed)"; } # Remove the worktree registration; CLEANUP unlinks the files. diff --git a/docs/patch-stack.md b/docs/patch-stack.md index 18cb42f..b0e93a7 100644 --- a/docs/patch-stack.md +++ b/docs/patch-stack.md @@ -58,9 +58,9 @@ order. ## Patch file format -Patches **must** carry mail-style headers (`From:`, `Date:`, `Subject:`) so -that `git mailinfo` can extract the author and subject. The standard way to -produce them is `git format-patch`: +Patches should still carry mail-style headers (`From:`, `Date:`, `Subject:`) +so that `git mailinfo` can extract the subject for the build report. The +standard way to produce them is `git format-patch`: ```sh # Single commit: @@ -70,8 +70,9 @@ git format-patch -1 git format-patch .. ``` -Bare unified diffs (output of `diff -u` or `git diff`) will not work — they -lack the authorship information that the importer requires. +Bare unified diffs (output of `diff -u` or `git diff`) will apply — the +series is applied with `git apply`, not imported — but they carry no +subject, so they are reported under their filename instead. ## Setting up the repository from scratch @@ -166,6 +167,11 @@ Until the stack is rebased, animals will report `PatchStackBroken` for that branch rather than a generic build failure, making it easy to tell "stack needs maintenance" apart from "PostgreSQL broke something." +A `series` entry naming a patch file that is not present now stops the run and reports +`PatchStackBroken`, naming the entry. Until this changed the entry was +silently skipped and the branch built and reported green without it, so a +typo in `series` could hide a patch from testing indefinitely. + ## Supporting multiple PostgreSQL branches @@ -205,6 +211,17 @@ supported, but relative paths are preferred: they are visible in the `series` file itself, and they behave identically on platforms without filesystem symlink support. +Note that a shared patch is applied with more tolerance for drifting +context than one stored in the branch's own subdirectory. A patch +written for a branch should apply to that branch exactly; if it stops +doing so, upstream has moved beneath the stack and you want to be told, +not to have it quietly absorbed. A patch written against `master` and +reached from a stable branch is a different case: the surrounding code +legitimately differs, so it is allowed to match on less context. If a +shared patch drifts far enough that even that fails, it has stopped +being the same patch for both branches and wants splitting into +per-branch copies. + ## Viewing what was applied on the web dashboard @@ -268,12 +285,16 @@ changes there are invisible to it — including right before you push, which is exactly when you are most likely to run it. Commit first, then run `--manifest`, to be sure it's reporting what you're about to push. -You can also apply the series by hand in a throw-away clone: +You can also apply the series by hand the same way an animal does: ```sh git clone --branch REL_17_STABLE https://git.postgresql.org/git/postgresql.git /tmp/pg-test -git -C /tmp/pg-test quiltimport --patches /path/to/patches.git/REL_17_STABLE +cd /tmp/pg-test +while read -r patch rest; do + case "$patch" in ''|\#*) continue;; esac + git apply --index -C3 "/path/to/patches.git/REL_17_STABLE/$patch" || break +done < /path/to/patches.git/REL_17_STABLE/series ``` -A zero exit code means every patch applied; a non-zero exit code (plus the -reject output) shows what needs attention before you push. +`check_patch_stack.pl --sequential` does exactly this, and reports which +patch failed. From 62d9fb928dd33a638af5765d3a189b8a32450ec8 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Mon, 31 Aug 2026 09:24:14 -0400 Subject: [PATCH 4/6] Stop reading a strip level out of a series line's comment 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 --- PGBuild/PatchSeries.pm | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/PGBuild/PatchSeries.pm b/PGBuild/PatchSeries.pm index cfdc3a0..4789954 100644 --- a/PGBuild/PatchSeries.pm +++ b/PGBuild/PatchSeries.pm @@ -123,6 +123,12 @@ sub resolve_patch_path # the leading strip, split(/\s+/, " 0001.patch") yields an empty first # field and the entry is silently dropped; the trailing strip keeps a # CRLF series file from producing names with a stray carriage return. +# +# Scanning for the strip level stops at a "#" token: everything from +# there on is comment, which is where the security stacks keep a redmine +# id. Reading past it took a "-p2" written in a comment as a strip level +# -- a level neither git quiltimport nor a release wrap would use, so +# the farm would have applied the patch somewhere the wrap did not. sub parse_series { my $content = shift; @@ -143,7 +149,11 @@ sub parse_series my $strip; foreach my $t (@tok) { - $strip = $1 if $t =~ /^-p(\d+)$/; + last if $t =~ /^#/; + if ($t =~ /^-p(\d+)$/) + { + $strip = $1; + } } push(@out, { name => $name, strip => $strip }); } From 24d346880de03201f91f18b6f1e8c7c1c41c38f8 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Mon, 31 Aug 2026 09:24:46 -0400 Subject: [PATCH 5/6] Apply the patch series with the patches repo's own driver 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 --- PGBuild/Modules/PatchStack.pm | 238 +++++++++++++++++++++++++++++++++- 1 file changed, 233 insertions(+), 5 deletions(-) diff --git a/PGBuild/Modules/PatchStack.pm b/PGBuild/Modules/PatchStack.pm index 718ec75..80bf406 100644 --- a/PGBuild/Modules/PatchStack.pm +++ b/PGBuild/Modules/PatchStack.pm @@ -19,7 +19,11 @@ git repo, with a per-Postgres-branch subdirectory holding a C file and the patch files referenced by it (one per line, applied in order). -Patches are applied with C, one at a time in series order, +The series is applied by the driver script at the top of the patches +repository when there is one, and otherwise by this module's own +C loop -- see L below. + +The built-in loop applies the patches one at a time in series order, stopping at the first entry that is missing or fails to apply. No commits are created and C never moves; the working tree is restored after the run. @@ -49,6 +53,48 @@ naming the entry. C, used before this, skipped such an entry and exited zero, so a branch could build and report a green result with a patch missing from its stack. +=head2 DRIVER + +A patch stack that a release wrap will apply should be applied here by +the same code the wrap runs, so that what the animal tests is what the +wrap produces rather than something resembling it. That code is a +script at the top of the patches repository, taking the directory +holding a C file as its only argument, named C +unless the C config setting says otherwise. + +The name is really an interface between the patches repository and +every animal that clones it, not an implementation detail of either, so +the default is what the security farm relies on and the setting exists +for a repository that has good reason to differ. Configuring one is +also a statement that the repository has an applier of its own: if the +named script is not there the run reports C, rather +than falling back and reporting a green build for a stack applied by +something other than what the config asked for. Only the default name +is allowed to be absent. + +C left undef is the default rather than a setting of its own, +as for C. Set to an empty string or C<0> it asks for the +built-in loop and skips the lookup entirely. + +The script is run in the source tree -- clean and at upstream's tip at +that point -- and is handed the materialized copy of the series +described above, so a shared entry or a symlink is an ordinary file by +the time it sees one, exactly as on wrap day against a real checkout. + +It applies with C, so C moves and a commit is created per +patch. C resets to the commit the tree was on beforehand, and +first clears any C state left behind by an entry that failed, +which a reset does not remove. Any nonzero exit is reported as +C. + +When no driver is configured and the patches repository carries no +script under the default name, the built-in C loop runs +instead. That is not a transition measure: +C is a general module, and a quilt-style repository that no +release wrap consumes has no reason to carry a driver. Which of the two +applied the series is recorded in C as +C. + =head2 RUN TRIGGER The module forces a run whenever the identity of this branch's patch @@ -92,6 +138,11 @@ In the animal's C: repo => 'https://example.org/git/some-patches.git', patches_branch => 'quilt', # default: quilt local_repo => undef, # default: /patch_stack. + # the applier the patches repo supplies for itself. Unset or + # undef, 'import-series.pl' is used if the repo has one. Named, + # the named script must be there or the run reports broken. Set + # to '' to use the built-in apply loop and look for nothing. + driver => 'import-series.pl', subdir => { # map Postgres branch name to subdirectory name in the # patches branch. Default for unlisted branches is the @@ -113,6 +164,7 @@ use PGBuild::SCM; use PGBuild::Utils qw(:DEFAULT $st_prefix $branch_root $devnull); use PGBuild::PatchSeries qw(series_manifest materialize_series apply_series); +use Cwd qw(getcwd); use File::Path qw(mkpath); use strict; @@ -120,6 +172,18 @@ use warnings; (my $MODULE = __PACKAGE__) =~ s/PGBuild::Modules:://; +# The name the applier a patches repository supplies for itself is +# looked for under, at the top of the repository, when the animal's +# config does not name one. See L. +my $DRIVER = 'import-series.pl'; + +# What each of the driver's documented exit codes means, for the log. +my %DRIVER_EXIT = ( + 1 => 'patch files named in series are missing', + 2 => 'usage or setup error', + 3 => 'a patch failed to apply', +); + our ($VERSION); $VERSION = 'REL_21'; my $hooks = { @@ -129,6 +193,27 @@ my $hooks = { 'cleanup' => \&cleanup, }; +# The driver to look for, and whether its absence is an error: a driver +# named in the config has to be there, because naming one is a statement +# that this 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 a green build. +# Left at the default, the name is looked for and its absence is +# ordinary. +# +# undef is the default rather than a setting of its own, as it is for +# local_repo: an undef in a generated config means the animal has no +# opinion. An empty string or a 0 is an opinion, and asks for the +# built-in loop. +sub _driver_config +{ + my $stackconf = shift; + + my $driver = $stackconf->{driver}; + $driver = $DRIVER unless defined $driver; + return ($driver, ($driver && defined $stackconf->{driver}) ? 1 : 0); +} + sub setup { my $class = __PACKAGE__; @@ -153,6 +238,8 @@ sub setup my $local_repo = $stackconf->{local_repo} || "$buildroot/patch_stack.$conf->{animal}"; + my ($driver, $driver_required) = _driver_config($stackconf); + my $self = { buildroot => $buildroot, pgbranch => $branch, @@ -163,10 +250,14 @@ sub setup patches_branch => $stackconf->{patches_branch} || 'quilt', subdir => $subdir, local_repo => $local_repo, + driver => $driver, + driver_required => $driver_required, applied => 0, patches_id => '', stack_commit => '', manifest => undef, + applier => '', + src_head => '', }; bless($self, $class); @@ -310,7 +401,6 @@ sub _apply_patches my $log = shift; my $local = $self->{local_repo}; my $sub = $self->{subdir}; - my $srcdir = $self->{srcdir}; my $patchdir = "$local/$sub"; unless (-f "$patchdir/series") @@ -335,6 +425,123 @@ sub _apply_patches # that the tree has been touched before the first patch lands. $self->{applied} = 1; + my $name = $self->{driver}; + if ($name) + { + my $driver = "$local/$name"; + return $self->_apply_with_driver($log, $patchdir, $driver) + if -f $driver; + + if ($self->{driver_required}) + { + push(@$log, "$MODULE: no $name at the top of $self->{repo}\n"); + $self->{series_status} = 'broken'; + return 0; + } + } + + return $self->_apply_builtin($log, $patchdir); +} + +# Hand the series to the patches repository's own driver, which is what +# a release wrap runs, so the farm exercises the wrap's applier rather +# than an imitation of it. Everything the two callers do differently -- +# choosing branches, fetching, resetting to upstream, deciding whether +# to run at all -- stays out of the driver and on this side. +# +# The driver is run in the source tree because that is the tree it acts +# on: it checks that it is in a clean git working tree and applies with +# "git am", both of which read the current directory. +sub _apply_with_driver +{ + my $self = shift; + my $log = shift; + my $patchdir = shift; + my $driver = shift; + my $srcdir = $self->{srcdir}; + + $self->{applier} = $self->{driver}; + + # git am moves HEAD, unlike the built-in loop, so record where the + # tree started: that, not HEAD, is what cleanup() resets to. + my $base = `git -C "$srcdir" rev-parse --verify --quiet HEAD 2>$devnull`; + chomp $base; + $self->{src_head} = $base; + + push(@$log, "$MODULE: applying patch series from $patchdir with $driver\n"); + + # A commit per patch needs an identity to commit as. Supply one only + # when git cannot find its own: an animal's git is often unconfigured, + # and git's guess from the host name fails outright on a host with no + # domain, but an owner who has configured an identity should keep it. + # The env vars would otherwise override the config. A patch's own + # From: still wins either way -- git am sets the author fields from + # the patch before it commits. + my %ident; + my $have_ident = `git -C "$srcdir" var GIT_COMMITTER_IDENT 2>$devnull`; + if ($? >> 8 || !$have_ident) + { + my $animal = $self->{bfconf}->{animal} || 'buildfarm'; + %ident = ( + GIT_COMMITTER_NAME => 'PostgreSQL Buildfarm', + GIT_COMMITTER_EMAIL => "$animal\@buildfarm.invalid", + GIT_AUTHOR_NAME => 'PostgreSQL Buildfarm', + GIT_AUTHOR_EMAIL => "$animal\@buildfarm.invalid", + ); + } + local @ENV{ keys %ident } = values %ident; + + # Run the driver with the perl we are running under rather than + # relying on the shebang line and an execute bit, neither of which + # survives a checkout on every platform an animal runs on. + my $here = getcwd(); + unless (chdir $srcdir) + { + push(@$log, "$MODULE: cannot chdir to $srcdir: $!\n"); + $self->{series_status} = 'broken'; + return 0; + } + my @out = eval { run_log(qq{"$^X" "$driver" "$patchdir"}) }; + my $err = $@; + my $status = $? >> 8; + chdir $here; + + if ($err) + { + push(@$log, "$MODULE: running $self->{driver}: $err"); + $self->{series_status} = 'broken'; + return 0; + } + + push(@$log, @out); + + if ($status) + { + my $why = $DRIVER_EXIT{$status} || 'unknown failure'; + push(@$log, + "$MODULE: $self->{driver} exited $status ($why)" + . " on $self->{subdir}/series\n"); + $self->{series_status} = 'broken'; + return 0; + } + + $self->{series_status} = 'applied'; + return 1; +} + +# Apply the series ourselves, for a patches repository that carries no +# driver. See L for why this is a permanent path and not a +# fallback waiting to be removed. +sub _apply_builtin +{ + my $self = shift; + my $log = shift; + my $patchdir = shift; + my $sub = $self->{subdir}; + my $srcdir = $self->{srcdir}; + + $self->{applier} = 'git-apply'; + push(@$log, "$MODULE: applying patch series from $patchdir\n"); my $result = apply_series($self->{manifest}, $patchdir, $srcdir, undef); @@ -405,6 +612,15 @@ sub _patch_stack_log_lines . (defined $self->{series_status} ? $self->{series_status} : '') . "\n"); + # Which applier ran: the patches repository's own driver, named, or + # the built-in loop as "git-apply". Empty when nothing was applied. + # A server that predates the key ignores it, as it does any other + # key it does not know. + push(@lines, + "patch_stack_applier: " + . (defined $self->{applier} ? $self->{applier} : '') + . "\n"); + foreach my $p (@{ $self->{series_patches} || [] }) { push( @@ -544,15 +760,27 @@ sub cleanup return unless $self->{applied}; - # HEAD never moved, so this restores pristine upstream. Because the - # patches were applied with --index, it also removes files they + # Reset to the commit the tree was on before the series was applied. + # The built-in loop creates no commits and never moves HEAD, so + # there that is HEAD itself; the driver applies with git am, which + # commits, so there it is the recorded starting point. + # + # A driver run that stopped partway also leaves git am state behind, + # which a reset does not clear, so back that out first. The abort + # exits nonzero when no am is in progress, which is the ordinary + # case, so its status is ignored. + # + # Resetting restores pristine upstream. Because the patches were + # applied with --index (or committed), it also removes files they # added. The clean sweeps anything that escaped: the buildfarm never # builds in the source tree, so nothing untracked there is ours to # keep, and one leaked file would otherwise be compiled on every # subsequent run. -fd rather than -fdx: ignored files are left # alone. print time_str(), "$MODULE: restoring $srcdir\n" if $verbose > 1; - run_log("git -C $srcdir reset --hard --quiet HEAD"); + my $base = $self->{src_head}; + run_log("git -C $srcdir am --abort") if $base; + run_log("git -C $srcdir reset --hard --quiet " . ($base || 'HEAD')); run_log("git -C $srcdir clean -qfd"); return; } From 900eb65ef18948e125752bf3f47927af67a66e76 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Mon, 31 Aug 2026 09:24:46 -0400 Subject: [PATCH 6/6] Check the driver's reading of the series against our own 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 --- PGBuild/Modules/PatchStack.pm | 103 ++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/PGBuild/Modules/PatchStack.pm b/PGBuild/Modules/PatchStack.pm index 80bf406..07309eb 100644 --- a/PGBuild/Modules/PatchStack.pm +++ b/PGBuild/Modules/PatchStack.pm @@ -81,6 +81,18 @@ that point -- and is handed the materialized copy of the series described above, so a shared entry or a symlink is an ordinary file by the time it sees one, exactly as on wrap day against a real checkout. +Before anything is applied, the driver is run with C<--list>, which +prints its own reading of the C file and exits without touching +anything: one line per entry, in order, the name and the strip level +separated by a tab, C<-> where the line gave no level. That is compared +against the reading behind the manifest, and a disagreement is reported +with both readings and nothing applied. Two programs parse the same +file -- we do because the rebuild digest and the list of blobs to +materialize come out of it, the driver does because its reading is what +gets applied -- and nothing else would notice if they differed. A driver +that does not understand C<--list> is reported the same way, rather than +skipping the check. + It applies with C, so C moves and a commit is created per patch. C resets to the commit the tree was on beforehand, and first clears any C state left behind by an entry that failed, @@ -462,6 +474,8 @@ sub _apply_with_driver $self->{applier} = $self->{driver}; + return 0 unless $self->_check_series_agreement($log, $patchdir, $driver); + # git am moves HEAD, unlike the built-in loop, so record where the # tree started: that, not HEAD, is what cleanup() resets to. my $base = `git -C "$srcdir" rev-parse --verify --quiet HEAD 2>$devnull`; @@ -529,6 +543,95 @@ sub _apply_with_driver return 1; } +# Check the driver's reading of the series against our own before +# anything is applied. +# +# Two programs parse the same series file. We parse it because the +# digest that decides whether this branch rebuilds, and the list of +# blobs to materialize, both come out of that reading; the driver parses +# it because that reading is what gets applied. Nothing would otherwise +# notice if the two read a line differently, and they have: a strip +# level written past the "#" that starts a comment -- where the security +# stacks keep a redmine id -- was a level to us and comment text to +# quilt, so the farm would have applied a patch at a level no wrap would +# use and reported a green build for it. +# +# The driver's --list prints its reading and exits without touching +# anything: one line per entry, in series order, the name and the strip +# level separated by a tab, with "-" 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 driver that does not understand --list exits with a usage error, and +# that is a broken stack too. A check that quietly skips itself when the +# other side is unfamiliar is the failure this exists to prevent, and +# --list is part of the interface from the first driver onwards. +sub _check_series_agreement +{ + my $self = shift; + my $log = shift; + my $patchdir = shift; + my $driver = shift; + my $name = $self->{driver}; + + my @out = eval { run_log(qq{"$^X" "$driver" --list "$patchdir"}) }; + if ($@) + { + push(@$log, "$MODULE: running $name --list: $@"); + $self->{series_status} = 'broken'; + return 0; + } + if ($? >> 8) + { + push(@$log, + "$MODULE: $name --list failed, so its reading of" + . " $self->{subdir}/series cannot be checked against ours\n", + @out); + $self->{series_status} = 'broken'; + return 0; + } + + my @theirs; + foreach my $line (@out) + { + chomp $line; + next if $line eq ''; + my ($pname, $strip) = split(/\t/, $line, 2); + push(@theirs, { name => $pname, strip => $strip }); + } + + my @ours = map { + { + name => $_->{name}, + strip => defined $_->{strip} ? $_->{strip} : '-' + } + } @{ $self->{manifest}{entries} }; + + my $agreed = scalar(@ours) == scalar(@theirs); + foreach my $i (0 .. $#ours) + { + last unless $agreed; + my $t = $theirs[$i]; + $agreed = 0 + if $ours[$i]{name} ne $t->{name} || $ours[$i]{strip} ne $t->{strip}; + } + return 1 if $agreed; + + # Report both readings in full rather than the first line they differ + # on: the point of disagreement is not always where the damage is, + # and this is rare enough to be worth the log space. + push(@$log, + "$MODULE: $name reads $self->{subdir}/series differently than" + . " we do, so what would be applied is not what was digested\n"); + push(@$log, "$MODULE: our reading:\n"); + push(@$log, " $_->{name}\t$_->{strip}\n") foreach @ours; + push(@$log, "$MODULE: theirs ($name --list):\n"); + push(@$log, " $_->{name}\t$_->{strip}\n") foreach @theirs; + + $self->{series_status} = 'broken'; + return 0; +} + # Apply the series ourselves, for a patches repository that carries no # driver. See L for why this is a permanent path and not a # fallback waiting to be removed.