Skip to content

Commit

Permalink
Recognize PR# at end of commit title for github_pull_request_* templates
Browse files Browse the repository at this point in the history
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
```
  • Loading branch information
bolinfest committed Apr 19, 2024
1 parent 1d26709 commit d241767
Show file tree
Hide file tree
Showing 7 changed files with 65 additions and 8 deletions.
2 changes: 2 additions & 0 deletions eden/scm/sapling/ext/github/github_repo_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ def find_github_repo(repo) -> Result[GitHubRepo, NotGitHubRepoError]:
test_url = os.environ.get("SL_TEST_GH_URL")
if test_url:
url = util.url(test_url)
elif hasattr(repo, "get_github_url"):
url = util.url(repo.get_github_url())
else:
url = repo.ui.paths.get("default", "default-push").url
except AttributeError: # ex. paths.default is not set
Expand Down
2 changes: 1 addition & 1 deletion eden/scm/sapling/ext/github/pr_marker.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def _get_draft_commits(repo) -> t.Dict[PullRequestId, t.Set[Node]]:

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


def _get_landed_commits(
Expand Down
45 changes: 41 additions & 4 deletions eden/scm/sapling/ext/github/pr_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@
import re
from typing import Optional

from .github_repo_util import find_github_repo
from .pullrequest import PullRequestId
from .pullrequeststore import PullRequestStore


def get_pull_request_for_context(
store: PullRequestStore,
repo,
ctx,
) -> Optional[PullRequestId]:
"""Returns a pull request associated with a commit context, if any. Checks
Expand All @@ -20,20 +22,55 @@ def get_pull_request_for_context(
"""
node = ctx.node()
pr = store.find_pull_request(node)
return pr if pr else _parse_github_pull_request_url(ctx.description())
return pr if pr else _parse_github_pull_request_url(ctx.description(), repo)


def _parse_github_pull_request_url(descr: str) -> Optional[PullRequestId]:
def _parse_github_pull_request_url(descr: str, repo) -> Optional[PullRequestId]:
r"""If the commit message has a comment in a special format that indicates
it is associated with a GitHub pull request, returns the corresponding
PullRequestId.
Match the number at the end of the title.
>>> from .testutil import FakeGitHubRepo
>>> gh_repo = FakeGitHubRepo(name="dotslash")
>>> commit_msg = 'Document experimental `fetch` subcommand (#24)\nSummary:\nDocument...'
>>> _parse_github_pull_request_url(commit_msg, gh_repo)
PullRequestId(hostname='github.com', owner='facebook', name='dotslash', number=24)
Should work with a single line title.
>>> commit_msg = 'Document experimental `fetch` subcommand (#24)'
>>> _parse_github_pull_request_url(commit_msg, gh_repo)
PullRequestId(hostname='github.com', owner='facebook', name='dotslash', number=24)
Does not match if the pattern appears after the first line.
>>> commit_msg = 'TITLE\nDocument experimental `fetch` subcommand (#24)\nSummary:\nDocument...'
>>> _parse_github_pull_request_url(commit_msg, gh_repo) is None
True
Match the "Pull Request resolved" text.
>>> descr = 'foo\nPull Request resolved: https://github.com/bolinfest/ghstack-testing/pull/71\nbar'
>>> _parse_github_pull_request_url(descr)
>>> _parse_github_pull_request_url(descr, gh_repo)
PullRequestId(hostname='github.com', owner='bolinfest', name='ghstack-testing', number=71)
>>> _parse_github_pull_request_url('') is None
Test a trivial "no match" case.
>>> _parse_github_pull_request_url('', gh_repo) is None
True
"""
# The default for "squash and merge" in the GitHub pull request flow appears
# to put the pull request number at the end of the first line of the commit
# message, so check that first.
match = re.search(r'^.*\(#([1-9][0-9]*)\)\r?(\n|$)', descr)
if match:
result = find_github_repo(repo)
if result.is_ok():
github_repo = result.unwrap()
return PullRequestId(
hostname=github_repo.hostname,
owner=github_repo.owner,
name=github_repo.name,
number=int(match.group(1)),
)

# This is the format used by ghstack, though other variants may be supported
# in the future.
match = re.search(
Expand Down
2 changes: 1 addition & 1 deletion eden/scm/sapling/ext/github/pr_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def _prefetch(repo, ctx_iter):
pr_store = PullRequestStore(repo)
for batch in util.eachslice(ctx_iter, peek_ahead):
cached = getattr(repo, _PR_STATUS_CACHE, {})
pr_list = {get_pull_request_for_context(pr_store, ctx) for ctx in batch}
pr_list = {get_pull_request_for_context(pr_store, repo, ctx) for ctx in batch}

pr_list = [pr for pr in pr_list if pr and pr not in cached]
if pr_list:
Expand Down
2 changes: 1 addition & 1 deletion eden/scm/sapling/ext/github/submit.py
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,7 @@ async def get_repo(hostname: str, owner: str, name: str) -> Repository:

async def derive_commit_data(node: bytes, repo, store: PullRequestStore) -> CommitData:
ctx = repo[node]
pr_id = get_pull_request_for_context(store, ctx)
pr_id = get_pull_request_for_context(store, repo, ctx)
pr = await get_pull_request_details_or_throw(pr_id) if pr_id else None
msg = None
if pr:
Expand Down
2 changes: 1 addition & 1 deletion eden/scm/sapling/ext/github/templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def get_pull_request_url_for_rev(repo, ctx, **args) -> Optional[PullRequestId]:
return pull_request_url

store = get_pull_request_store(repo, args["cache"])
pull_request_url = get_pull_request_for_context(store, ctx)
pull_request_url = get_pull_request_for_context(store, repo, ctx)

revcache[_GITHUB_PULL_REQUEST_URL_REVCACHE_KEY] = (
pull_request_url if pull_request_url is not None else _NO_ENTRY
Expand Down
18 changes: 18 additions & 0 deletions eden/scm/sapling/ext/github/testutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,29 @@
"""test utilities for doctests in this folder
"""

from sapling import git


class FakeRepo:
pass


"""Designed to be compatible with find_github_repo()."""
class FakeGitHubRepo:
def __init__(
self,
hostname: str = "github.com",
owner: str = "facebook",
name: str = "sapling",
) -> None:
# This should be sufficient to satisfy git.isgitpeer().
self.storerequirements = {git.GIT_STORE_REQUIREMENT}
self.github_url = f"https://{hostname}/{owner}/{name}"

def get_github_url(self) -> str:
return self.github_url


class FakePullRequestStore:
def find_pull_request(self, node: bytes):
return None
Expand Down

0 comments on commit d241767

Please sign in to comment.