Skip to content

Fix NameError in git-webkit review with a branch argument - #78222

Open
TheRealAlexxx wants to merge 1 commit into
WebKit:mainfrom
TheRealAlexxx:review-branch-argument-nameerror
Open

TheRealAlexxx wants to merge 1 commit into
WebKit:mainfrom
TheRealAlexxx:review-branch-argument-nameerror

Conversation

@TheRealAlexxx

@TheRealAlexxx TheRealAlexxx commented Oct 25, 2026 •

Copy link
Copy Markdown

Fix NameError in git-webkit review with a branch argument

git-webkit review accepts a pull-request number, a URL, or a branch name. The branch-name path calls PullRequest.find_existing_pull_request(repository, rmt, branch=target), but no variable named rmt exists in that scope, so every review by branch name crashes with NameError. This includes the reported git-webkit review --dry-run HEAD. See https://bugs.webkit.org/show_bug.cgi?id=321220.

Pass the remote repository, which is what the variable was meant to be. In addition, resolve HEAD to the current branch when running from a local checkout, so the reported command reviews the pull-request for the branch you have checked out instead of failing to find a branch literally named HEAD. Without a local checkout, HEAD cannot be resolved and the command reports that plainly.

A new test creates a pull-request in a mock repository and reviews it both by branch name and as HEAD, checking that each command completes instead of crashing.

PYTHONPATH=Tools/Scripts/libraries/webkitscmpy:Tools/Scripts python3 -m unittest webkitscmpy.test.review_unittest webkitscmpy.test.pull_request_unittest
python3 Tools/Scripts/check-webkit-style Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/review.py Tools/Scripts/libraries/webkitscmpy/webkitscmpy/test/review_unittest.py
```<!--EWS-Status-Bubble-Start-->
https://github.com/WebKit/WebKit/commit/8372ec75c285c20e12d908ee0639ebdc99eb9f52

| Misc | iOS, visionOS, tvOS & watchOS  | macOS  | Linux |  Windows |
| ----- | ---------------------- | ------- |  ----- |  --------- |
| [✅ 🧪 style](https://ews-build.webkit.org/#/builders/38/builds/197526 "Passed style check") | [✅ 🛠 ios](https://ews-build.webkit.org/#/builders/159/builds/70474 "Built successfully") | [✅ 🛠 mac](https://ews-build.webkit.org/#/builders/168/builds/64502 "Built successfully") | [✅ 🛠 wpe](https://ews-build.webkit.org/#/builders/5/builds/206881 "Built successfully") | [✅ 🛠 win](https://ews-build.webkit.org/#/builders/59/builds/160618 "Built successfully") 
| [✅ 🧪 bindings](https://ews-build.webkit.org/#/builders/9/builds/199384 "Passed tests") | [✅ 🛠 ios-sim](https://ews-build.webkit.org/#/builders/155/builds/72816 "Built successfully") | [✅ 🛠 mac-cmake-debug](https://ews-build.webkit.org/#/builders/156/builds/70439 "Built successfully") | [✅ 🧪 wpe-wk2](https://ews-build.webkit.org/#/builders/34/builds/153647 "Passed tests") | [✅ 🧪 win-tests](https://ews-build.webkit.org/#/builders/60/builds/106080 "Passed tests") 
| [✅ 🧪 webkitperl](https://ews-build.webkit.org/#/builders/11/builds/200482 "Passed tests") | [✅ 🧪 ios-wk2](https://ews-build.webkit.org/#/builders/162/builds/57603 "Passed tests") | [✅ 🧪 api-mac](https://ews-build.webkit.org/#/builders/18/builds/173917 "Passed tests") | [✅ 🧪 api-wpe](https://ews-build.webkit.org/#/builders/41/builds/134337 "Passed tests") | 
| [✅ 🧪 webkitpy](https://ews-build.webkit.org/#/builders/6/builds/196863 "Passed tests") | [✅ 🧪 ios-wk2-wpt](https://ews-build.webkit.org/#/builders/154/builds/56731 "Passed tests") | [✅ 🧪 api-mac-cmake-debug](https://ews-build.webkit.org/#/builders/165/builds/53133 "Passed tests") | | 
| | [✅ 🧪 api-ios](https://ews-build.webkit.org/#/builders/13/builds/166085 "Passed tests") | [✅ 🧪 mac-wk2](https://ews-build.webkit.org/#/builders/169/builds/54090 "Passed tests") | [✅ 🛠 gtk3-gcc](https://ews-build.webkit.org/#/builders/284/builds/18471 "Built successfully") | 
| | | [✅ 🧪 mac-cmake-debug-wk2](https://ews-build.webkit.org/#/builders/161/builds/58422 "Passed tests") | [✅ 🛠 gtk](https://ews-build.webkit.org/#/builders/2/builds/209563 "Built successfully") | 
| [✅ 🧪 services](https://ews-build.webkit.org/#/builders/28/builds/196927 "Passed tests") | [✅ 🛠 vision](https://ews-build.webkit.org/#/builders/153/builds/69965 "Built successfully") | | [✅ 🧪 gtk-wk2](https://ews-build.webkit.org/#/builders/1/builds/164397 "Passed tests") | 
| | [✅ 🛠 vision-sim](https://ews-build.webkit.org/#/builders/160/builds/70576 "Built successfully") | [✅ 🧪 mac-intel-wk2](https://ews-build.webkit.org/#/builders/170/builds/49597 "Passed tests") | [✅ 🧪 api-gtk](https://ews-build.webkit.org/#/builders/21/builds/164150 "Passed tests") | 
| | [✅ 🧪 vision-wk2](https://ews-build.webkit.org/#/builders/164/builds/58595 "Passed tests") | | [  ~~🛠 playstation~~](https://ews-build.webkit.org/#/builders/134/builds/141694 "The change is no longer eligible for processing.") | 
| | [✅ 🛠 tv](https://ews-build.webkit.org/#/builders/158/builds/69766 "Built successfully") | [✅ 🧪 mac-site-isolation](https://ews-build.webkit.org/#/builders/185/builds/29033 "Passed tests") | | 
| | [✅ 🛠 tv-sim](https://ews-build.webkit.org/#/builders/157/builds/68009 "Built successfully") | | | 
| | [✅ 🛠 watch](https://ews-build.webkit.org/#/builders/163/builds/70095 "Built successfully") | | | 
| | [✅ 🛠 watch-sim](https://ews-build.webkit.org/#/builders/152/builds/69725 "Built successfully") | | | 
<!--EWS-Status-Bubble-End-->

https://bugs.webkit.org/show_bug.cgi?id=321220

Reviewed by NOBODY (OOPS!).

git-webkit review accepts a pull-request number, a URL, or a branch name. The branch-name path calls PullRequest.find_existing_pull_request(repository, rmt, branch=target), but no variable named rmt exists in that scope, so every review by branch name crashes with NameError. This includes the reported git-webkit review --dry-run HEAD. See https://bugs.webkit.org/show_bug.cgi?id=321220.

Pass the remote repository, which is what the variable was meant to be. In addition, resolve HEAD to the current branch when running from a local checkout, so the reported command reviews the pull-request for the branch you have checked out instead of failing to find a branch literally named HEAD. Without a local checkout, HEAD cannot be resolved and the command reports that plainly.

A new test creates a pull-request in a mock repository and reviews it both by branch name and as HEAD, checking that each command completes instead of crashing.

```
PYTHONPATH=Tools/Scripts/libraries/webkitscmpy:Tools/Scripts python3 -m unittest webkitscmpy.test.review_unittest webkitscmpy.test.pull_request_unittest
python3 Tools/Scripts/check-webkit-style Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/review.py Tools/Scripts/libraries/webkitscmpy/webkitscmpy/test/review_unittest.py
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants