ci: harden backport cherry-pick against package/directory renames - #6964
ci: harden backport cherry-pick against package/directory renames#6964Yicong-Huang wants to merge 2 commits into
Conversation
Both backport scripts invoke `git cherry-pick` bare, leaving three merge knobs at defaults that misbehave when a package/directory rename lands between main and a release branch: - merge.renameLimit / diff.renameLimit: Git silently skips inexact rename detection once unpaired paths exceed the limit, so renamed files degrade to deleted+added and picked hunks hit paths that no longer exist. - merge.directoryRenames (default `conflict`): files a backport *adds* under an old package path are not auto-placed into the renamed directory. - rename-threshold: a rename that also rewrites package/import lines can drop a small file below the 50% default and be missed. Wrap both cherry-picks (preflight and fallback-branch) identically with raised rename limits, directory-rename detection, and a 40% threshold so a green preflight and the branch it later builds cannot disagree. Pure config passthrough to the merge machinery; no logic change. Closes #6963 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6964 +/- ##
============================================
- Coverage 78.98% 78.97% -0.02%
+ Complexity 3786 3782 -4
============================================
Files 1160 1160
Lines 46105 46105
Branches 5115 5115
============================================
- Hits 36418 36410 -8
- Misses 8067 8072 +5
- Partials 1620 1623 +3
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 392 | 0.239 | 24,894/33,962/33,962 us | 🔴 +11.1% / 🔴 +109.2% |
| ⚪ | bs=100 sw=10 sl=64 | 790 | 0.482 | 124,740/145,587/145,587 us | ⚪ within ±5% / 🔴 +34.0% |
| ⚪ | bs=1000 sw=10 sl=64 | 912 | 0.557 | 1,094,590/1,138,666/1,138,666 us | ⚪ within ±5% / 🔴 +9.5% |
Baseline details
Latest main ee41d9a from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 392 tuples/sec | 406 tuples/sec | 767.9 tuples/sec | -3.4% | -49.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.239 MB/s | 0.248 MB/s | 0.469 MB/s | -3.6% | -49.0% |
| bs=10 sw=10 sl=64 | p50 | 24,894 us | 22,412 us | 12,502 us | +11.1% | +99.1% |
| bs=10 sw=10 sl=64 | p95 | 33,962 us | 36,974 us | 16,234 us | -8.1% | +109.2% |
| bs=10 sw=10 sl=64 | p99 | 33,962 us | 36,974 us | 18,919 us | -8.1% | +79.5% |
| bs=100 sw=10 sl=64 | throughput | 790 tuples/sec | 815 tuples/sec | 974.8 tuples/sec | -3.1% | -19.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.482 MB/s | 0.497 MB/s | 0.595 MB/s | -3.0% | -19.0% |
| bs=100 sw=10 sl=64 | p50 | 124,740 us | 120,756 us | 102,449 us | +3.3% | +21.8% |
| bs=100 sw=10 sl=64 | p95 | 145,587 us | 145,915 us | 108,652 us | -0.2% | +34.0% |
| bs=100 sw=10 sl=64 | p99 | 145,587 us | 145,915 us | 116,310 us | -0.2% | +25.2% |
| bs=1000 sw=10 sl=64 | throughput | 912 tuples/sec | 913 tuples/sec | 1,004 tuples/sec | -0.1% | -9.2% |
| bs=1000 sw=10 sl=64 | MB/s | 0.557 MB/s | 0.558 MB/s | 0.613 MB/s | -0.2% | -9.1% |
| bs=1000 sw=10 sl=64 | p50 | 1,094,590 us | 1,089,996 us | 999,606 us | +0.4% | +9.5% |
| bs=1000 sw=10 sl=64 | p95 | 1,138,666 us | 1,171,111 us | 1,046,770 us | -2.8% | +8.8% |
| bs=1000 sw=10 sl=64 | p99 | 1,138,666 us | 1,171,111 us | 1,076,937 us | -2.8% | +5.7% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,509.89,200,128000,392,0.239,24894.14,33962.15,33962.15
1,100,10,64,20,2533.13,2000,1280000,790,0.482,124739.80,145587.12,145587.12
2,1000,10,64,20,21920.23,20000,12800000,912,0.557,1094590.28,1138665.74,1138665.74
xuang7
left a comment
There was a problem hiding this comment.
This PR seems to miss a third cherry-pick path: the green path in direct-backport-push.yml (push-backports job, line 401) that cherry-picks directly onto the release branch. That command still runs without the rename-detection settings, while the preflight check now uses them. As a result, a rename handled successfully during preflight could still conflict during the actual cherry-pick, which is the inconsistency this PR is intended to prevent.
…ck too The green push path in direct-backport-push.yml cherry-picks straight onto the release branch and was still running bare, so a package/directory rename resolved cleanly by the preflight (prepare-backport-checkout.sh) could still conflict here — exactly the preflight/apply inconsistency this change set is meant to remove. Wrap it with the same rename limits, directory-rename detection, and 40% threshold as the other two call sites. Reported-by: @xuang7 in review of #6964 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Good catch, thanks @xuang7 — you're right, Fixed in 4bd0333: wrapped it with the same |
What changes were proposed in this PR?
The backport automation already uses
git cherry-pick(a 3-way merge with rename detection), notpatch/git apply, so a renamed file is normally followed to its new path automatically. But everycherry-pickcall site invokes it bare, leaving three merge knobs at defaults that misbehave precisely when a package/directory rename lands betweenmainand arelease/*branch — producing spurious conflicts on backports that would otherwise apply cleanly.This PR wraps all backport
cherry-pickinvocations with the same three settings:merge.renameLimit/diff.renameLimit— Git silently skips inexact rename detection once the number of unpaired added/deleted paths exceeds the limit (warning only). A package-wide rename can blow past the default, degrading renamed files to "deleted + added" so picked hunks land on paths that no longer exist. Raised so detection stays on.merge.directoryRenames=true— the defaultconflictwill not auto-place a file the backport adds under an old package path into the renamed directory. Backports that add files are common here (see the feature-absent guard increate-backport-branch.sh).-Xrename-threshold=40%— a rename that also rewritespackage/importlines can drop a small file's similarity below the 50% default and miss the rename.Applied identically at all three cherry-pick call sites so no stage can disagree with another over a rename:
.github/scripts/prepare-backport-checkout.sh— pre-merge preflight cherry-pick of the squashed range..github/scripts/create-backport-branch.sh— post-merge fallback cherry-pick that opens the draft backport PR..github/workflows/direct-backport-push.yml— post-merge green path that cherry-picks straight onto the release branch (thanks @xuang7 for catching this one).Pure configuration passthrough to the merge machinery — no control-flow change, no new dependencies.
Any related issues, documentation, discussions?
Closes #6963
How was this PR tested?
This is a configuration-only change to two shell scripts and one workflow's shell step, with no control-flow change. Verified
bash -n(syntax) on the edited scripts and YAML parse on the workflow. The added-cflags and-Xrename-thresholdare standard, long-supportedgit cherry-pick/ merge-machinery options, so behavior on the existing (no-rename) path is unchanged; the new settings only take effect when a rename is present, which is exactly the case that previously conflicted. No unit tests exist for these CI helper scripts, and the real end-to-end path (backport preflight + Direct Backport Push) exercises them on the next backport-labeled PR.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)