diff --git a/eden/scm/sapling/ext/github/consts/query.py b/eden/scm/sapling/ext/github/consts/query.py index ef111962c8153..77b69e83575c4 100644 --- a/eden/scm/sapling/ext/github/consts/query.py +++ b/eden/scm/sapling/ext/github/consts/query.py @@ -81,6 +81,18 @@ } """ +GRAPHQL_ADD_COMMENT = """ +mutation ($subjectId: ID!, $body: String!) { + addComment(input: {subjectId: $subjectId, body: $body}) { + commentEdge { + node { + id + } + } + } +} +""" + GRAPHQL_CREATE_BRANCH = """ mutation ($repositoryId: ID!, $name: String!, $oid: GitObjectID!) { createRef(input: {repositoryId: $repositoryId, name: $name, oid: $oid}) { diff --git a/eden/scm/sapling/ext/github/gh_submit.py b/eden/scm/sapling/ext/github/gh_submit.py index 6004ef4959eec..2ccc2356d5969 100644 --- a/eden/scm/sapling/ext/github/gh_submit.py +++ b/eden/scm/sapling/ext/github/gh_submit.py @@ -355,6 +355,20 @@ async def update_pull_request( return Ok(result.unwrap()["data"]["updatePullRequest"]["pullRequest"]["id"]) +async def add_comment(hostname: str, subject_id: str, body: str) -> Result[str, str]: + """Adds a comment to an issue or pull request, returning the comment node ID.""" + params: Dict[str, _Params] = { + "query": query.GRAPHQL_ADD_COMMENT, + "subjectId": subject_id, + "body": body, + } + result = await gh_cli.make_request(params, hostname=hostname) + if result.is_err(): + return Err(result.unwrap_err()) + else: + return Ok(result.unwrap()["data"]["addComment"]["commentEdge"]["node"]["id"]) + + async def create_branch( *, hostname: str, repo_id: str, branch_name: str, oid: str ) -> Result[str, str]: diff --git a/eden/scm/sapling/ext/github/mock_utils.py b/eden/scm/sapling/ext/github/mock_utils.py index added9869489b..423a7bac99f05 100644 --- a/eden/scm/sapling/ext/github/mock_utils.py +++ b/eden/scm/sapling/ext/github/mock_utils.py @@ -286,7 +286,7 @@ def expect_update_pr_request( ) -> "UpdatePrRequest": if not stack_pr_ids: stack_pr_ids = [pr_number] - stack_pr_ids = list(reversed(sorted(stack_pr_ids))) + stack_pr_ids = sorted(stack_pr_ids, reverse=True) if len(stack_pr_ids) > 1: pr_list = [ @@ -313,6 +313,19 @@ def expect_update_pr_request( self._add_request(key, request) return request + def expect_add_comment_request( + self, pr_id: str, body: str + ) -> "AddCommentRequest": + params: ParamsType = { + "query": query.GRAPHQL_ADD_COMMENT, + "subjectId": pr_id, + "body": body, + } + key = create_request_key(params, self.hostname) + request = AddCommentRequest(key, pr_id) + self._add_request(key, request) + return request + def expect_get_username_request( self, ) -> "GetUsernameRequest": @@ -545,6 +558,29 @@ def get_response(self) -> Result[JsonDict, str]: return self._response +class AddCommentRequest(MockRequest): + def __init__(self, key: str, pr_id: str) -> None: + self._key = key + self._response: Optional[Result[JsonDict, str]] = None + + self._pr_id = pr_id + + def and_respond(self): + data = { + "data": { + "addComment": { + "commentEdge": {"node": {"id": f"comment_{self._pr_id}"}} + } + } + } + self._response = Ok(data) + + def get_response(self) -> Result[JsonDict, str]: + if self._response is None: + raise MockResponseNotSet(self._key) + return self._response + + class GetUsernameRequest(MockRequest): def __init__(self, key: str) -> None: self._key = key diff --git a/eden/scm/sapling/ext/github/submit.py b/eden/scm/sapling/ext/github/submit.py index 423bdaff16bcf..7bbe4b61dd573 100644 --- a/eden/scm/sapling/ext/github/submit.py +++ b/eden/scm/sapling/ext/github/submit.py @@ -8,7 +8,7 @@ import webbrowser from dataclasses import dataclass from enum import Enum -from typing import Any, List, Optional, Tuple +from typing import Any, List, Optional, Set, Tuple from sapling import error, formatter, git, hintutil, templatekw from sapling.context import changectx @@ -128,6 +128,14 @@ class PullRequestParams: number: int +@dataclass +class PullRequestDeltaComment: + pr: PullRequestDetails + old_head_oid: str + new_head_oid: str + body: str + + async def get_partitions(ui, repo, store, filter) -> List[List[CommitData]]: commits_to_process = await asyncio.gather( *[derive_commit_data(node, repo, store) for node in repo.nodes(filter)] @@ -168,15 +176,83 @@ async def update_commits_in_stack( return 0 origin = get_push_origin(ui) use_placeholder_strategy = ui.configbool("github", "placeholder-strategy") - if use_placeholder_strategy: - params = await create_placeholder_strategy_params( - ui, partitions, github_repo, origin + params = await create_submit_strategy_params( + ui, partitions, github_repo, origin, use_placeholder_strategy + ) + check_pull_request_creation_limit(ui, params) + + refs_to_update = params.refs_to_update + + gitdir = None + + def get_gitdir() -> str: + nonlocal gitdir + if gitdir is None: + gitdir = git.readgitdir(repo) + if not gitdir: + raise error.Abort(_("could not find gitdir")) + return gitdir + + if not refs_to_update: + ui.status_err(_("no pull requests to update\n")) + return 0 + + repository = params.repository + delta_comments, repository = await prepare_pull_request_delta_comments( + ui, partitions, repository, origin, github_repo + ) + if delta_comments: + await archive_delta_comment_heads( + delta_comments, ui, origin, none_throws(repository), get_gitdir ) - else: - params = await create_serial_strategy_params( + + repository = await update_single_workflow_base_branches( + workflow, partitions, repository, origin, github_repo, ui + ) + + gitdir = get_gitdir() + git_push_args = ["push", "--force", origin] + refs_to_update + ui.status_err(_("pushing %d to %s\n") % (len(refs_to_update), origin)) + run_git_command(git_push_args, gitdir) + + repository = await create_pull_requests_after_push( + params, + use_placeholder_strategy, + workflow, + repository, + origin, + github_repo, + store, + ui, + is_draft, + ) + repository = await rewrite_pull_request_bodies_and_archive_tip( + partitions, workflow, repository, origin, github_repo, ui, get_gitdir + ) + await post_pull_request_delta_comments(delta_comments, repository, ui) + + # Open pull requests in browser if --open flag was specified + if is_open: + open_pull_requests(partitions, ui) + + return 0 + + +async def create_submit_strategy_params( + ui, + partitions: List[List[CommitData]], + github_repo: GitHubRepo, + origin: str, + use_placeholder_strategy: bool, +): + if use_placeholder_strategy: + return await create_placeholder_strategy_params( ui, partitions, github_repo, origin ) + return await create_serial_strategy_params(ui, partitions, github_repo, origin) + +def check_pull_request_creation_limit(ui, params) -> None: max_pull_requests_to_create = ui.configint("github", "max-prs-to-create", "5") if ( max_pull_requests_to_create >= 0 @@ -194,23 +270,26 @@ async def update_commits_in_stack( ) ) - refs_to_update = params.refs_to_update - gitdir = None - - def get_gitdir() -> str: - nonlocal gitdir - if gitdir is None: - gitdir = git.readgitdir(repo) - if not gitdir: - raise error.Abort(_("could not find gitdir")) - return gitdir +async def update_single_workflow_base_branches( + workflow: SubmitWorkflow, + partitions: List[List[CommitData]], + repository: Optional[Repository], + origin: str, + github_repo: GitHubRepo, + ui, +) -> Optional[Repository]: + if workflow != SubmitWorkflow.SINGLE: + return repository - if not refs_to_update: - ui.status_err(_("no pull requests to update\n")) - return 0 + existing_prs = [ + p for p in partitions if p[0].pr and p[0].pr.state == PullRequestState.OPEN + ] + if not existing_prs: + return repository - repository = params.repository + if not repository: + repository = await get_repository_for_origin(origin, github_repo.hostname) # For the SINGLE workflow, we must update the base branch on existing PRs # BEFORE pushing the new branch contents. Otherwise, when commits are @@ -218,64 +297,83 @@ def get_gitdir() -> str: # in its (old) base branch and auto-close the PR as "merged". # # See https://github.com/facebook/sapling/issues/1275 - if workflow == SubmitWorkflow.SINGLE: - existing_prs = [ - p for p in partitions if p[0].pr and p[0].pr.state == PullRequestState.OPEN - ] - if existing_prs: - if not repository: - repository = await get_repository_for_origin( - origin, github_repo.hostname - ) - # Update base branches on existing PRs before pushing. - # Process from bottom of stack to top so bases are set correctly. - for index in range(len(partitions)): - partition = partitions[index] - pr = partition[0].pr - if not pr or pr.state != PullRequestState.OPEN: - continue - base = repository.get_base_branch() - if index < len(partitions) - 1: - base = none_throws(partitions[index + 1][0].head_branch_name) - result = await gh_submit.update_pull_request( - repository.hostname, pr.node_id, pr.title, pr.body, base - ) - if result.is_err(): - ui.status_err( - _("warning, updating base for #%d may not have succeeded: %s\n") - % (pr.number, result.unwrap_err()) - ) - else: - ui.status_err(_("updated base for %s\n") % pr.url) - - gitdir = get_gitdir() - git_push_args = ["push", "--force", origin] + refs_to_update - ui.status_err(_("pushing %d to %s\n") % (len(refs_to_update), origin)) - run_git_command(git_push_args, gitdir) - - if params.pull_requests_to_create: - if not repository: - repository = await get_repository_for_origin(origin, github_repo.hostname) - if use_placeholder_strategy: - assert isinstance(params, PlaceholderStrategyParams) - await create_pull_requests_from_placeholder_issues( - params.pull_requests_to_create, - workflow, - repository, - store, - ui, - is_draft, + # + # Update base branches on existing PRs before pushing. + # Process from bottom of stack to top so bases are set correctly. + for index in range(len(partitions)): + partition = partitions[index] + pr = partition[0].pr + if not pr or pr.state != PullRequestState.OPEN: + continue + base = repository.get_base_branch() + if index < len(partitions) - 1: + base = none_throws(partitions[index + 1][0].head_branch_name) + result = await gh_submit.update_pull_request( + repository.hostname, pr.node_id, pr.title, pr.body, base + ) + if result.is_err(): + ui.status_err( + _("warning, updating base for #%d may not have succeeded: %s\n") + % (pr.number, result.unwrap_err()) ) else: - assert isinstance(params, SerialStrategyParams) - await create_pull_requests_serially( - params.pull_requests_to_create, - workflow, - repository, - store, - ui, - is_draft, - ) + ui.status_err(_("updated base for %s\n") % pr.url) + + return repository + + +async def create_pull_requests_after_push( + params, + use_placeholder_strategy: bool, + workflow: SubmitWorkflow, + repository: Optional[Repository], + origin: str, + github_repo: GitHubRepo, + store: PullRequestStore, + ui, + is_draft: bool, +) -> Optional[Repository]: + if not params.pull_requests_to_create: + return repository + + if not repository: + repository = await get_repository_for_origin(origin, github_repo.hostname) + + if use_placeholder_strategy: + assert isinstance(params, PlaceholderStrategyParams) + await create_pull_requests_from_placeholder_issues( + params.pull_requests_to_create, + workflow, + repository, + store, + ui, + is_draft, + ) + else: + assert isinstance(params, SerialStrategyParams) + await create_pull_requests_serially( + params.pull_requests_to_create, + workflow, + repository, + store, + ui, + is_draft, + ) + + return repository + + +async def rewrite_pull_request_bodies_and_archive_tip( + partitions: List[List[CommitData]], + workflow: SubmitWorkflow, + repository: Optional[Repository], + origin: str, + github_repo: GitHubRepo, + ui, + get_gitdir, +) -> Repository: + if not repository: + repository = await get_repository_for_origin(origin, github_repo.hostname) # Now that each pull request has a named branch pushed to GitHub, we can # create/update the pull request title and body, as appropriate. @@ -285,9 +383,6 @@ def get_gitdir() -> str: # Add the head of the stack to the sapling-pr-archive branch. tip = hex(partitions[0][0].node) - - if not repository: - repository = await get_repository_for_origin(origin, github_repo.hostname) rewrite_and_archive_requests = [ rewrite_pull_request_body( partitions, index, workflow, pr_numbers_and_num_commits, repository, ui @@ -303,15 +398,140 @@ def get_gitdir() -> str: ) ] await asyncio.gather(*rewrite_and_archive_requests) + return repository - # Open pull requests in browser if --open flag was specified - if is_open: - pr_urls = [none_throws(p[0].pr).url for p in partitions if p[0].pr] - for url in pr_urls: - ui.status_err(_("opening %s\n") % url) - webbrowser.open(url) - return 0 +def open_pull_requests(partitions: List[List[CommitData]], ui) -> None: + pr_urls = [none_throws(p[0].pr).url for p in partitions if p[0].pr] + for url in pr_urls: + ui.status_err(_("opening %s\n") % url) + webbrowser.open(url) + + +async def prepare_pull_request_delta_comments( + ui, + partitions: List[List[CommitData]], + repository: Optional[Repository], + origin: str, + github_repo: GitHubRepo, +) -> Tuple[List[PullRequestDeltaComment], Optional[Repository]]: + if not ui.configbool("github", "pr-submit-comment-deltas"): + return [], repository + + if not repository: + repository = await get_repository_for_origin(origin, github_repo.hostname) + return collect_pull_request_delta_comments(partitions, repository), repository + + +def collect_pull_request_delta_comments( + partitions: List[List[CommitData]], + repository: Repository, +) -> List[PullRequestDeltaComment]: + comments = [] + for partition in partitions: + head_commit_data = partition[0] + pr = head_commit_data.pr + if not pr or pr.state != PullRequestState.OPEN: + continue + + old_head_oid = pr.head_oid + new_head_oid = hex(head_commit_data.node) + if old_head_oid == new_head_oid: + continue + + comments.append( + PullRequestDeltaComment( + pr=pr, + old_head_oid=old_head_oid, + new_head_oid=new_head_oid, + body=create_pull_request_delta_comment_body( + repository, old_head_oid, new_head_oid + ), + ) + ) + return comments + + +def create_pull_request_delta_comment_body( + repository: Repository, old_head_oid: str, new_head_oid: str +) -> str: + owner, name = repository.get_upstream_owner_and_name() + compare_url = ( + f"https://{repository.hostname}/{owner}/{name}/compare/" + f"{old_head_oid}..{new_head_oid}" + ) + return "\n".join( + [ + f"[//]: # (sapling-pr-delta old={old_head_oid} new={new_head_oid})", + "", + "Sapling updated this PR.", + "", + f"Review delta: {compare_url}", + f"Previous head: `{old_head_oid[:12]}`", + f"New head: `{new_head_oid[:12]}`", + ] + ) + + +async def archive_delta_comment_heads( + comments: List[PullRequestDeltaComment], + ui, + origin: str, + repository: Repository, + get_gitdir, +) -> None: + # Add the old head commits to the sapling-pr-archive branch before pushing. + # Otherwise, compare links may depend on commits GitHub no longer considers + # reachable after the PR branches are force-pushed. + archived: Set[str] = set() + archive_mutations = 0 + for comment in comments: + if comment.old_head_oid in archived: + continue + archived.add(comment.old_head_oid) + await maybe_sleep_between_github_mutations(archive_mutations) + archive_mutations += 1 + try: + await add_commit_to_archives( + oid_to_archive=comment.old_head_oid, + ui=ui, + origin=origin, + repository=repository, + get_gitdir=get_gitdir, + ) + except error.Abort as ex: + ui.status_err( + _("warning, could not archive %s for PR delta link: %s\n") + % (comment.old_head_oid, ex) + ) + + +async def maybe_sleep_between_github_mutations(previous_mutations: int) -> None: + if previous_mutations <= 0: + return + + # GitHub recommends avoiding concurrent mutative requests and waiting at + # least one second between them to reduce secondary rate-limit failures. + # Open question: should `sl pr submit` have a shared limiter across all of + # its GitHub mutations instead of pacing only this optional feature? + await asyncio.sleep(1) + + +async def post_pull_request_delta_comments( + comments: List[PullRequestDeltaComment], repository: Repository, ui +) -> None: + for index, comment in enumerate(comments): + await maybe_sleep_between_github_mutations(index) + result = await gh_submit.add_comment( + repository.hostname, comment.pr.node_id, comment.body + ) + if result.is_err(): + ui.status_err( + _("warning, posting update delta for #%d may not have succeeded: %s\n") + % (comment.pr.number, result.unwrap_err()) + ) + else: + ui.status_err(_("posted update delta for %s\n") % comment.pr.url) async def rewrite_pull_request_body( diff --git a/eden/scm/tests/github/mock_delta_update.py b/eden/scm/tests/github/mock_delta_update.py new file mode 100644 index 0000000000000..2d4877be70ae4 --- /dev/null +++ b/eden/scm/tests/github/mock_delta_update.py @@ -0,0 +1,163 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This software may be used and distributed according to the terms of the +# GNU General Public License version 2. + +import os + +from sapling import error, extensions +from sapling.ext.github import github_gh_cli, submit +from sapling.ext.github.consts import query +from sapling.ext.github.mock_utils import ( + mock_run_git_command, + OWNER, + OWNER_ID, + REPO_ID, + REPO_NAME, + USER_NAME, +) +from sapling.result import Ok + + +ARCHIVED_DELTA_HEAD = False +PUSHED = False + + +def _env(name: str) -> str: + value = os.environ.get(name) + if not value: + raise error.Abort(f"missing {name}") + return value + + +async def _make_request( + real_make_request, + params, + hostname, + endpoint="graphql", + method=None, +): + request_query = params.get("query") + if request_query == query.GRAPHQL_GET_REPOSITORY: + return Ok( + { + "data": { + "repository": { + "id": REPO_ID, + "owner": {"id": OWNER_ID, "login": OWNER}, + "name": REPO_NAME, + "isFork": False, + "defaultBranchRef": {"name": "main"}, + "parent": None, + } + } + } + ) + + if request_query == query.GRAPHQL_GET_PULL_REQUEST: + number = params["number"] + head_oid = _env(f"SL_TEST_PR{number}_HEAD") + title = "one" if number == 42 else "two" + return Ok( + { + "data": { + "repository": { + "pullRequest": { + "id": f"PR_id_{number}", + "url": f"https://github.com/{OWNER}/{REPO_NAME}/pull/{number}", + "state": "OPEN", + "headRefOid": head_oid, + "headRefName": f"pr{number}", + "baseRefOid": "base", + "baseRefName": "main", + "body": "", + "title": title, + } + } + } + } + ) + + if request_query == query.GRAPHQL_UPDATE_PULL_REQUEST: + return Ok( + { + "data": { + "updatePullRequest": { + "pullRequest": {"id": params["pullRequestId"]} + } + } + } + ) + + if request_query == query.GRAPHQL_GET_LOGIN: + return Ok({"data": {"viewer": {"login": USER_NAME}}}) + + if request_query == query.GRAPHQL_MERGE_BRANCH: + global ARCHIVED_DELTA_HEAD + old_head = _env("SL_TEST_DELTA_OLD") + new_head = _env("SL_TEST_DELTA_NEW") + if params["head"] == old_head: + if PUSHED: + raise error.Abort("old delta head archived after push") + ARCHIVED_DELTA_HEAD = True + elif params["head"] == new_head: + if not PUSHED: + raise error.Abort("new head archived before push") + else: + raise error.Abort(f"unexpected archive head: {params['head']}") + return Ok( + { + "data": { + "mergeBranch": { + "mergeCommit": {"oid": f"merge-{params['head'][:12]}"} + } + } + } + ) + + if request_query == query.GRAPHQL_ADD_COMMENT: + expected_pr = _env("SL_TEST_DELTA_PR") + old_head = _env("SL_TEST_DELTA_OLD") + new_head = _env("SL_TEST_DELTA_NEW") + if not ARCHIVED_DELTA_HEAD: + raise error.Abort("delta comment posted before old head was archived") + if not PUSHED: + raise error.Abort("delta comment posted before push") + expected_subject = f"PR_id_{expected_pr}" + if params["subjectId"] != expected_subject: + raise error.Abort( + f"expected delta comment on {expected_subject}, got {params['subjectId']}" + ) + body = params["body"] + expected_compare = ( + f"https://github.com/{OWNER}/{REPO_NAME}/compare/{old_head}..{new_head}" + ) + for expected in [ + f"sapling-pr-delta old={old_head} new={new_head}", + expected_compare, + ]: + if expected not in body: + raise error.Abort(f"missing {expected!r} from {body!r}") + return Ok( + { + "data": { + "addComment": {"commentEdge": {"node": {"id": "delta_comment"}}} + } + } + ) + + raise error.Abort(f"unexpected GitHub request: {params}") + + +def _run_git_command(real_run_git_command, args, gitdir): + global PUSHED + if args and args[0] == "push": + if not ARCHIVED_DELTA_HEAD: + raise error.Abort("pushed before old delta head was archived") + PUSHED = True + return mock_run_git_command(real_run_git_command, args, gitdir) + + +def uisetup(ui): + extensions.wrapfunction(github_gh_cli, "_make_request", _make_request) + extensions.wrapfunction(submit, "run_git_command", _run_git_command) diff --git a/eden/scm/tests/test-ext-github-pr-submit-delta-comments.t b/eden/scm/tests/test-ext-github-pr-submit-delta-comments.t new file mode 100644 index 0000000000000..9858213bfa0ac --- /dev/null +++ b/eden/scm/tests/test-ext-github-pr-submit-delta-comments.t @@ -0,0 +1,72 @@ +#require git no-eden no-windows + +Test that `sl pr submit` can post GitHub compare links for updated PRs. + + $ eagerepo + $ enable github + $ setconfig hint.ack='*' + $ export SL_TEST_GH_URL=https://github.com/facebook/test_github_repo.git + $ . $TESTDIR/git.sh + +Overlap workflow posts a delta comment for the updated PR. + + $ setconfig github.pr-workflow=overlap + $ sl init --git overlap + $ cd overlap + $ echo a > a1 + $ sl ci -Aqm one + $ echo a >> a1 + $ sl ci -Aqm two + $ sl pr submit --config extensions.pr_submit_create_overlap=$TESTDIR/github/mock_create_prs.py + pushing 2 to https://github.com/facebook/test_github_repo.git + created new pull request: https://github.com/facebook/test_github_repo/pull/42 + created new pull request: https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/42 + + $ export SL_TEST_PR42_HEAD=$(sl log -r ".^" -T "{node}") + $ export SL_TEST_PR43_HEAD=$(sl log -r "." -T "{node}") + $ echo b >> a1 + $ sl amend -qm "two amended" + $ export SL_TEST_DELTA_PR=43 + $ export SL_TEST_DELTA_OLD=$SL_TEST_PR43_HEAD + $ export SL_TEST_DELTA_NEW=$(sl log -r "." -T "{node}") + $ sl pr submit --config github.pr-submit-comment-deltas=true --config extensions.pr_submit_delta_overlap=$TESTDIR/github/mock_delta_update.py + #42 is up-to-date + pushing 1 to https://github.com/facebook/test_github_repo.git + updated body for https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/42 + posted update delta for https://github.com/facebook/test_github_repo/pull/43 + +Single workflow posts the same style of delta comment. + + $ cd .. + $ setconfig github.pr-workflow=single + $ sl init --git single + $ cd single + $ echo a > a1 + $ sl ci -Aqm one + $ echo a >> a1 + $ sl ci -Aqm two + $ sl pr submit --config extensions.pr_submit_create_single=$TESTDIR/github/mock_create_prs.py + pushing 2 to https://github.com/facebook/test_github_repo.git + created new pull request: https://github.com/facebook/test_github_repo/pull/42 + created new pull request: https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/42 + + $ export SL_TEST_PR42_HEAD=$(sl log -r ".^" -T "{node}") + $ export SL_TEST_PR43_HEAD=$(sl log -r "." -T "{node}") + $ echo b >> a1 + $ sl amend -qm "two amended" + $ export SL_TEST_DELTA_PR=43 + $ export SL_TEST_DELTA_OLD=$SL_TEST_PR43_HEAD + $ export SL_TEST_DELTA_NEW=$(sl log -r "." -T "{node}") + $ sl pr submit --config github.pr-submit-comment-deltas=true --config extensions.pr_submit_delta_single=$TESTDIR/github/mock_delta_update.py + #42 is up-to-date + updated base for https://github.com/facebook/test_github_repo/pull/43 + updated base for https://github.com/facebook/test_github_repo/pull/42 + pushing 1 to https://github.com/facebook/test_github_repo.git + updated body for https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/42 + posted update delta for https://github.com/facebook/test_github_repo/pull/43 diff --git a/website/docs/git/github.md b/website/docs/git/github.md index 084548012ec3b..5c8af3f515bba 100644 --- a/website/docs/git/github.md +++ b/website/docs/git/github.md @@ -53,6 +53,19 @@ See the dedicated [Sapling Stack](/docs/git/sapling-stack.md) page for more info - Creates "overlapping" pull requests that may be confusing to reviewers using the GitHub pull request UI. Reviewers are strongly encouraged to use [ReviewStack](/docs/addons/reviewstack.md) for code review instead of GitHub. +When reviewers do use GitHub, you can ask Sapling to post a short update +comment with a GitHub compare link each time `sl pr submit` updates an existing +pull request: + +``` +sl config github.pr-submit-comment-deltas true +``` + +The compare link shows the delta between the previous submitted PR head and the +new PR head. GitHub's Files Changed tab still shows the current PR diff against +its base, so ReviewStack remains the preferred stack-aware review UI. Large +stacks may still create one comment for each updated pull request. + :::tip You can use the `pr` revset to automatically pull and checkout GitHub pull request. For example, `sl goto pr123`. See `sl help revsets` for more info.