diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index a4718d173db..05dc516e190 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -24,7 +24,7 @@ repos: hooks: - id: check-added-large-files # Retain the exact lossless cross-platform performance evidence archives. - exclude: ^docs/benchmarks/((executor-copy-audit|bounded-read-ahead)-20261101|(directory-task-slots|packed-visit-snapshots|bounded-match-results|useful-match-workers|direct-evaluator-workers|derived-content-handoff|immutable-default-catalogs)-20261102)\.json\.gz$ + exclude: ^docs/benchmarks/((executor-copy-audit|bounded-read-ahead)-20261101|(directory-task-slots|packed-visit-snapshots|bounded-match-results|useful-match-workers|direct-evaluator-workers|derived-content-handoff|immutable-default-catalogs|prepared-diff-configuration)-20261102)\.json\.gz$ - id: check-merge-conflict - id: check-yaml # BINARY_TYPES below: archive containers are binary but can START with text (a phar begins with diff --git a/CHANGELOG.md b/CHANGELOG.md index c0d5692c004..deb037f0277 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ # 0.11.0 +- Prepare file-diff normalization once per action style during a run instead of recompiling ignored-line regexes for every file pair. + - Reuse immutable default language and MIME catalogs across repeated library runs while preserving custom configurations and later registrations. - Reuse requested hashes and line counts from content already loaded by workers, without retaining file bytes between stages. diff --git a/TODO.md b/TODO.md index 6c033e81a07..dcf48ea25f9 100644 --- a/TODO.md +++ b/TODO.md @@ -420,8 +420,12 @@ EP/JC/P work; candidates are not speedup claims until measured on both platforms refreshing after append-only registration and preserving custom overlays and old views. [Warm construction, RSS and native controls](docs/benchmarks/immutable-default-catalogs-20261102.md) qualify reuse only; first-parse generated tables and per-run MIME binding remain unqualified. -- [ ] O15-O22: Reconcile remaining invariant operands, - paired streaming compare, reusable stage pools, byte-bounded overlap, archive indexing/admission, +- [x] O15a: Qualify invariant file-diff configuration without moving conditional peer/content + observations. [Native, in-memory and slower controls](docs/benchmarks/prepared-diff-configuration-20261102.md) + retain once-per-style options/regex records, direct-context fallback and cross-platform costs. +- [ ] O15b: Qualify remaining hash-spec/default binding and template-family gaps against actual + production preparation; preserve dynamic defaults, conditional observations and EP03/EP07 gates. +- [ ] O16-O22: Reconcile paired streaming compare, reusable stage pools, byte-bounded overlap, archive indexing/admission, optional bulk metadata, similarity representations and output churn with EP/JC/P acceptance. Keep macrobenchmark, kernel and diagnostic contracts separate. diff --git a/docs/benchmarks/prepared-diff-configuration-20261102.json.gz b/docs/benchmarks/prepared-diff-configuration-20261102.json.gz new file mode 100644 index 00000000000..e04129e24e9 Binary files /dev/null and b/docs/benchmarks/prepared-diff-configuration-20261102.json.gz differ diff --git a/docs/benchmarks/prepared-diff-configuration-20261102.md b/docs/benchmarks/prepared-diff-configuration-20261102.md new file mode 100644 index 00000000000..bfdf8d9f42e --- /dev/null +++ b/docs/benchmarks/prepared-diff-configuration-20261102.md @@ -0,0 +1,88 @@ +# Prepare invariant file-diff configuration (2026-11-02) + +## Decision + +Reuse complete immutable file-diff options for each distinct action style during a run. Existing whole-command validation retains its error ordering. Preparation resolves algorithm, style/default format and context, normalization tokens and the RE2 ignored-line expression without observing files or rendering peer templates. A reached action looks up the shared style record after its existing peer/source reads, exact equality and binary handling. Direct evaluator callers may omit the table or a style and retain context-dependent lazy construction. + +The table owns ordinary unique records, keyed by owned style strings, and survives AST cloning for ordered/deferred evaluation. It has no reference to temporary source records and no per-file lock, erased callback, thread, clock or reference-counted configuration. It is bounded by the number of distinct action styles. `DiffOptions` embeds a noncopyable/nonmovable RE2 object; borrowing complete records avoids rebuilding or wrapping regex objects per entry. Existing validation compiles its regex once separately, then each distinct style is compiled once for execution. + +This closes only the diff-configuration subset of O15/EP03. Peer and expected-hash templates were already prepared; no duplicate template mechanism is added. Hash specification/default binding and the wider EP07 qualifications remain open. Comparison-mode patches already prepare their own options and are unchanged. + +## Measurement contracts + +Parent `563a5c1abb50c1410993cb0860c333c76faccdba` (#1054); clean `clang_release`/ThinLTO baseline and candidate binaries. Runtime variants change only `evaluate.h`, `evaluate.cc` and `run.cc`; both receive identical benchmark fixtures/scenarios. Every native invocation has an independent stdout and zero-exit oracle. Retain source/fixture/binary identities, flags, drivers, raw values and validation logs in the compressed evidence. + +Native: 42 cases per host, three sessions, per-case/participant discarded warmups, nine rotating rounds and fastest-seven means. Flat 10/1,000/10,000-file trees cover normalized, plain, equal and skipped diff actions with one/three/ten requested workers, plus ordinary listing controls. Linux uses tmpfs and CPU affinity; macOS uses warm APFS without affinity. Diff actions remain ordered coordinator work; requested workers do not prove action concurrency. + +In-memory: 32 whole-engine cases per platform, three sessions and two variants, nine repetitions/fastest-seven per process. Each includes run preflight, option preparation, traversal, diff, output and teardown; parser/fixture construction is excluded. File counts 1/10/1,000/10,000 and requested workers 1/3 cover normalized/plain/skipped diffs and names. Reference-tree/production output checks run before timing; these kernels are not external-command timings or externally normalized tool comparisons. + +Confirmation: six native controls, five sessions, a byte-identical baseline participant, warmup/nine rounds/fastest-seven. Confirmed data supplements rather than replaces the original population. No compilation, lint or sanitizer overlaps a timing population on its host. An earlier prototype exceeded the callback complexity limit; its reports/sources are retained separately. All displayed results use the rebuilt final helper form. + +## Native normalized file diffs + +Median paired session change; negative means faster. + +| Files | Requested workers | Linux | macOS | +| -----: | ----------------: | ------: | -----: | +| 10 | 1 | -1.83% | -1.52% | +| 10 | 3 | -5.09% | -0.10% | +| 10 | 10 | -0.23% | -0.02% | +| 1,000 | 1 | -20.37% | -5.80% | +| 1,000 | 3 | -19.87% | -5.61% | +| 1,000 | 10 | -19.59% | -5.24% | +| 10,000 | 1 | -20.53% | -5.86% | +| 10,000 | 3 | -20.31% | -6.48% | +| 10,000 | 10 | -19.75% | -5.78% | + +## In-memory whole-engine controls + +Median session fastest-seven times, candidate relative to baseline. Includes preparation each iteration. + +| Task | Files | Requested workers | Linux | macOS | +| :-------------- | -----: | ----------------: | ------: | ------: | +| diff-normalized | 1 | 1 | -0.36% | +0.76% | +| diff-normalized | 1 | 3 | +0.23% | +1.60% | +| diff-normalized | 10,000 | 1 | -86.85% | -88.33% | +| diff-normalized | 10,000 | 3 | -86.81% | -88.26% | +| diff-plain | 1 | 1 | +1.65% | +0.66% | +| diff-plain | 1 | 3 | +1.15% | -0.34% | +| diff-plain | 10,000 | 1 | -3.07% | -3.06% | +| diff-plain | 10,000 | 3 | -2.58% | -0.48% | +| diff-skipped | 1 | 1 | +23.46% | +21.55% | +| diff-skipped | 1 | 3 | +23.79% | +20.08% | +| diff-skipped | 10,000 | 1 | +0.35% | -0.33% | +| diff-skipped | 10,000 | 3 | +0.64% | +1.50% | +| name | 1 | 1 | +0.62% | +0.66% | +| name | 1 | 3 | +1.78% | -0.63% | +| name | 10,000 | 1 | +0.29% | -0.26% | +| name | 10,000 | 3 | +0.27% | +2.05% | + +## Slower controls and confirmation + +Keep the initial slower controls and compare the candidate and duplicate-baseline confirmation separately. + +| Files / task | Workers | Initial Linux | Confirm Linux | Baseline repeat Linux | Initial macOS | Confirm macOS | Baseline repeat macOS | +| :------------------ | ------: | ------------: | ------------: | --------------------: | ------------: | ------------: | --------------------: | +| 10 / listing | 10 | -2.14% | +3.59% | -1.48% | +0.20% | +1.53% | +0.11% | +| 1,000 / normalized | 3 | -19.87% | -19.40% | -0.01% | -5.61% | -4.36% | +0.43% | +| 10,000 / listing | 3 | +1.42% | +0.15% | -1.16% | -0.14% | +0.16% | +0.08% | +| 10,000 / equal | 10 | -0.43% | +0.34% | -0.07% | -0.26% | +0.31% | +0.28% | +| 10,000 / normalized | 1 | -20.53% | -21.29% | -0.33% | -5.86% | -5.73% | -0.18% | +| 10 / plain | 1 | -0.17% | -1.10% | -0.74% | +1.18% | +0.99% | +1.04% | + +Do not attribute changes in unrelated listing/equal-file controls to eliminated regex work. Retain modest positive costs as well as faster cases. Tiny cases include once-per-run record allocation and a style lookup; normalized large cases amortize these costs. The full raw population, including all sessions and non-displayed file counts, remains available. + +## Correctness, scope and footprint + +Direct tests cover style deduplication/default overrides, normalization errors, no-consumer behavior, byte-identical output and read counts versus unprepared contexts, skipped observations, missing-table-style fallback, separate runs with different defaults, and invalid globals before content reads. Existing full suites retain absent/binary/reversed patches, multiple formats, templates, archive/VFS behavior, safety, deferred effects and error handling. The fixture cannot mutate host paths. + +Complete core/extension release suites pass 321/321 on each host, with all extensions enabled and required Linux FUSE. The earlier lean invocation with ten skipped CLI targets is retained separately. Six focused ASan suites pass per host and six Linux TSan suites pass. Scoped clang-tidy and normal enforcing hooks remain required before publication. + +- linux: unstripped CLI 12,337,640 -> 12,360,488 bytes (+22,848). +- macos: unstripped CLI 4,069,696 -> 4,071,248 bytes (+1,552). + +Backend regex allocation bytes are not estimated as table capacity. There is no new global cache and each run releases its records. No blanket claim of allocation-free preparation, first-command speedup or parallel diff execution is made. + +## Lossless evidence + +[prepared-diff-configuration-20261102.json.gz](prepared-diff-configuration-20261102.json.gz): 1,527,277 bytes; SHA-256 `5db3615728ac98b9040abb2eaea024a7a5242b188c399e9169f977bc5d9876b2`. Initial, confirmation, in-memory and development-failure populations have separate provenance. diff --git a/docs/design-expression-program.md b/docs/design-expression-program.md index 5588c69d3f7..79eac4a4d5f 100644 --- a/docs/design-expression-program.md +++ b/docs/design-expression-program.md @@ -865,5 +865,7 @@ Differential cases cover dynamic definitions, link/content failures, content fie missing/empty arguments, moved owners and repeated workers. Whole-run cases cover final hash defaults, rendered peer paths and failures. Native CI adds literal and transformed peer, similarity, diff and expected-hash preparation/kernels plus three whole-engine scenarios. No speedup or final retention -claim precedes those measurements. Hash-spec resolution and diff option/ignore-regex preparation -remain separate invariant-work candidates. +claim precedes those measurements. The later [O15a qualification](benchmarks/prepared-diff-configuration-20261102.md) +retains complete file-diff options/ignore-regex preparation at run scope, preserving direct-context +fallback and conditional observations. Hash-spec/default resolution remains separate invariant work; +this does not close EP03/EP07 or requalify every template family. diff --git a/docs/expression-qualification-audit.md b/docs/expression-qualification-audit.md index 44f6542e3c1..d22fdc1570e 100644 --- a/docs/expression-qualification-audit.md +++ b/docs/expression-qualification-audit.md @@ -68,3 +68,14 @@ FrozenMap/Set lookup experiments, dynamic interning and adaptive worker activati tracked opportunities. They do not justify delaying the already qualified production stack or silently expanding the acceptance gate. The toolchains_llvm 1.11.1 evaluation is also isolated from these changes, so its build/performance effects remain attributable to the version pin. + +## Later invariant-family qualification: O15a + +[O15a's two-platform evidence](benchmarks/prepared-diff-configuration-20261102.md) qualifies +complete per-style file-diff options/ignored-line regexes once per run. Conditional peer/template +rendering, reads, output/error ordering and direct-context fallback retain their existing behavior. +The production and original-tree whole-run factories receive the same immutable run configuration; +dedicated direct-context tests separately compare prepared and unprepared option construction. +Retain the documented native costs as well as normalized-pair gains. This closes that subset of +acceptance item 1; hash specification/default binding and remaining template-family qualification +stay open. It is not evidence for an additional executor representation, optimizer or shared pool. diff --git a/docs/performance-analysis.md b/docs/performance-analysis.md index 4195e05cb40..71547d81afe 100644 --- a/docs/performance-analysis.md +++ b/docs/performance-analysis.md @@ -3362,3 +3362,31 @@ This preserves the earlier lazy-default and macOS fixup findings. Generated flat first JSON parsing, per-run MIME observers and broader immutable catalog binding remain follow-up hypotheses; do not call them implemented or infer gains from the warm reset kernel. No new wrapper, lock, per-file clock, thread or exception mechanism is introduced. + +### O15a decision: prepare complete file-diff options once per style + +[The cross-platform report](benchmarks/prepared-diff-configuration-20261102.md) qualifies immutable +style-keyed options for file-diff actions. Algorithm/style/default selection, normalization tokens +and ignored-line RE2 compilation move to run preparation after the existing validation. File reads, +peer field rendering, exact equality, binary handling, short-circuiting and ordered output retain +their previous timing. Direct contexts retain a lazy fallback. No new shared executor, ownership +wrapper, lock, per-file clock or mutation route is added. + +On 1,000-10,000 normalized native pairs, initial gains are about 19% on Linux and 6% on macOS; +five-session confirmation retains those gains. Separate 10,000-entry in-memory whole-run kernels +improve about 87%/88%. Those kernels include option preparation and teardown, and do not describe +external command startup or parallel diff execution. Plain/equal/skipped and ordinary listing +controls remain separate. The final confirmation retains about 19-21% Linux / 4-6% macOS gains, with Linux tiny-listing ++3.59% and macOS tiny-listing +1.53% costs. Preserve the earlier prototype's Linux listing +3.67%, +tiny plain-diff +2.75%, and macOS tiny plain-diff +1.69% costs separately, alongside identical +baseline participants and the full sessions. + +The run owns one ordinary unique record per distinct action style. Noncopyable `DiffOptions` +embeds RE2; per-entry callbacks borrow the complete record rather than creating another regex or +reference-counted wrapper. A simple ordered style lookup also works for cloned/deferred AST nodes. +The complete extension-enabled suites pass 321/321 on both hosts, plus focused ASan on both and +Linux TSan. Shipping size, raw samples and development-oracle failures are recorded in the report. + +This is only the diff subset of O15. Peer/expected-hash templates already bind through the existing +prepared operand pool. Hash specification/default resolution remains a separate candidate; EP03 +and EP07 are not marked complete by this result. diff --git a/release-site.json b/release-site.json index 14d111bfcc3..865fd70bc81 100644 --- a/release-site.json +++ b/release-site.json @@ -72,7 +72,8 @@ "docs/benchmarks/useful-match-workers-20261102.md": "docs/benchmarks/useful-match-workers-20261102.html", "docs/benchmarks/direct-evaluator-workers-20261102.md": "docs/benchmarks/direct-evaluator-workers-20261102.html", "docs/benchmarks/derived-content-handoff-20261102.md": "docs/benchmarks/derived-content-handoff-20261102.html", - "docs/benchmarks/immutable-default-catalogs-20261102.md": "docs/benchmarks/immutable-default-catalogs-20261102.html" + "docs/benchmarks/immutable-default-catalogs-20261102.md": "docs/benchmarks/immutable-default-catalogs-20261102.html", + "docs/benchmarks/prepared-diff-configuration-20261102.md": "docs/benchmarks/prepared-diff-configuration-20261102.html" }, "generated_html": { "XFF.md": { diff --git a/xff/engine/BUILD.bazel b/xff/engine/BUILD.bazel index 0fc597ab718..2d9d7ffb280 100644 --- a/xff/engine/BUILD.bazel +++ b/xff/engine/BUILD.bazel @@ -1029,3 +1029,23 @@ cc_test( "@mboworks_mbo//mbo/testing:status_cc", ], ) + +cc_test( + name = "diff_configuration_test", + size = "small", + srcs = ["diff_configuration_test.cc"], + deps = [ + ":evaluate_cc", + ":run_cc", + ":walk_cc", + "//xff/parser:parser_cc", + "//xff/vfs:vfs_cc", + "@abseil-cpp//absl/status", + "@abseil-cpp//absl/status:statusor", + "@abseil-cpp//absl/time", + "@googletest//:gtest_main", + "@mboworks_mbo//mbo/diff:diff_cc", + "@mboworks_mbo//mbo/testing:matchers_cc", + "@mboworks_mbo//mbo/testing:status_cc", + ], +) diff --git a/xff/engine/diff_configuration_test.cc b/xff/engine/diff_configuration_test.cc new file mode 100644 index 00000000000..f6ab726faec --- /dev/null +++ b/xff/engine/diff_configuration_test.cc @@ -0,0 +1,207 @@ +// SPDX-FileCopyrightText: Copyright (c) M. Boerger, the MBO Works authors +// SPDX-License-Identifier: Apache-2.0 + +#include +#include +#include +#include +#include +#include + +#include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "absl/time/time.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include "mbo/testing/matchers.h" +#include "mbo/testing/status.h" +#include "xff/engine/evaluate.h" +#include "xff/engine/run.h" +#include "xff/parser/parser.h" +#include "xff/vfs/filesystem.h" + +namespace xff::engine { +namespace { +using ::mbo::testing::EqualsText; +using ::mbo::testing::StatusIs; +using ::testing::ElementsAre; +using ::testing::Eq; +using ::testing::HasSubstr; +using ::testing::IsEmpty; +using ::testing::IsFalse; +using ::testing::IsTrue; +using ::testing::Not; +using ::testing::SizeIs; + +struct DiffFs final : vfs::FileSystem { + std::map> files{ + {"root/a", "version 1\nkeep\nold\n"}, + {"root/b", "version 2\nkeep\nnew\n"}, + {"peer", "version 3\nkeep\nold\n"}, + }; + mutable std::size_t reads = 0; + + absl::StatusOr> ReadDir(std::string_view path) const override { + if (path != "root") { + return absl::NotFoundError("unknown directory"); + } + return std::vector{ + {.path = "root/a", .name = "a", .type = vfs::FileType::kRegular}, + {.path = "root/b", .name = "b", .type = vfs::FileType::kRegular}, + }; + } + + absl::StatusOr Stat(std::string_view path, bool) const override { + return vfs::Metadata{ + .type = path == "root" ? vfs::FileType::kDirectory : vfs::FileType::kRegular, + .size = 20, + .mode = 0644, + }; + } + + absl::StatusOr ReadContent(std::string_view path) const override { + ++reads; + const auto found = files.find(path); + return found == files.end() ? absl::StatusOr{absl::NotFoundError("missing peer")} : found->second; + } + + absl::Status Remove(std::string_view) const override { return absl::PermissionDeniedError("read-only fixture"); } + + bool Access(std::string_view, vfs::AccessMode) const override { return true; } + + absl::StatusOr ReadLink(std::string_view) const override { return absl::NotFoundError("not a link"); } + + absl::StatusOr FsType(std::string_view) const override { return "memory"; } + + absl::StatusOr IsCaseSensitive(std::string_view) const override { return true; } +}; + +struct DiffConfigurationTest : ::testing::Test { + DiffFs filesystem; +}; + +TEST_F(DiffConfigurationTest, DeduplicatesStylesAndResolvesDefaultsWithoutObservingFiles) { + ASSERT_OK_AND_ASSIGN( + const auto command, + parser::Parse({"root", "-diff:u1", "peer", ",", "-rdiff:u1", "peer", ",", "-diff:c", "peer"})); + ASSERT_OK_AND_ASSIGN( + const auto configurations, PrepareDiffConfigurations( + *command.expression, DiffDefaults{ + .algorithm = "direct", + .ignore = "ws,eofnl", + .ignore_matching = "^version ", + .format = mbo::diff::DiffOptions::OutputFormat::kNormal, + .context = 5, + })); + EXPECT_THAT(configurations, SizeIs(2)); + const auto& unified = *configurations.at("u1"); + EXPECT_THAT(unified.output_format, Eq(mbo::diff::DiffOptions::OutputFormat::kUnified)); + EXPECT_THAT(unified.context_size, Eq(1)); + EXPECT_THAT(unified.algorithm, Eq(mbo::diff::DiffOptions::Algorithm::kDirect)); + EXPECT_THAT(unified.ignore_all_space, IsTrue()); + EXPECT_THAT(unified.ignore_missing_final_newline, IsTrue()); + EXPECT_THAT(unified.time_format, IsEmpty()); + EXPECT_THAT(configurations.at("c")->context_size, Eq(5)); + EXPECT_THAT(filesystem.reads, Eq(0)); +} + +TEST_F(DiffConfigurationTest, ReportsInvalidNormalizationAndDoesNotPrepareOtherConsumers) { + ASSERT_OK_AND_ASSIGN(const auto diff, parser::Parse({"root", "-diff", "peer"})); + EXPECT_THAT( + PrepareDiffConfigurations(*diff.expression, {.ignore_matching = "["}), + StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("invalid --diff-ignore-matching regex"))); + EXPECT_THAT( + PrepareDiffConfigurations(*diff.expression, {.ignore = "invalid"}), + StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("unknown --diff-ignore token"))); + ASSERT_OK_AND_ASSIGN(const auto plain, parser::Parse({"root", "-name", "a"})); + ASSERT_OK_AND_ASSIGN(const auto empty, PrepareDiffConfigurations(*plain.expression, {.ignore_matching = "["})); + EXPECT_THAT(empty, IsEmpty()); +} + +TEST_F(DiffConfigurationTest, PreparedAndDirectPathsPreserveOutputReadsAndConditionalEvaluation) { + ASSERT_OK_AND_ASSIGN(const auto command, parser::Parse({"root", "-diff:u1", "peer"})); + ASSERT_OK_AND_ASSIGN( + const auto configurations, PrepareDiffConfigurations(*command.expression, {.ignore_matching = "^version "})); + const Visit visit{ + .path = "root/b", + .name = "b", + .root = "root", + .metadata = {.type = vfs::FileType::kRegular, .mode = 0644}, + }; + std::string direct; + std::string prepared; + Control control; + const auto direct_sink = [&](std::string_view text) { direct.append(text); }; + const auto prepared_sink = [&](std::string_view text) { prepared.append(text); }; + EvalContext direct_context{ + .visit = visit, + .emit = direct_sink, + .fs = filesystem, + .now = absl::UnixEpoch(), + .tz = absl::UTCTimeZone(), + .diff_ignore_matching = "^version ", + .control = control, + }; + EXPECT_THAT(Evaluate(*command.expression, direct_context), IsFalse()); + EXPECT_THAT(filesystem.reads, Eq(2)); + EvalContext prepared_context{ + .visit = visit, + .emit = prepared_sink, + .fs = filesystem, + .now = absl::UnixEpoch(), + .tz = absl::UTCTimeZone(), + .diff_configurations = configurations, + .control = control, + }; + EXPECT_THAT(Evaluate(*command.expression, prepared_context), IsFalse()); + EXPECT_THAT(filesystem.reads, Eq(4)); + EXPECT_THAT(prepared, EqualsText(direct)); + EXPECT_THAT(prepared, Not(IsEmpty())); + + ASSERT_OK_AND_ASSIGN(const auto skipped, parser::Parse({"root", "-false", "-diff", "peer"})); + EXPECT_THAT(Evaluate(*skipped.expression, prepared_context), IsFalse()); + EXPECT_THAT(filesystem.reads, Eq(4)); + + // A direct caller may supply a partial table; an absent style retains the legacy + // caller-context path rather than inventing defaults or dereferencing an absent record. + ASSERT_OK_AND_ASSIGN(const auto unlisted, parser::Parse({"root", "-diff:n", "peer"})); + prepared.clear(); + EXPECT_THAT(Evaluate(*unlisted.expression, prepared_context), IsFalse()); + EXPECT_THAT(filesystem.reads, Eq(6)); + EXPECT_THAT(prepared, HasSubstr("1c1")); +} + +TEST_F(DiffConfigurationTest, InvalidGlobalRegexStillFailsBeforeAnyContentObservation) { + ASSERT_OK_AND_ASSIGN( + const auto command, parser::Parse({"--diff-ignore-matching=[", "root", "-false", "-diff", "peer"})); + std::vector errors; + std::string output; + const auto result = RunFind( + command, filesystem, [&](std::string_view text) { output.append(text); }, + [&](std::string_view, absl::Status status) { errors.push_back(std::move(status)); }); + EXPECT_THAT(result.errors, Eq(2)); + EXPECT_THAT(errors, ElementsAre(StatusIs(absl::StatusCode::kInvalidArgument))); + EXPECT_THAT(output, IsEmpty()); + EXPECT_THAT(filesystem.reads, Eq(0)); +} + +TEST_F(DiffConfigurationTest, SeparateRunsHonorFinalGlobalsAndPreserveEarlyValidation) { + const auto run = [&](const std::vector& arguments) { + std::vector records; + ASSERT_OK_AND_ASSIGN(const auto command, parser::Parse(arguments)); + const auto result = RunFind( + command, filesystem, [&](std::string_view text) { records.emplace_back(text); }, + [](std::string_view, absl::Status status) { ADD_FAILURE() << status; }); + EXPECT_THAT(result.errors, Eq(0)); + if (arguments.front() == "--diff-ignore-matching=^version ") { + EXPECT_THAT(records, IsEmpty()); + } else { + EXPECT_THAT(records, ElementsAre(HasSubstr("version"))); + } + }; + run({"--diff-ignore-matching=^version ", "root/a", "-diff", "peer"}); + run({"root/a", "-diff", "peer"}); + run({"--diff-ignore-matching=^version ", "root/a", "-diff", "peer"}); +} +} // namespace +} // namespace xff::engine diff --git a/xff/engine/evaluate.cc b/xff/engine/evaluate.cc index f2553c374e7..d7ab37e9fdb 100644 --- a/xff/engine/evaluate.cc +++ b/xff/engine/evaluate.cc @@ -1415,7 +1415,7 @@ constexpr auto kDiffIgnoreTokens = mbo::container::MakeLimitedMap( // Each token runs its setter via kDiffIgnoreTokens; an unknown token is an InvalidArgument // (naming it). A non-empty `matching` is compiled into options.ignore_matching_lines; a // regex that does not compile is an InvalidArgument (carrying RE2's diagnostic). Shared by -// EvalDiff (per entry) and ValidateDiffIgnore (once, pre-walk, to reject a bad value early). +// run preparation, direct EvalDiff fallbacks, and pre-walk validation. absl::Status ApplyDiffIgnore(std::string_view tokens, std::string_view matching, mbo::diff::DiffOptions& options) { for (const std::string_view token : absl::StrSplit(tokens, ',', absl::SkipEmpty())) { const auto it = kDiffIgnoreTokens.find(token); @@ -1523,6 +1523,18 @@ bool EmitAbsentPatch(const parser::Expr& expr, EvalContext& ctx, bool reverse) { } } // namespace +mbo::types::OptionalRef ConfiguredDiffOptions( + const parser::Expr& expression, + const EvalContext& context) { + if (!context.diff_configurations) { + return {}; + } + const auto found = context.diff_configurations->find(expression.diff_style); + return found == context.diff_configurations->end() + ? mbo::types::OptionalRef{} + : mbo::types::OptionalRef{*found->second}; +} + // xff -diff[:STYLE] TARGET: an ACTION that diffs the entry against TARGET (a field template, // like -cmp) via mbo::diff and returns TRUE = same (silent when equal; prints the diff and is // false on a difference). STYLE picks the output (u3 default / c / n / y / none = silent). A @@ -1568,25 +1580,22 @@ bool EvalDiffWithTemplate( } return false; } - mbo::diff::DiffOptions options; // default-constructed (myers, unified, ctx 3) then tuned - // A -diff:STYLE token overrides only the parts it names; an omitted format/context falls back to - // the resolved global default (--diff-format / --diff-context / --context, else unified / 3). - options.output_format = style.format.value_or(ctx.diff_format); - options.context_size = style.context.value_or(ctx.diff_context); - // Git-style header: an empty time_format omits the per-file mtime (`--- a/one.txt`), so the - // output is reproducible regardless of the compared files' timestamps or the local time zone. - options.time_format = ""; - if (const std::optional algo = - mbo::diff::DiffOptions::ParseAlgorithmFlag(ctx.diff_algorithm); - algo.has_value()) { - options.algorithm = *algo; - } - // --diff-ignore / --diff-ignore-matching normalization. The values were validated before - // the walk (ValidateDiffIgnore), so this only configures; the returned status is ignored. - ApplyDiffIgnore(ctx.diff_ignore, ctx.diff_ignore_matching, options).IgnoreError(); + // Run-prepared records contain the complete style/default configuration. Keep the direct + // evaluator fallback lazy so its caller-selected context is still observed when reached. + std::optional fallback; + auto configured = ConfiguredDiffOptions(expr, ctx); + if (!configured) { + fallback.emplace(); + fallback->output_format = style.format.value_or(ctx.diff_format); + fallback->context_size = style.context.value_or(ctx.diff_context); + fallback->time_format.clear(); + fallback->algorithm = mbo::diff::DiffOptions::ParseAlgorithmFlag(ctx.diff_algorithm).value_or(fallback->algorithm); + ApplyDiffIgnore(ctx.diff_ignore, ctx.diff_ignore_matching, *fallback).IgnoreError(); + configured.set_ref(*fallback); + } const absl::StatusOr diff = mbo::diff::Diff::FileDiff( mbo::file::Artefact{.data = *lhs_data, .name = names.ok() ? patch::HeaderName(names->left) : ""}, - mbo::file::Artefact{.data = *rhs_data, .name = names.ok() ? patch::HeaderName(names->right) : ""}, options); + mbo::file::Artefact{.data = *rhs_data, .name = names.ok() ? patch::HeaderName(names->right) : ""}, *configured); if (!diff.ok()) { return false; // a diff failure -> treat as differing } @@ -4540,9 +4549,43 @@ std::optional ParseDiffFormatFlag(std::str return it->second; } -// Pre-walk validation of the --diff-ignore / --diff-ignore-matching values: runs the same -// ApplyDiffIgnore the per-entry -diff uses against a throwaway DiffOptions, so the token set and -// the regex share one source of truth with the apply path. See the header for the contract. +// Populate immutable records without moving conditional file/template observations. +namespace { +absl::Status AddDiffConfigurations( + const parser::Expr& expression, + DiffDefaults defaults, + DiffConfigurations& configurations) { + if (expression.descriptor && expression.descriptor->modifier_consumer == registry::ModifierConsumer::kFileDiff + && !configurations.contains(expression.diff_style)) { + const auto style = ParseDiffStyle(expression.diff_style); + auto options = std::make_unique(); + options->output_format = style.format.value_or(defaults.format); + options->context_size = style.context.value_or(defaults.context); + options->time_format.clear(); + if (const auto algorithm = mbo::diff::DiffOptions::ParseAlgorithmFlag(defaults.algorithm)) { + options->algorithm = *algorithm; + } + MBO_RETURN_IF_ERROR(ApplyDiffIgnore(defaults.ignore, defaults.ignore_matching, *options)); + configurations.emplace(expression.diff_style, std::move(options)); + } + if (expression.lhs) { + MBO_RETURN_IF_ERROR(AddDiffConfigurations(*expression.lhs, defaults, configurations)); + } + if (expression.rhs) { + MBO_RETURN_IF_ERROR(AddDiffConfigurations(*expression.rhs, defaults, configurations)); + } + return absl::OkStatus(); +} +} // namespace + +absl::StatusOr PrepareDiffConfigurations(const parser::Expr& expression, DiffDefaults defaults) { + DiffConfigurations result; + MBO_RETURN_IF_ERROR(AddDiffConfigurations(expression, defaults, result)); + return result; +} + +// Whole-command validation keeps the existing diagnostic order independently of +// whether execution later needs a prepared action configuration. absl::Status ValidateDiffIgnore(std::string_view tokens, std::string_view matching) { mbo::diff::DiffOptions options; return ApplyDiffIgnore(tokens, matching, options); diff --git a/xff/engine/evaluate.h b/xff/engine/evaluate.h index 6bbb2d7aa01..2d1f0b4cc08 100644 --- a/xff/engine/evaluate.h +++ b/xff/engine/evaluate.h @@ -60,6 +60,23 @@ namespace xff::engine { // (path plus terminator -- "p\n" for -print, "p\0" for -print0). using EmitFn = absl::FunctionRef; +// Immutable complete options for each distinct action style. The run owns these records; +// callbacks borrow them after their existing conditional reads. Direct evaluator users may +// leave the observer empty and retain context-dependent option construction. +using DiffConfigurations = std::map, std::less<>>; + +struct DiffDefaults { + std::string_view algorithm; + std::string_view ignore; + std::string_view ignore_matching; + mbo::diff::DiffOptions::OutputFormat format = mbo::diff::DiffOptions::OutputFormat::kUnified; + std::size_t context = 3; +}; + +// Performs syntax preparation only, with no VFS/content/template observations. Call after +// existing whole-command validation, which remains responsible for error ordering. +absl::StatusOr PrepareDiffConfigurations(const parser::Expr& expression, DiffDefaults defaults); + // Traversal-control side-channel, set by control actions during evaluation and // read by the driver: -prune asks the walk not to descend into the current // directory; -quit asks it to stop the traversal entirely. Like `emit`, these @@ -240,8 +257,8 @@ struct EvalContext { // --diff-ignore=: comma-separated normalization tokens for -diff (ws / change / // trail / blank / case), validated once before the walk. Empty -> exact. Only -diff reads it. std::string_view diff_ignore; - // --diff-ignore-matching=REGEX: -diff ignores lines matching this RE2 (compiled per -diff - // entry; the pattern is validated once before the walk). Empty -> no line filter. + // --diff-ignore-matching=REGEX fallback for direct evaluator contexts. Whole runs borrow + // the immutable options below; direct calls compile when reached. Empty -> no line filter. std::string_view diff_ignore_matching; // --diff-format=u|c|n|y|...: the default -diff output format (built-in unified). A per-action // -diff:STYLE letter overrides it. Resolved once before the walk; only -diff reads it. @@ -250,6 +267,7 @@ struct EvalContext { // --context feeds it only when before==after; --diff-context overrides --context. A per-action // -diff:uN overrides both. Resolved once before the walk; only -diff reads it. std::size_t diff_context = 3; + mbo::types::OptionalRef diff_configurations; mbo::types::OptionalRef diff_paths; mbo::types::OptionalRef diff_destinations; // --hash-algorithm=ALGO / --hash-encoding=hex|base64: the default digest for a bare -hash diff --git a/xff/engine/expression_run_benchmark.cc b/xff/engine/expression_run_benchmark.cc index 9e3187da7ec..248447e7df6 100644 --- a/xff/engine/expression_run_benchmark.cc +++ b/xff/engine/expression_run_benchmark.cc @@ -49,6 +49,9 @@ class RunFs final : public xff::vfs::FileSystem { } absl::StatusOr ReadContent(std::string_view path) const override { + if (path == "peer/diff-control") { + return "reference\n"; + } return path.ends_with(".txt") ? "needle\n" : "absent\n"; } @@ -147,6 +150,26 @@ int main(int argc, char** argv) { benchmark::Initialize(&argc, argv); const auto scenarios = std::to_array({ {.name = "compare-template", .arguments = {"-type", "f", "-cmp", "peer/{relpath:s/file/copy/}"}}, + { + .name = "diff-normalized", + .arguments = + { + "--diff-ignore-matching=^(needle|absent|reference)$", + "-type", + "f", + "-diff:none", + "peer/diff-control", + "-print", + }, + }, + { + .name = "diff-plain", + .arguments = {"-type", "f", "-diff", "peer/diff-control"}, + }, + { + .name = "diff-skipped", + .arguments = {"--diff-ignore-matching=^(needle|absent|reference)$", "-false", "-diff", "peer/diff-control"}, + }, {.name = "diff-template", .arguments = {"-type", "f", "-diff", "peer/{relpath}"}}, {.name = "hash-template", .arguments = {"-type", "f", "-hasheq", "{hash}"}}, {.name = "first-limit", .arguments = {"-type", "f", "-first", "3", "-print"}}, diff --git a/xff/engine/run.cc b/xff/engine/run.cc index 01b10741e32..cc2638ba790 100644 --- a/xff/engine/run.cc +++ b/xff/engine/run.cc @@ -5133,6 +5133,22 @@ RunResult RunFindCore( } } } + DiffConfigurations diff_configurations; + if (expression) { + auto prepared = PrepareDiffConfigurations( + *expression, DiffDefaults{ + .algorithm = diff_algorithm, + .ignore = diff_ignore, + .ignore_matching = diff_ignore_matching, + .format = diff_format, + .context = diff_context, + }); + if (!prepared.ok()) { + on_error("--diff-ignore", prepared.status()); + return RunResult{.errors = 2}; + } + diff_configurations = *std::move(prepared); + } // The whole --archive surface, resolved as one value before the walk consumes it. const absl::StatusOr archive_options = ResolveArchiveOptions(command.globals, style); if (!archive_options.ok()) { @@ -6264,6 +6280,7 @@ RunResult RunFindCore( .diff_ignore_matching = diff_ignore_matching, .diff_format = diff_format, .diff_context = diff_context, + .diff_configurations = diff_configurations, .diff_paths = *diff_paths, .diff_destinations = diff_destinations, .hash_algorithm = hash_algorithm, @@ -6422,6 +6439,7 @@ RunResult RunFindCore( .diff_ignore_matching = diff_ignore_matching, .diff_format = diff_format, .diff_context = diff_context, + .diff_configurations = diff_configurations, .diff_paths = *diff_paths, .diff_destinations = diff_destinations, .hash_algorithm = hash_algorithm,