janhoy commented on PR #5021:
URL: https://github.com/apache/solr/pull/5021#issuecomment-6004954285
Thanks David for fixing this. It will be the third attempt at a not-terrible
RM experience handling these yml files. I was close but not close enough. Below
is some (AI) comments...
> 🤖 The research and this comment were generated by Claude Code. I've read
it through and agree with the findings, but treat the details as
machine-produced and verify anything that matters.
I checked out the branch and verified the claims empirically — the core fix
is correct, and the bug it fixes is definitely real. Against the actual 9.11.0
history (`upstream/main...upstream/branch_9_11`):
| pathspec | commits selected |
| --- | --- |
| `changelog/` (old) | **183** — every feature commit with a changelog entry
since the branch cut |
| `changelog/v9.11.0/` (new) | **6** — exactly the right ones |
I also reproduced the modify/delete case in a scratch repo and confirmed
that `-X ours -X no-renames` really does leave a `UD` unmerged path that `-X
ours` cannot settle, that `git diff --diff-filter=U` reports it, and that the
`git rm` + `cherry-pick --continue` recovery produces the correct tree. And the
narrowing is safe for the commits that matter: `prepare` stages `git add
changelog` as a single commit touching both `unreleased/` and `v{version}/`, so
it's still selected, and the `.gitkeep` commit the narrowing now excludes is
independently re-created per target by `ensure_unreleased_gitkeep`.
A few things below, only the first two of which I'd consider worth acting on
before merge.
### 1. `--cherry-pick` now costs ~3.5 min per target, for no benefit
Measured on this repo:
```
git log --cherry-pick --right-only upstream/main...upstream/branch_9_11 --
changelog/v9.11.0/ ...
→ 3m22s
```
Git computes patch-ids for the **entire** symmetric difference before path
limiting prunes it — that's 6652 commits for `main` and 6576 for `branch_10x`.
So roughly 7 minutes of completely output-free hang, in a wizard step that runs
with `confirm_each_command: false` and `--push`. This is exactly the
cross-major scenario the PR is about, so it'll bite on the next 9.x release.
For comparison, the candidate list *without* `--cherry-pick` takes
**0.04s**. Filtering those 6 candidates by patch-id against the target's own 6
pathspec-touching commits gives the identical answer in well under a second — I
verified both produce the same result (zero commits for 9.11.0, since main
already has them).
The simplest option is to drop `--cherry-pick` altogether:
`recover_cherry_pick` already `--skip`s a pick that is empty because it's
already applied, so that is what provides re-run idempotency now. Relatedly,
the step-4 comment still credits `--cherry-pick` with making forward-port
idempotent, which is stale either way.
### 2. `recover_cherry_pick` can delete an unreleased entry it shouldn't
The recovery removes *any* conflicting path under `changelog/unreleased/`.
The pre-existing stale pass just below it is deliberately more careful — it
only removes unreleased files that have a counterpart in
`changelog/v{version}/`. Without that guard, an identically-named entry on the
target that belongs to a *different* version gets silently `git rm`'d, losing a
contributor's pending changelog entry on a branch the script then pushes.
Applying the same counterpart check, and falling through to the existing
error path when it doesn't hold, would keep the two code paths consistent.
### 3. `git restore changelog/` is unguarded destruction of uncommitted work
`cmd_forward_port` has no clean-tree precondition, and the `git checkout
<release_branch>` in step 1 will happily carry non-conflicting local
modifications along. A `git status --porcelain` assertion at the top of the
function would make both new `restore` calls safe, and would also protect the
new `pull` and the per-target checkouts.
Related: `restore` only touches tracked files, so the "blocks the next
checkout" problem isn't fully closed. A target branch whose version folder has
no tracked `version-summary.md` gets an untracked one, which still blocks
checkout if the next branch tracks that path. Probably not reachable today, but
worth a comment if you don't want to handle it.
### 4. The release branch isn't pulled, only the targets
Step 6 pushes `release_branch` too, so if it's behind the remote the push is
rejected *after* all the target work is done. The same `git pull --ff-only` in
step 1 would make this symmetric with the stated goal ("so the final push isn't
rejected").
### Nits
- `recover_cherry_pick` ignores `dry_run`. It's unreachable in dry-run today
(a dry-run `git()` returns rc 0 and never raises), but it's unguarded, so it
would `git rm` and `cherry-pick --continue` for real the day that changes.
- `unmerged_paths` is exposed to `core.quotepath`: a non-ASCII entry
filename (changelog names derive from JIRA summaries) comes back quoted and the
following `git rm` then fails. `git diff -z`, or `git ls-files -u -z`, avoids
it. The pre-existing `commit_touches_unreleased` has the same latent issue.
- In `releaseWizard.yaml`, "but not yet on the target to `branch_10x`,
`main`, and `branch_9x`" reads a bit garbled now. More importantly, the
description doesn't mention that the script now `git pull --ff-only`s each
target — RMs should know it moves their local branches.
- `git restore` needs git >= 2.23. Fine in practice; just noting there's no
documented minimum anywhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
