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/Modules/PatchStack.pm b/PGBuild/Modules/PatchStack.pm index 54bb607..718ec75 100644 --- a/PGBuild/Modules/PatchStack.pm +++ b/PGBuild/Modules/PatchStack.pm @@ -19,34 +19,70 @@ 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. - -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. +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 +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 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 -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 +110,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 apply_series); -use File::Path qw(mkpath); -use File::Basename qw(dirname); +use File::Path qw(mkpath); use strict; use warnings; @@ -129,7 +165,8 @@ sub setup local_repo => $local_repo, applied => 0, patches_id => '', - pre_apply_sha => '', + stack_commit => '', + manifest => undef, }; bless($self, $class); @@ -181,13 +218,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 +235,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 +274,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; } - } - 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); + 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 }); } - 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; } @@ -426,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; } @@ -496,21 +376,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 +444,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 +464,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) @@ -605,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/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; 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 9026b05..87a5fba 100755 --- a/check_patch_stack.pl +++ b/check_patch_stack.pl @@ -25,30 +25,39 @@ =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: 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 +68,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 +84,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 apply_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 +106,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 +157,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 @@ -184,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 ($@) { @@ -196,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", @@ -215,122 +272,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 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 ($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}, $manifest); } # Default mode: dry-run each patch independently against the pristine @@ -370,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"; @@ -390,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. @@ -456,34 +410,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 +446,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 +461,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..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 @@ -122,9 +123,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 @@ -158,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 @@ -175,28 +189,112 @@ 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. + +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 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 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.