From d2f521b21373d1dabfed91ef119b6404e27cc168 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Tue, 11 Aug 2026 12:09:31 -0400 Subject: [PATCH 1/3] 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 b382d77053cf034b37c1a9f8cd47a92c158847d6 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Tue, 11 Aug 2026 12:09:42 -0400 Subject: [PATCH 2/3] 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 4530e4c9632c6f24065bcfa8fe56a743272ec1f8 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Tue, 11 Aug 2026 12:09:54 -0400 Subject: [PATCH 3/3] 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 bd0167b..51c2f5d 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.