uros-b commented on code in PR #57764:
URL: https://github.com/apache/spark/pull/57764#discussion_r3718711189
##########
dev/merge_spark_pr.py:
##########
@@ -615,6 +727,83 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref):
return pick_ref, pick_hash
+def branches_with_merge_footer(pr_num, branch_names):
Review Comment:
Following up on my previous reply with an honest cost/benefit, because
having now done the extraction I am not convinced it was the right call for
this PR, and I would rather ask than quietly ship it.
**What it cost.** The commit is +252/-147 across four files, and the new
module is 222 lines. But the actual shared logic is small:
```
dev/spark_merge_footer.py: 222 lines total
executable logic 64
license + docstrings + doctests + blanks 159
```
The genuinely deduplicated code -- `git log --grep` plus `for-each-ref
--contains` plumbing -- is roughly **15 lines**. Everything else is packaging:
a license header, a module docstring explaining the refresh policy, and 20
doctests. So this trades ~15 duplicated lines for a new file and a new
cross-script import.
**Problems it introduces.**
1. **First cross-script import in `dev/`.** `merge_spark_pr.py` previously
imported stdlib only. It now depends on a sibling module resolved via
`sys.path[0]`, which works when the script is executed but silently breaks if
either tool is ever vendored, copied, or invoked in a way that does not put
`dev/` first on the path. Committers do run these from odd places.
2. **Coupling two tools with different risk profiles.** `pr_merge_status.py`
is read-only and may exit freely. `merge_spark_pr.py` pushes to `apache/spark`
and must never abort mid-merge. I preserved both policies via an injected
`run_git`, but that injection exists purely to keep the coupling safe -- it is
complexity created by sharing, not complexity that sharing removed.
3. **A refactor of a working tool, for no user-visible gain.**
`pr_merge_status.py` was correct before. It is now rewritten to gain nothing
except sharing. I verified byte-identical output across eight cases, but any
change to a working committer tool carries nonzero risk, and this one is not
needed to fix the bug this PR is about.
4. **Scope creep on the review surface.** This PR set out to make a
`branch-M.x` PR backportable. It now also modifies a second committer tool and
adds a third file, which is more for reviewers to check than the original fix
warranted.
**What it genuinely bought.** One real thing: extracting it exposed a latent
`int` vs `str` bug (`pr_merge_status.py` passes the PR number from argv as a
string; `merge_footer_pr` returns an int, so `==` never matched). That is a
fair point in favour of consolidation. But note that bug only existed *because*
I had just written the validated matcher -- it was not a pre-existing
divergence between the two tools.
**So: was this necessary for this PR?** My inclination is no. The four
correctness defects you raised earlier were real and are fixed; this last item
is a design preference on ~15 lines of git plumbing. I would suggest reverting
3c5f8f6 and filing a follow-up JIRA to do the consolidation properly -- with
the `int`/`str` fix carried over -- so the two tools can be unified
deliberately rather than as a tail-end change to a bugfix PR.
Happy to keep it if you would rather it land together; I have no strong
attachment either way and it is tested. Which would you prefer?
--
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]
