Skip to content

Commit d241767

Browse files
committed
Recognize PR# at end of commit title for github_pull_request_* templates
Summary: On GitHub, commits that are squash/merged from pull requests such as facebook/dotslash@4c0563c commonly have the pull request number at the end of the first line of the commit message like so: ``` Document experimental `fetch` subcommand (facebook#24) ``` This updates the logic for the various `github_pull_request_*` templates to match this pattern. Note this required adding `repo` as an argument to `_parse_github_pull_request_url()` so that it could produce a complete `PullRequestId` object even if it only had the commit number in the commit message. Test Plan: Added a `FakeGitHubRepo` to `testutil.py` to make it easier to write doctests against this new logic. ``` ./tests/run-tests.py ./tests/test-doctest.py ``` I also looked for a repo that doesn't use Meta's tooling (so it doesn't have the `Pull Request resolved` line in its commits) and decided to test with https://github.com/google/generative-ai-docs as follows: ``` $ sl clone https://github.com/google/generative-ai-docs $ cd generative-ai-docs $ /home/mbolin/src/sapling/eden/scm/sl log -T '{node} {github_pull_request_number}\n' --limit 10 d216731f8ab1f2e46da37b62ada121848f1be9e9 375 bc112742515b93e718d9c417203de12fd8786f6a 374 9c91576af5a7b45885ef89858fd3b904501fbb8e 370 45cac496b9a93f4c2251dbe42b0eff71b2a47bc7 360 650a2a48c12ac09a8fe03640f257da6cfac68976 363 576a808e5110dee0d3a9b4456ab8d572c29a8905 359 35e3ae486b2dec7a09952153dda62819a5f4abf2 358 71f9451edc19de37c311906b7f3a689bcacb2bc6 352 da44a67beb8a85bda0e27eb38d5bb8ecec5b26dd 342 955e2842f720fdddb0164d168bc1379470b9b8e9 340 $ /home/mbolin/src/sapling/eden/scm/sl log -T '{node} {github_pull_request_url}\n' --limit 10 d216731f8ab1f2e46da37b62ada121848f1be9e9 https://reviewstack.dev/google/generative-ai-docs/pull/375 bc112742515b93e718d9c417203de12fd8786f6a https://reviewstack.dev/google/generative-ai-docs/pull/374 9c91576af5a7b45885ef89858fd3b904501fbb8e https://reviewstack.dev/google/generative-ai-docs/pull/370 45cac496b9a93f4c2251dbe42b0eff71b2a47bc7 https://reviewstack.dev/google/generative-ai-docs/pull/360 650a2a48c12ac09a8fe03640f257da6cfac68976 https://reviewstack.dev/google/generative-ai-docs/pull/363 576a808e5110dee0d3a9b4456ab8d572c29a8905 https://reviewstack.dev/google/generative-ai-docs/pull/359 35e3ae486b2dec7a09952153dda62819a5f4abf2 https://reviewstack.dev/google/generative-ai-docs/pull/358 71f9451edc19de37c311906b7f3a689bcacb2bc6 https://reviewstack.dev/google/generative-ai-docs/pull/352 da44a67beb8a85bda0e27eb38d5bb8ecec5b26dd https://reviewstack.dev/google/generative-ai-docs/pull/342 955e2842f720fdddb0164d168bc1379470b9b8e9 https://reviewstack.dev/google/generative-ai-docs/pull/340 ```
1 parent 1d26709 commit d241767

File tree

7 files changed

+65
-8
lines changed

7 files changed

+65
-8
lines changed

eden/scm/sapling/ext/github/github_repo_util.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@ def find_github_repo(repo) -> Result[GitHubRepo, NotGitHubRepoError]:
8686
test_url = os.environ.get("SL_TEST_GH_URL")
8787
if test_url:
8888
url = util.url(test_url)
89+
elif hasattr(repo, "get_github_url"):
90+
url = util.url(repo.get_github_url())
8991
else:
9092
url = repo.ui.paths.get("default", "default-push").url
9193
except AttributeError: # ex. paths.default is not set

eden/scm/sapling/ext/github/pr_marker.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ def _get_draft_commits(repo) -> t.Dict[PullRequestId, t.Set[Node]]:
9191

9292
def _get_pr_for_context(repo, ctx) -> t.Optional[PullRequestId]:
9393
store = PullRequestStore(repo)
94-
return get_pull_request_for_context(store, ctx)
94+
return get_pull_request_for_context(store, repo, ctx)
9595

9696

9797
def _get_landed_commits(

eden/scm/sapling/ext/github/pr_parser.py

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@
66
import re
77
from typing import Optional
88

9+
from .github_repo_util import find_github_repo
910
from .pullrequest import PullRequestId
1011
from .pullrequeststore import PullRequestStore
1112

1213

1314
def get_pull_request_for_context(
1415
store: PullRequestStore,
16+
repo,
1517
ctx,
1618
) -> Optional[PullRequestId]:
1719
"""Returns a pull request associated with a commit context, if any. Checks
@@ -20,20 +22,55 @@ def get_pull_request_for_context(
2022
"""
2123
node = ctx.node()
2224
pr = store.find_pull_request(node)
23-
return pr if pr else _parse_github_pull_request_url(ctx.description())
25+
return pr if pr else _parse_github_pull_request_url(ctx.description(), repo)
2426

2527

26-
def _parse_github_pull_request_url(descr: str) -> Optional[PullRequestId]:
28+
def _parse_github_pull_request_url(descr: str, repo) -> Optional[PullRequestId]:
2729
r"""If the commit message has a comment in a special format that indicates
2830
it is associated with a GitHub pull request, returns the corresponding
2931
PullRequestId.
3032
33+
Match the number at the end of the title.
34+
>>> from .testutil import FakeGitHubRepo
35+
>>> gh_repo = FakeGitHubRepo(name="dotslash")
36+
>>> commit_msg = 'Document experimental `fetch` subcommand (#24)\nSummary:\nDocument...'
37+
>>> _parse_github_pull_request_url(commit_msg, gh_repo)
38+
PullRequestId(hostname='github.com', owner='facebook', name='dotslash', number=24)
39+
40+
Should work with a single line title.
41+
>>> commit_msg = 'Document experimental `fetch` subcommand (#24)'
42+
>>> _parse_github_pull_request_url(commit_msg, gh_repo)
43+
PullRequestId(hostname='github.com', owner='facebook', name='dotslash', number=24)
44+
45+
Does not match if the pattern appears after the first line.
46+
>>> commit_msg = 'TITLE\nDocument experimental `fetch` subcommand (#24)\nSummary:\nDocument...'
47+
>>> _parse_github_pull_request_url(commit_msg, gh_repo) is None
48+
True
49+
50+
Match the "Pull Request resolved" text.
3151
>>> descr = 'foo\nPull Request resolved: https://github.com/bolinfest/ghstack-testing/pull/71\nbar'
32-
>>> _parse_github_pull_request_url(descr)
52+
>>> _parse_github_pull_request_url(descr, gh_repo)
3353
PullRequestId(hostname='github.com', owner='bolinfest', name='ghstack-testing', number=71)
34-
>>> _parse_github_pull_request_url('') is None
54+
55+
Test a trivial "no match" case.
56+
>>> _parse_github_pull_request_url('', gh_repo) is None
3557
True
3658
"""
59+
# The default for "squash and merge" in the GitHub pull request flow appears
60+
# to put the pull request number at the end of the first line of the commit
61+
# message, so check that first.
62+
match = re.search(r'^.*\(#([1-9][0-9]*)\)\r?(\n|$)', descr)
63+
if match:
64+
result = find_github_repo(repo)
65+
if result.is_ok():
66+
github_repo = result.unwrap()
67+
return PullRequestId(
68+
hostname=github_repo.hostname,
69+
owner=github_repo.owner,
70+
name=github_repo.name,
71+
number=int(match.group(1)),
72+
)
73+
3774
# This is the format used by ghstack, though other variants may be supported
3875
# in the future.
3976
match = re.search(

eden/scm/sapling/ext/github/pr_status.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ def _prefetch(repo, ctx_iter):
4444
pr_store = PullRequestStore(repo)
4545
for batch in util.eachslice(ctx_iter, peek_ahead):
4646
cached = getattr(repo, _PR_STATUS_CACHE, {})
47-
pr_list = {get_pull_request_for_context(pr_store, ctx) for ctx in batch}
47+
pr_list = {get_pull_request_for_context(pr_store, repo, ctx) for ctx in batch}
4848

4949
pr_list = [pr for pr in pr_list if pr and pr not in cached]
5050
if pr_list:

eden/scm/sapling/ext/github/submit.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -710,7 +710,7 @@ async def get_repo(hostname: str, owner: str, name: str) -> Repository:
710710

711711
async def derive_commit_data(node: bytes, repo, store: PullRequestStore) -> CommitData:
712712
ctx = repo[node]
713-
pr_id = get_pull_request_for_context(store, ctx)
713+
pr_id = get_pull_request_for_context(store, repo, ctx)
714714
pr = await get_pull_request_details_or_throw(pr_id) if pr_id else None
715715
msg = None
716716
if pr:

eden/scm/sapling/ext/github/templates.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ def get_pull_request_url_for_rev(repo, ctx, **args) -> Optional[PullRequestId]:
6969
return pull_request_url
7070

7171
store = get_pull_request_store(repo, args["cache"])
72-
pull_request_url = get_pull_request_for_context(store, ctx)
72+
pull_request_url = get_pull_request_for_context(store, repo, ctx)
7373

7474
revcache[_GITHUB_PULL_REQUEST_URL_REVCACHE_KEY] = (
7575
pull_request_url if pull_request_url is not None else _NO_ENTRY

eden/scm/sapling/ext/github/testutil.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,29 @@
66
"""test utilities for doctests in this folder
77
"""
88

9+
from sapling import git
10+
911

1012
class FakeRepo:
1113
pass
1214

1315

16+
"""Designed to be compatible with find_github_repo()."""
17+
class FakeGitHubRepo:
18+
def __init__(
19+
self,
20+
hostname: str = "github.com",
21+
owner: str = "facebook",
22+
name: str = "sapling",
23+
) -> None:
24+
# This should be sufficient to satisfy git.isgitpeer().
25+
self.storerequirements = {git.GIT_STORE_REQUIREMENT}
26+
self.github_url = f"https://{hostname}/{owner}/{name}"
27+
28+
def get_github_url(self) -> str:
29+
return self.github_url
30+
31+
1432
class FakePullRequestStore:
1533
def find_pull_request(self, node: bytes):
1634
return None

0 commit comments

Comments
 (0)