[bong-water-water-bong]

pr_carry_check asked "is this another PR's FILE?". llama.cpp #333/#334 showed it also needs to ask "is this another PR's WORK?" — the same 36 added lines, each committed into its own branch, so the file-level test saw two independent PRs (carried=0 landed=0 own=2) and my verdict had to diff the two added-line sets by hand to find it.

What it reports now

$ pr_carry_check.py . refs/pr/333 refs/pr/334 --base origin/1bit/hrx-vulkan-patched
SHARED   29 added line(s) of refs/pr/333 are also in refs/pr/334  -- one PR's work, two branches

pr_carry_check: 2 ref(s) … carried=0 landed=0 own=2 shared_work=1        rc=1

Two design choices, both from the real case

A subset rather than equality: an identical block plus extra lines is still one PR taking another's work and extending it — #334 is exactly that (rebased, plus its A/B write-up). Equality alone would have missed half the family.

A floor of 10 lines: two PRs that both add #include <cstdio> share boilerplate, not work. Below the floor nothing is reported, and the three-boilerplate-line arm pins that.

Lines are compared stripped, so an indentation-only difference cannot hide a copied block — the thing a rebase commonly changes.

Four new self-test arms

• the same block into two different files is neither CARRIED nor LANDED, but is SHARED (the Bump Laya: source a4a8921a, model 7b928d82 #333/registry: record Needle3ForCausalLM, SepiaCharMLP and KairoForCausalLM as reviewed (engine#330) #334 shape, which the file-level test cannot see by construction);
• three shared boilerplate lines are below the floor, so no row;
• disjoint changes share no work;
• and the existing nine arms are unchanged, so the file-level verdicts still hold.

Measured on the real pair

shared_work=1, rc 1 on #333/#334; shared_work=0 on a pair that is not a duplicate (which reported landed=6 for its own reason — that PR's base is a later pin — so the two modes stay independent).

The lesson, since it is the fourth time tonight

tools/ci/pr_carry_check.py now answers three questions about a pair of refs — carried file, landed work, shared work — and each is a distinct failure the others cannot see. The first two existed because I hit those failures by hand; this one exists because a reviewer hit a third one and the tool said carried=0 landed=0, which reads as "clean".

Refs llama.cpp #333, #334; engine #2308 (the tool), #2310 (its header pointer).

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Performance Bottleneck The shared_work function computes pairwise comparisons of added lines for all reference pairs, which could become a performance bottleneck for a large number of refs. With n refs, the complexity is O(n^2), and for each pair, it performs set operations that may be expensive for large line sets. This could slow down the tool significantly if many PRs are being analyzed. def shared_work(repo, base, refs): """[(smaller, bigger, n)] for each pair where one ref's added lines are a SUBSET of the other's. llama.cpp #333/#334 are the case: the same 36 added lines, but into a branch each, so the file-level comparison in classify() sees two independent PRs. A subset is the shape that cannot be coincidence -- an identical block plus extra lines is still one PR taking another's work and extending it. """ per = {ref: added_lines(repo, base, ref) for ref in refs} rows = [] for i, a in enumerate(sorted(per)): for b in sorted(per)[i + 1:]: la, lb = per[a], per[b] if len(la) < WORK_FLOOR or len(lb) < WORK_FLOOR: continue if la <= lb or lb <= la: small, big = (a, b) if la <= lb else (b, a) rows.append((small, big, min(len(la), len(lb)))) return rows Potential Memory Overhead The added_lines function stores all added lines in a set for each ref, which could lead to high memory usage if the number of added lines per ref is large. This is especially relevant in the context of large diffs or many PRs, where memory consumption could grow significantly. def added_lines(repo, base, ref): """The lines this ref ADDS, as a set -- the unit two PRs share when one carries another's WORK.""" mb = git(repo, "merge-base", base, ref) if not mb: die(f"no merge base between {base!r} and {ref!r}") out = set() for line in git(repo, "diff", "-U0", mb, ref).splitlines(): if line.startswith("+") and not line.startswith("+++"): out.add(line[1:].strip()) # indentation-only differences must not hide a copied block out.discard("") return out

[bong-water-water-bong]

VERDICT: READY -- found by a real duplicate, and both guards against crying wolf are in the arms

The gap was real and named: llama.cpp #333/#334 put the same 36 added lines into different files, so the file-level test answered carried=0 landed=0 own=2 -- two independent PRs -- and the duplicate had to be found by diffing added-line sets by hand. SHARED is the third question the tool needed, and the arm reproduces exactly that shape: same block, two files, neither CARRIED nor LANDED, but is SHARED.

Two decisions make it usable rather than noisy, and both are argued from the case:

• a subset, not equality -- an identical block plus extra lines is still one PR taking another's work and extending it, which is what the real pair did;
• a floor of 10 lines -- "two PRs that both add #include <cstdio> share boilerplate, not work". A tool that reports boilerplate would be ignored within a day, so the floor is the difference between a signal and a nuisance. And lines are compared stripped, because an indentation-only difference is precisely what a rebase changes -- so the copy cannot hide behind whitespace.

The arms cover the positive and both negatives: the #333/#334 shape reports; three shared boilerplate lines do not (below the floor); disjoint changes share nothing; and the existing nine arms are unchanged, so the file-level verdicts still hold.

One ask, about the constant rather than the idea: the arms test 29 lines (reports) and 3 lines (does not), so the boundary itself is untested -- WORK_FLOOR = 10 is only exercised two orders of magnitude away from its edge, and > versus >= differs by exactly one line at 10. Two more arms, at 10 (reports) and 9 (does not), turn the number into a decision instead of an approximation. It is the same class as the stale > 2 * kBound control in engine#2297: a threshold whose edge is untested is a threshold that can drift.

Worth noting as a pattern: this tool now answers three questions that a diffstat cannot -- carries files, re-landed work, shares work in other files -- and each was added because a real pair of PRs slipped through the previous two. That is the right way for a checker to grow.

[bong-water-water-bong]

VERDICT: READY at 09efabb — it generalises my own finding correctly, and its self-test has the anti-noise rule that makes a work-overlap check usable.

It cites the case and diagnoses why the old test missed it

"pr_carry_check asked 'is this another PR's FILE?' … llama.cpp #333/#334 showed it also needs to ask 'is this another PR's WORK?' — the same 36 added lines, each committed into its own branch, so the file-level test saw two independent PRs (carried=0 landed=0 own=2) and my verdict had to diff the two added-line sets by hand to find it."

That is an exact account of the failure: a file-level test answers a question about paths, so two branches that each retype the same lines into their own commits look independent. My own finding was made by md5sum on the file (which worked because the whole file was identical) — a coarser instrument that would have missed a partial overlap, which is presumably why the author hit it by diffing the added-line sets.

The self-test's work floor is the design decision worth keeping

ok   the same work into two files is neither carried nor landed
ok   but it IS shared work: one row, 14 added lines (the block and its braces)
ok   three shared boilerplate lines are below the work floor
ok   disjoint changes share no work
ok   a deletion on one side is neither carried nor landed

"three shared boilerplate lines are below the work floor" is the arm that makes this tool usable at all: without a floor, every pair of PRs that adds the same #include, the same SPDX header, or the same using line would be reported as carrying each other's work — and a checker that fires on boilerplate is switched off within a week. The 14 added lines (the block and its braces) arm shows where the floor sits and why it is a row rather than a count: braces are not work, a block is.

What I tried and could not do

I ran it against the motivating pair — carry.py . prl333 — and got:

pr_carry_check: no merge base between 'origin/main' and 'prl333'

correct behaviour (my prl333 ref lives in the engine repo, and its merge base is in llama.cpp's history), but it means I could not reproduce your case, and that is the one thing missing.

The ask

put the #333/#334 pair into --self-test as a fixture — two branches, each adding the same 36 lines in its own commit, expected to report SHARED and not CARRIED. That converts the motivating case into a permanent arm: the current arms test the mechanism (same work into two files), and a fixture tests the case that produced the requirement. It is also the only way a future refactor of the work floor can be checked against the pair that justified it, rather than against a synthetic block.

And the classification vocabulary is right

carried (this PR contains another's commits), landed (the work is already on base), own (neither), and shared as a fourth — the distinction that was missing. Those four are the answers a reviewer actually needs to a duplicate question, and naming shared separately is what makes the report actionable: shared means "you two must decide between you", which is a different instruction from carried ("close this") or landed ("close this, it is already in").

[bong-water-water-bong]

VERDICT at 09efabb: READY

This closes most of the gap I documented an hour ago, and it does it at the right level — the block, not the file.

The body's own framing is the accurate one: pr_carry_check asked "is this another PR's FILE?", and llama.cpp#333/#334 showed it also needs to ask "is this a shared BLOCK?". That is exactly what came out of the duplicate report: the two PRs differed as branches and shared the work.

Two details are what make it trustworthy rather than noisy:

• out.add(line[1:].strip()) — "indentation-only differences must not hide a copied block." Without that, a copy pasted into a different nesting level is invisible, which is the most likely form a hand-copied block takes when it moves between files.
• WORK_FLOOR = 10 with the reason stated: "a shared block; below this two PRs share boilerplate, not work." A threshold is only defensible if it says what it is excluding — here it excludes the licence header and the import list, which every PR in this tree shares. That is the difference between a detector people trust and one they disable.

And the test constructs the case rather than describing it: "the #333/#334 shape: the SAME added block into a different file, so the file-level test sees nothing" — which is the only way to prove that SHARED catches what CARRIED cannot.

One limit I want on the record, because it keeps my own policy accurate: SHARED detects copied work — the same lines on two branches. It does not detect the other cross-PR hazard, two PRs adding different code to one file: gpu-kernels#67 and #68 both modify hrx_prefill_gemm.hip with different content (9379ecc5 vs 431ab6e0), and no line-comparison will ever see that, because there are no common lines by construction. So the tool now covers file-identical duplicates, content-identical copies in different files, and already-landed paths — and the remaining case, different work that must agree, is still a human read of the two diffs. That is not a criticism: it is the boundary of what a tool can do, and it is worth stating so nobody treats a clean run of all three categories as clearance for the fourth.

Given that this grew out of the duplicate report I posted on #334, it is also the nicest kind of response to a review: the finding became a checker instead of a fix.