diff --git a/.github/workflows/reviewstack.dev-deploy.yml b/.github/workflows/reviewstack.dev-deploy.yml index 3e509023a31fc..0ed3c0b784df9 100644 --- a/.github/workflows/reviewstack.dev-deploy.yml +++ b/.github/workflows/reviewstack.dev-deploy.yml @@ -1,26 +1,33 @@ -name: Publish https://reviewstack.dev +name: Deploy ReviewStack to Cloudflare Pages on: workflow_dispatch: - schedule: - - cron: '0 0 * * 1-5' + push: + branches: + - main + paths: + - 'eden/contrib/reviewstack/**' + - 'eden/contrib/reviewstack.dev/**' + - 'eden/contrib/shared/**' + - '.github/workflows/reviewstack.dev-deploy.yml' jobs: deploy: runs-on: ubuntu-22.04 - # Our build container already has Node, Yarn, and Python installed. - container: - image: ${{ format('ghcr.io/{0}/build_ubuntu_22_04:latest', github.repository) }} concurrency: group: ${{ github.workflow }}-${{ github.ref }} steps: - name: Checkout Code - uses: actions/checkout@v6 - - name: Grant Access - run: git config --global --add safe.directory "$PWD" + uses: actions/checkout@v4 + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: '20' + cache: 'yarn' + cache-dependency-path: eden/contrib/yarn.lock - name: Install dependencies working-directory: ./eden/contrib/ - run: yarn install --prefer-offline + run: yarn install # Build codegen and then do some sanity checks so we don't push the site # when the tests are broken. @@ -37,11 +44,10 @@ jobs: working-directory: ./eden/contrib/reviewstack.dev run: yarn release - # Push to the release branch. - - name: Deploy - uses: peaceiris/actions-gh-pages@v4 - if: ${{ github.ref == 'refs/heads/main' }} + # Deploy to Cloudflare Pages + - name: Deploy to Cloudflare Pages + uses: cloudflare/wrangler-action@v3 with: - github_token: ${{ secrets.GITHUB_TOKEN }} - publish_branch: reviewstack.dev-prod - publish_dir: ./eden/contrib/reviewstack.dev/build + apiToken: ${{ secrets.CLOUDFLARE_API_TOKEN }} + accountId: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} + command: pages deploy ./eden/contrib/reviewstack.dev/build --project-name=reviewstack diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000000000..84f442c0fae07 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,111 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## Project Overview + +Sapling SCM is a cross-platform, highly scalable, Git-compatible source control system. It consists of three main components: + +- **Sapling CLI** (`eden/scm/`): The `sl` command-line tool, based on Mercurial. Written in Python and Rust. +- **Interactive Smartlog (ISL)** (`addons/`): Web-based and VS Code UI for repository visualization. React/TypeScript. +- **Mononoke** (`eden/mononoke/`): Server-side distributed source control server (Rust). Not yet publicly supported. +- **EdenFS** (`eden/fs/`): Virtual filesystem for large checkouts (C++). Not yet publicly supported. + +## Build Commands + +### Sapling CLI (primary development) + +From `eden/scm/`: + +```bash +make oss # Build OSS version for local usage (creates ./sl binary) +make install-oss # Install CLI to $PREFIX/bin +make local # Build for inplace usage +make clean # Remove build artifacts +``` + +### Running Tests + +```bash +# All tests (from eden/scm/) +make tests + +# Single test +make test-foo # Runs test-foo.t + +# Using getdeps (full CI-style testing) +./build/fbcode_builder/getdeps.py test --allow-system-packages --src-dir=. sapling + +# Single test with getdeps +./build/fbcode_builder/getdeps.py test --allow-system-packages --src-dir=. sapling --retry 0 --filter test-foo.t +``` + +### ISL (Interactive Smartlog) + +From `addons/`: + +```bash +yarn install # Install dependencies +yarn dev # Start dev server for ISL +``` + +From `addons/isl/`: + +```bash +yarn start # Start Vite dev server +yarn build # Production build +yarn test # Run Jest tests +yarn eslint # Lint TypeScript/React code +``` + +### Website + +From `website/`: + +```bash +yarn start # Start Docusaurus dev server +yarn build # Build static site +yarn lint # Run ESLint and Stylelint +yarn format:diff # Check formatting with Prettier +yarn typecheck # TypeScript type checking +``` + +## Architecture + +### Sapling CLI (`eden/scm/`) + +- `sapling/` - Python CLI modules and commands +- `lib/` - Rust libraries (117+ modules): dag, indexedlog, gitcompat, checkout, clone, etc. +- `exec/` - Rust executables: hgmain, scm_daemon +- `tests/` - `.t` test files (1000+) + +### ISL (`addons/`) + +Yarn workspace with: +- `isl/` - React/Vite web UI +- `isl-server/` - Node.js backend +- `vscode/` - VS Code extension +- `shared/` - Shared utilities +- `components/` - Reusable UI components + +## Code Style + +- 2 spaces indentation +- 80 character line length +- Rust: 2024 edition, rustfmt configured in `.rustfmt.toml` +- TypeScript/JavaScript: ESLint + Prettier +- Python: Flake8 + +## Dependencies + +- Python 3.8+ +- Rust (stable) +- CMake 3.8+ +- OpenSSL +- Node.js + Yarn 1.22 (for ISL/addons) + +## Git Workflow + +- Branch from `main` +- Developed internally at Meta, exported to GitHub +- CLA required for contributions (https://code.facebook.com/cla) diff --git a/CLAUDE.md b/CLAUDE.md new file mode 120000 index 0000000000000..47dc3e3d863cf --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/addons/components/KeyboardShortcuts.tsx b/addons/components/KeyboardShortcuts.tsx index ddb14cce93491..9d7bcb13b2b5a 100644 --- a/addons/components/KeyboardShortcuts.tsx +++ b/addons/components/KeyboardShortcuts.tsx @@ -43,6 +43,7 @@ export enum KeyCode { R = 82, S = 83, T = 84, + Comma = 188, Period = 190, QuestionMark = 191, SingleQuote = 222, diff --git a/addons/isl-server/src/CodeReviewProvider.ts b/addons/isl-server/src/CodeReviewProvider.ts index 65db0130103d5..98574617bd94f 100644 --- a/addons/isl-server/src/CodeReviewProvider.ts +++ b/addons/isl-server/src/CodeReviewProvider.ts @@ -29,7 +29,7 @@ export type DiffSummaries = Map; export interface CodeReviewProvider { triggerDiffSummariesFetch(diffs: Array): unknown; - onChangeDiffSummaries(callback: (result: Result) => unknown): Disposable; + onChangeDiffSummaries(callback: (result: Result, currentUser?: string) => unknown): Disposable; /** Run a command not handled within sapling, such as a separate submit handler */ runExternalCommand?( diff --git a/addons/isl-server/src/ServerToClientAPI.ts b/addons/isl-server/src/ServerToClientAPI.ts index 956268ee757dc..63f6adff0f08e 100644 --- a/addons/isl-server/src/ServerToClientAPI.ts +++ b/addons/isl-server/src/ServerToClientAPI.ts @@ -132,8 +132,8 @@ export default class ServerToClientAPI { if (repo.codeReviewProvider != null) { this.repoDisposables.push( - repo.codeReviewProvider.onChangeDiffSummaries(value => { - this.postMessage({type: 'fetchedDiffSummaries', summaries: value}); + repo.codeReviewProvider.onChangeDiffSummaries((value, currentUser) => { + this.postMessage({type: 'fetchedDiffSummaries', summaries: value, currentUser}); }), ); } diff --git a/addons/isl-server/src/analytics/eventNames.ts b/addons/isl-server/src/analytics/eventNames.ts index b802275a45449..e0a48db0a0f6a 100644 --- a/addons/isl-server/src/analytics/eventNames.ts +++ b/addons/isl-server/src/analytics/eventNames.ts @@ -107,6 +107,7 @@ export type TrackEventName = | 'PrSubmitOperation' | 'PullOperation' | 'PullRevOperation' + | 'PullStackOperation' | 'PurgeOperation' | 'RebaseWarningTimeout' | 'RebaseKeepOperation' diff --git a/addons/isl-server/src/github/generated/graphql.ts b/addons/isl-server/src/github/generated/graphql.ts index 00ef5939473b4..d9f9d79124f01 100644 --- a/addons/isl-server/src/github/generated/graphql.ts +++ b/addons/isl-server/src/github/generated/graphql.ts @@ -29508,7 +29508,7 @@ export type YourPullRequestsQueryVariables = Exact<{ }>; -export type YourPullRequestsQueryData = { __typename?: 'Query', search: { __typename?: 'SearchResultItemConnection', nodes?: Array<{ __typename?: 'App' } | { __typename?: 'Discussion' } | { __typename?: 'Issue' } | { __typename?: 'MarketplaceListing' } | { __typename?: 'Organization' } | { __typename: 'PullRequest', number: number, title: string, body: string, state: PullRequestState, isDraft: boolean, url: string, reviewDecision?: PullRequestReviewDecision | null, comments: { __typename?: 'IssueCommentConnection', totalCount: number }, mergeQueueEntry?: { __typename?: 'MergeQueueEntry', estimatedTimeToMerge?: number | null } | null, baseRef?: { __typename?: 'Ref', name: string, target?: { __typename?: 'Blob', oid: string } | { __typename?: 'Commit', oid: string } | { __typename?: 'Tag', oid: string } | { __typename?: 'Tree', oid: string } | null } | null, headRef?: { __typename?: 'Ref', name: string, target?: { __typename?: 'Blob', oid: string } | { __typename?: 'Commit', oid: string } | { __typename?: 'Tag', oid: string } | { __typename?: 'Tree', oid: string } | null } | null, commits: { __typename?: 'PullRequestCommitConnection', nodes?: Array<{ __typename?: 'PullRequestCommit', commit: { __typename?: 'Commit', oid: string, statusCheckRollup?: { __typename?: 'StatusCheckRollup', state: StatusState } | null } } | null> | null } } | { __typename?: 'Repository' } | { __typename?: 'User' } | null> | null } }; +export type YourPullRequestsQueryData = { __typename?: 'Query', viewer: { __typename?: 'User', login: string }, search: { __typename?: 'SearchResultItemConnection', nodes?: Array<{ __typename?: 'App' } | { __typename?: 'Discussion' } | { __typename?: 'Issue' } | { __typename?: 'MarketplaceListing' } | { __typename?: 'Organization' } | { __typename: 'PullRequest', number: number, title: string, body: string, state: PullRequestState, isDraft: boolean, url: string, reviewDecision?: PullRequestReviewDecision | null, author?: { __typename?: 'Bot', login: string, avatarUrl: string } | { __typename?: 'EnterpriseUserAccount', login: string, avatarUrl: string } | { __typename?: 'Mannequin', login: string, avatarUrl: string } | { __typename?: 'Organization', login: string, avatarUrl: string } | { __typename?: 'User', login: string, avatarUrl: string } | null, comments: { __typename?: 'IssueCommentConnection', totalCount: number }, mergeQueueEntry?: { __typename?: 'MergeQueueEntry', estimatedTimeToMerge?: number | null } | null, baseRef?: { __typename?: 'Ref', name: string, target?: { __typename?: 'Blob', oid: string } | { __typename?: 'Commit', oid: string } | { __typename?: 'Tag', oid: string } | { __typename?: 'Tree', oid: string } | null } | null, headRef?: { __typename?: 'Ref', name: string, target?: { __typename?: 'Blob', oid: string } | { __typename?: 'Commit', oid: string } | { __typename?: 'Tag', oid: string } | { __typename?: 'Tree', oid: string } | null } | null, commits: { __typename?: 'PullRequestCommitConnection', nodes?: Array<{ __typename?: 'PullRequestCommit', commit: { __typename?: 'Commit', oid: string, statusCheckRollup?: { __typename?: 'StatusCheckRollup', state: StatusState } | null } } | null> | null } } | { __typename?: 'Repository' } | { __typename?: 'User' } | null> | null } }; export type YourPullRequestsWithoutMergeQueueQueryVariables = Exact<{ searchQuery: Scalars['String']; @@ -29516,7 +29516,7 @@ export type YourPullRequestsWithoutMergeQueueQueryVariables = Exact<{ }>; -export type YourPullRequestsWithoutMergeQueueQueryData = { __typename?: 'Query', search: { __typename?: 'SearchResultItemConnection', nodes?: Array<{ __typename?: 'App' } | { __typename?: 'Discussion' } | { __typename?: 'Issue' } | { __typename?: 'MarketplaceListing' } | { __typename?: 'Organization' } | { __typename: 'PullRequest', number: number, title: string, body: string, state: PullRequestState, isDraft: boolean, url: string, reviewDecision?: PullRequestReviewDecision | null, comments: { __typename?: 'IssueCommentConnection', totalCount: number }, baseRef?: { __typename?: 'Ref', name: string, target?: { __typename?: 'Blob', oid: string } | { __typename?: 'Commit', oid: string } | { __typename?: 'Tag', oid: string } | { __typename?: 'Tree', oid: string } | null } | null, headRef?: { __typename?: 'Ref', name: string, target?: { __typename?: 'Blob', oid: string } | { __typename?: 'Commit', oid: string } | { __typename?: 'Tag', oid: string } | { __typename?: 'Tree', oid: string } | null } | null, commits: { __typename?: 'PullRequestCommitConnection', nodes?: Array<{ __typename?: 'PullRequestCommit', commit: { __typename?: 'Commit', oid: string, statusCheckRollup?: { __typename?: 'StatusCheckRollup', state: StatusState } | null } } | null> | null } } | { __typename?: 'Repository' } | { __typename?: 'User' } | null> | null } }; +export type YourPullRequestsWithoutMergeQueueQueryData = { __typename?: 'Query', viewer: { __typename?: 'User', login: string }, search: { __typename?: 'SearchResultItemConnection', nodes?: Array<{ __typename?: 'App' } | { __typename?: 'Discussion' } | { __typename?: 'Issue' } | { __typename?: 'MarketplaceListing' } | { __typename?: 'Organization' } | { __typename: 'PullRequest', number: number, title: string, body: string, state: PullRequestState, isDraft: boolean, url: string, reviewDecision?: PullRequestReviewDecision | null, author?: { __typename?: 'Bot', login: string, avatarUrl: string } | { __typename?: 'EnterpriseUserAccount', login: string, avatarUrl: string } | { __typename?: 'Mannequin', login: string, avatarUrl: string } | { __typename?: 'Organization', login: string, avatarUrl: string } | { __typename?: 'User', login: string, avatarUrl: string } | null, comments: { __typename?: 'IssueCommentConnection', totalCount: number }, baseRef?: { __typename?: 'Ref', name: string, target?: { __typename?: 'Blob', oid: string } | { __typename?: 'Commit', oid: string } | { __typename?: 'Tag', oid: string } | { __typename?: 'Tree', oid: string } | null } | null, headRef?: { __typename?: 'Ref', name: string, target?: { __typename?: 'Blob', oid: string } | { __typename?: 'Commit', oid: string } | { __typename?: 'Tag', oid: string } | { __typename?: 'Tree', oid: string } | null } | null, commits: { __typename?: 'PullRequestCommitConnection', nodes?: Array<{ __typename?: 'PullRequestCommit', commit: { __typename?: 'Commit', oid: string, statusCheckRollup?: { __typename?: 'StatusCheckRollup', state: StatusState } | null } } | null> | null } } | { __typename?: 'Repository' } | { __typename?: 'User' } | null> | null } }; export const CommentParts = ` fragment CommentParts on Comment { @@ -29578,6 +29578,9 @@ export const PullRequestCommentsQuery = ` ${ReactionParts}`; export const YourPullRequestsQuery = ` query YourPullRequestsQuery($searchQuery: String!, $numToFetch: Int!) { + viewer { + login + } search(query: $searchQuery, type: ISSUE, first: $numToFetch) { nodes { ... on PullRequest { @@ -29587,6 +29590,10 @@ export const YourPullRequestsQuery = ` body state isDraft + author { + login + avatarUrl + } url reviewDecision comments { @@ -29624,6 +29631,9 @@ export const YourPullRequestsQuery = ` `; export const YourPullRequestsWithoutMergeQueueQuery = ` query YourPullRequestsWithoutMergeQueueQuery($searchQuery: String!, $numToFetch: Int!) { + viewer { + login + } search(query: $searchQuery, type: ISSUE, first: $numToFetch) { nodes { ... on PullRequest { @@ -29633,6 +29643,10 @@ export const YourPullRequestsWithoutMergeQueueQuery = ` body state isDraft + author { + login + avatarUrl + } url reviewDecision comments { diff --git a/addons/isl-server/src/github/githubCodeReviewProvider.ts b/addons/isl-server/src/github/githubCodeReviewProvider.ts index 0796650cbcc5b..7afd532ff05e6 100644 --- a/addons/isl-server/src/github/githubCodeReviewProvider.ts +++ b/addons/isl-server/src/github/githubCodeReviewProvider.ts @@ -41,6 +41,7 @@ import { YourPullRequestsQuery, YourPullRequestsWithoutMergeQueueQuery, } from './generated/graphql'; +import {parseStackInfo, type StackEntry} from './parseStackInfo'; import queryGraphQL from './queryGraphQL'; export type GitHubDiffSummary = { @@ -60,23 +61,35 @@ export type GitHubDiffSummary = { head: Hash; /** Name of the branch on GitHub, which should match the local bookmark */ branchName?: string; + /** Stack info parsed from PR body Sapling footer. Top-to-bottom order (first = top of stack). */ + stackInfo?: StackEntry[]; + /** Author login (GitHub username) */ + author?: string; + /** Author avatar URL */ + authorAvatarUrl?: string; }; const DEFAULT_GH_FETCH_TIMEOUT = 60_000; // 1 minute +export type DiffSummariesData = { + summaries: Map; + currentUser?: string; +}; + type GitHubCodeReviewSystem = CodeReviewSystem & {type: 'github'}; export class GitHubCodeReviewProvider implements CodeReviewProvider { constructor( private codeReviewSystem: GitHubCodeReviewSystem, private logger: Logger, ) {} - private diffSummaries = new TypedEventEmitter<'data', Map>(); + private diffSummaries = new TypedEventEmitter<'data', DiffSummariesData>(); private hasMergeQueueSupport: Promise | null = null; onChangeDiffSummaries( - callback: (result: Result>) => unknown, + callback: (result: Result>, currentUser?: string) => unknown, ): Disposable { - const handleData = (data: Map) => callback({value: data}); + const handleData = (data: DiffSummariesData) => + callback({value: data.summaries}, data.currentUser); const handleError = (error: Error) => callback({error}); this.diffSummaries.on('data', handleData); this.diffSummaries.on('error', handleError); @@ -111,11 +124,14 @@ export class GitHubCodeReviewProvider implements CodeReviewProvider { private fetchYourPullRequestsGraphQL( includeMergeQueue: boolean, ): Promise { + // Calculate date 30 days ago for the updated filter + const thirtyDaysAgo = new Date(); + thirtyDaysAgo.setDate(thirtyDaysAgo.getDate() - 30); + const dateFilter = thirtyDaysAgo.toISOString().split('T')[0]; + const variables = { - // TODO: somehow base this query on the list of DiffIds - // This is not very easy with github's graphql API, which doesn't allow more than 5 "OR"s in a search query. - // But if we used one-query-per-diff we would reach rate limiting too quickly. - searchQuery: `repo:${this.codeReviewSystem.owner}/${this.codeReviewSystem.repo} is:pr author:@me`, + // Fetch all open PRs in the repo updated in the last 30 days + searchQuery: `repo:${this.codeReviewSystem.owner}/${this.codeReviewSystem.repo} is:pr is:open updated:>=${dateFilter}`, numToFetch: 50, }; if (includeMergeQueue) { @@ -138,9 +154,10 @@ export class GitHubCodeReviewProvider implements CodeReviewProvider { this.logger.info('fetching github PR summaries'); const allSummaries = await this.fetchYourPullRequestsGraphQL(hasMergeQueueSupport); if (allSummaries?.search.nodes == null) { - this.diffSummaries.emit('data', new Map()); + this.diffSummaries.emit('data', {summaries: new Map()}); return; } + const currentUser = allSummaries.viewer?.login; const map = new Map(); for (const summary of allSummaries.search.nodes) { @@ -151,6 +168,9 @@ export class GitHubCodeReviewProvider implements CodeReviewProvider { this.logger.warn(`PR #${id} is missing base or head ref, skipping.`); continue; } + // Parse stack info from the PR body (Sapling footer format) + const stackInfo = parseStackInfo(summary.body) ?? undefined; + map.set(id, { type: 'github', title: summary.title, @@ -174,11 +194,14 @@ export class GitHubCodeReviewProvider implements CodeReviewProvider { base: summary.baseRef.target.oid, head: summary.headRef.target.oid, branchName: summary.headRef.name, + stackInfo, + author: summary.author?.login ?? undefined, + authorAvatarUrl: summary.author?.avatarUrl ?? undefined, }); } } this.logger.info(`fetched ${map.size} github PR summaries`); - this.diffSummaries.emit('data', map); + this.diffSummaries.emit('data', {summaries: map, currentUser}); } catch (error) { this.logger.info('error fetching github PR summaries: ', error); this.diffSummaries.emit('error', error as Error); diff --git a/addons/isl-server/src/github/parseStackInfo.ts b/addons/isl-server/src/github/parseStackInfo.ts new file mode 100644 index 0000000000000..f326cae91dde3 --- /dev/null +++ b/addons/isl-server/src/github/parseStackInfo.ts @@ -0,0 +1,115 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Represents a single entry in a PR stack. + */ +export type StackEntry = { + /** True if this is the current PR (marked with arrow in footer) */ + isCurrent: boolean; + /** The PR number */ + prNumber: number; +}; + +/** + * The Sapling footer marker that indicates the start of stack info. + */ +const SAPLING_FOOTER_MARKER = '[//]: # (BEGIN SAPLING FOOTER)'; + +/** + * Legacy marker for backward compatibility. + */ +const LEGACY_STACK_MARKER = 'Stack created with [Sapling]'; + +/** + * Regex to match stack entries in the PR body. + * Matches lines like: + * * #123 + * * #123 (2 commits) + * * __->__ #42 + * * __->__ #42 (3 commits) + */ +const STACK_ENTRY_REGEX = /^\* (__->__ )?#(\d+).*$/; + +/** + * Parse stack info from PR body. Matches the Sapling footer format: + * + * Stack ordering (top-to-bottom as it appears in the PR body): + * - First entry = top of stack (newest commits) + * - Last entry = closest to trunk (oldest commits) + * + * Example footer: + * ``` + * --- + * [//]: # (BEGIN SAPLING FOOTER) + * Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](...). + * * #125 + * * __->__ #124 ← current PR (marked with arrow) + * * #123 + * * #122 ← closest to trunk (bottom of stack) + * ``` + * + * @param body The PR body text + * @returns Array of stack entries in same order (top-to-bottom), or null if no stack info found + */ +export function parseStackInfo(body: string): StackEntry[] | null { + if (!body) { + return null; + } + + const lines = body.split(/\r?\n/); + let inStackList = false; + const stackEntries: StackEntry[] = []; + + for (const line of lines) { + if (lineHasStackListMarker(line)) { + inStackList = true; + continue; + } + + if (inStackList) { + const match = STACK_ENTRY_REGEX.exec(line); + if (match) { + const [, arrow, number] = match; + stackEntries.push({ + isCurrent: Boolean(arrow), + prNumber: parseInt(number, 10), + }); + } else if (stackEntries.length > 0) { + // We've reached the end of the list (non-matching line after entries) + break; + } + } + } + + return stackEntries.length > 0 ? stackEntries : null; +} + +/** + * Check if a line indicates the start of the stack list. + */ +function lineHasStackListMarker(line: string): boolean { + return line === SAPLING_FOOTER_MARKER || line.startsWith(LEGACY_STACK_MARKER); +} + +/** + * Get the index of the current PR in the stack. + * @param stackEntries The parsed stack entries + * @returns The index of the current PR, or -1 if not found + */ +export function getCurrentPrIndex(stackEntries: StackEntry[]): number { + return stackEntries.findIndex(entry => entry.isCurrent); +} + +/** + * Get all PR numbers in the stack. + * @param stackEntries The parsed stack entries + * @returns Array of PR numbers in stack order (top-to-bottom) + */ +export function getStackPrNumbers(stackEntries: StackEntry[]): number[] { + return stackEntries.map(entry => entry.prNumber); +} diff --git a/addons/isl-server/src/github/queries/YourPullRequestsQuery.graphql b/addons/isl-server/src/github/queries/YourPullRequestsQuery.graphql index 5044a12c9cfea..f71c8ab90ae07 100644 --- a/addons/isl-server/src/github/queries/YourPullRequestsQuery.graphql +++ b/addons/isl-server/src/github/queries/YourPullRequestsQuery.graphql @@ -1,4 +1,7 @@ query YourPullRequestsQuery($searchQuery: String!, $numToFetch: Int!) { + viewer { + login + } search(query: $searchQuery, type: ISSUE, first: $numToFetch) { nodes { ... on PullRequest { @@ -8,6 +11,10 @@ query YourPullRequestsQuery($searchQuery: String!, $numToFetch: Int!) { body state isDraft + author { + login + avatarUrl + } url reviewDecision comments { diff --git a/addons/isl-server/src/github/queries/YourPullRequestsWithoutMergeQueueQuery.graphql b/addons/isl-server/src/github/queries/YourPullRequestsWithoutMergeQueueQuery.graphql index 47cf783cb60e6..0f816cec37f11 100644 --- a/addons/isl-server/src/github/queries/YourPullRequestsWithoutMergeQueueQuery.graphql +++ b/addons/isl-server/src/github/queries/YourPullRequestsWithoutMergeQueueQuery.graphql @@ -1,4 +1,7 @@ query YourPullRequestsWithoutMergeQueueQuery($searchQuery: String!, $numToFetch: Int!) { + viewer { + login + } search(query: $searchQuery, type: ISSUE, first: $numToFetch) { nodes { ... on PullRequest { @@ -8,6 +11,10 @@ query YourPullRequestsWithoutMergeQueueQuery($searchQuery: String!, $numToFetch: body state isDraft + author { + login + avatarUrl + } url reviewDecision comments { diff --git a/addons/isl/src/App.tsx b/addons/isl/src/App.tsx index ca536a2aebf9c..797495ea25386 100644 --- a/addons/isl/src/App.tsx +++ b/addons/isl/src/App.tsx @@ -22,6 +22,7 @@ import {availableCwds, CwdSelections} from './CwdSelector'; import {Drawers} from './Drawers'; import {EmptyState} from './EmptyState'; import {useCommand} from './ISLShortcuts'; +import {PRDashboard} from './PRDashboard'; import {Internal} from './Internal'; import {TopBar} from './TopBar'; import {TopLevelAlerts} from './TopLevelAlert'; @@ -95,9 +96,22 @@ function ISLDrawers() { right: {...state.right, collapsed: !state.right.collapsed}, })); }); + useCommand('ToggleLeftSidebar', () => { + setDrawerState(state => ({ + ...state, + left: {...state.left, collapsed: !state.left.collapsed}, + })); + }); return ( + + PR Stacks + + } + left={} rightLabel={ <> diff --git a/addons/isl/src/Commit.tsx b/addons/isl/src/Commit.tsx index 07ec351359532..406f898ce4c1d 100644 --- a/addons/isl/src/Commit.tsx +++ b/addons/isl/src/Commit.tsx @@ -43,6 +43,7 @@ import {clipboardLinkHtml} from './clipboard'; import { branchingDiffInfos, codeReviewProvider, + currentGitHubUser, diffSummary, latestCommitMessageTitle, } from './codeReview/CodeReviewInfo'; @@ -131,6 +132,25 @@ const commitLabelForCommit = atomFamilyWeak((hash: string) => }), ); +/** + * Atom to check if a commit is from an external author (someone other than the current user). + * Returns true if the PR author differs from the current GitHub user. + */ +export const isExternalCommitByDiffId = atomFamilyWeak((diffId: string) => + atom(get => { + const currentUser = get(currentGitHubUser); + if (currentUser == null) { + return false; + } + const summary = get(diffSummary(diffId)); + if (summary?.value == null) { + return false; + } + const author = summary.value.type === 'github' ? summary.value.author : undefined; + return author != null && author !== currentUser; + }), +); + export const Commit = memo( ({ commit, diff --git a/addons/isl/src/ISLShortcuts.tsx b/addons/isl/src/ISLShortcuts.tsx index f685a1ebbf12f..3645030d4d542 100644 --- a/addons/isl/src/ISLShortcuts.tsx +++ b/addons/isl/src/ISLShortcuts.tsx @@ -21,6 +21,7 @@ const CMD = isMac ? Modifier.CMD : Modifier.CTRL; export const [ISLCommandContext, useCommand, dispatchCommand, allCommands] = makeCommandDispatcher({ OpenShortcutHelp: [Modifier.SHIFT, KeyCode.QuestionMark], ToggleSidebar: [CMD, KeyCode.Period], + ToggleLeftSidebar: [CMD, KeyCode.Comma], OpenUncommittedChangesComparisonView: [CMD, KeyCode.SingleQuote], OpenHeadChangesComparisonView: [[CMD, Modifier.SHIFT], KeyCode.SingleQuote], Escape: [Modifier.NONE, KeyCode.Escape], @@ -58,6 +59,7 @@ export const ISLShortcutLabels: Partial> = { Escape: t('Dismiss Tooltips and Popups'), OpenShortcutHelp: t('Open Shortcut Help'), ToggleSidebar: t('Toggle Commit Info Sidebar'), + ToggleLeftSidebar: t('Toggle PR Stacks Sidebar'), OpenUncommittedChangesComparisonView: t('Open Uncommitted Changes Comparison View'), OpenHeadChangesComparisonView: t('Open Head Changes Comparison View'), SelectAllCommits: t('Select All Commits'), diff --git a/addons/isl/src/PRDashboard.css b/addons/isl/src/PRDashboard.css new file mode 100644 index 0000000000000..f6fd480aac7cd --- /dev/null +++ b/addons/isl/src/PRDashboard.css @@ -0,0 +1,145 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +.pr-dashboard { + display: flex; + flex-direction: column; + height: 100%; + overflow: hidden; +} + +.pr-dashboard-header { + display: flex; + align-items: center; + justify-content: space-between; + padding: var(--pad); + border-bottom: 1px solid var(--subtle-hover-darken); + flex-shrink: 0; +} + +.pr-dashboard-title { + font-weight: bold; + font-size: 14px; +} + +.pr-dashboard-content { + flex: 1; + overflow-y: auto; + padding: var(--pad); +} + +.pr-dashboard-empty { + display: flex; + flex-direction: column; + align-items: center; + justify-content: center; + gap: var(--pad); + padding: calc(var(--pad) * 2); + color: var(--foreground-sub); + text-align: center; +} + +.pr-dashboard-empty .codicon { + font-size: 24px; + opacity: 0.5; +} + +/* Stack Card */ +.stack-card { + border: 1px solid var(--subtle-hover-darken); + border-radius: 4px; + margin-bottom: var(--pad); + background: var(--background); +} + +.stack-card-header { + display: flex; + align-items: center; + padding: var(--halfpad) var(--pad); + gap: var(--halfpad); + background: var(--subtle-hover-darken); + border-radius: 4px 4px 0 0; +} + +.stack-card-expand-button { + padding: 2px; + min-width: auto; +} + +.stack-card-title { + flex: 1; + font-weight: 500; + cursor: pointer; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.stack-card-pull-button { + gap: 4px; +} + +.stack-card-prs { + padding: var(--halfpad); +} + +/* PR Row */ +.pr-row { + display: flex; + align-items: center; + padding: var(--halfpad); + gap: var(--halfpad); + border-radius: 2px; +} + +.pr-row:hover { + background: var(--subtle-hover-darken); +} + +.pr-row-status { + width: 16px; + text-align: center; + flex-shrink: 0; +} + +.pr-row-number { + color: var(--foreground-sub); + text-decoration: none; + flex-shrink: 0; +} + +.pr-row-number:hover { + text-decoration: underline; +} + +.pr-row-title { + flex: 1; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +/* PR State Colors */ +.pr-state-open { + color: var(--signal-good); +} + +.pr-state-merged { + color: var(--scm-modified-foreground); +} + +.pr-state-closed { + color: var(--signal-bad); +} + +.pr-state-draft { + color: var(--foreground-sub); +} + +.pr-state-queued { + color: var(--signal-warning); +} diff --git a/addons/isl/src/PRDashboard.tsx b/addons/isl/src/PRDashboard.tsx new file mode 100644 index 0000000000000..c3de5ed0a75d5 --- /dev/null +++ b/addons/isl/src/PRDashboard.tsx @@ -0,0 +1,154 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import type {PRStack} from './codeReview/PRStacksAtom'; +import type {DiffSummary} from './types'; + +import {Button} from 'isl-components/Button'; +import {Icon} from 'isl-components/Icon'; +import {Tooltip} from 'isl-components/Tooltip'; +import {useAtomValue} from 'jotai'; +import {useState} from 'react'; +import serverAPI from './ClientToServerAPI'; +import {prStacksAtom} from './codeReview/PRStacksAtom'; +import {T} from './i18n'; +import {useRunOperation} from './operationsState'; +import {PullStackOperation} from './operations/PullStackOperation'; + +import './PRDashboard.css'; + +export function PRDashboard() { + const stacks = useAtomValue(prStacksAtom); + + const handleRefresh = () => { + serverAPI.postMessage({type: 'fetchDiffSummaries'}); + }; + + return ( +
+
+ + PR Stacks + + + + +
+
+ {stacks.length === 0 ? ( +
+ + + No pull requests found + +
+ ) : ( + stacks.map(stack => ) + )} +
+
+ ); +} + +function StackCard({stack}: {stack: PRStack}) { + const [isExpanded, setIsExpanded] = useState(true); + const runOperation = useRunOperation(); + + const handlePullStack = () => { + runOperation(new PullStackOperation(stack.topPrNumber, /* goto */ true)); + }; + + const toggleExpanded = () => { + setIsExpanded(prev => !prev); + }; + + const headerTitle = stack.isStack + ? `Stack (${stack.prs.length} PRs)` + : `PR #${stack.topPrNumber}`; + + return ( +
+
+ + + {headerTitle} + + + + +
+ {isExpanded && ( +
+ {stack.prs.map(pr => ( + + ))} +
+ )} +
+ ); +} + +function PRRow({pr}: {pr: DiffSummary}) { + const stateIcon = getPRStateIcon(pr.state); + const stateClass = getPRStateClass(pr.state); + + return ( +
+ {stateIcon} + e.stopPropagation()}> + #{pr.number} + + + {pr.title} + +
+ ); +} + +function getPRStateIcon(state: DiffSummary['state']): string { + switch (state) { + case 'MERGED': + return '✓'; + case 'CLOSED': + return '✕'; + case 'DRAFT': + return '○'; + case 'MERGE_QUEUED': + return '◐'; + case 'OPEN': + default: + return '●'; + } +} + +function getPRStateClass(state: DiffSummary['state']): string { + switch (state) { + case 'MERGED': + return 'pr-state-merged'; + case 'CLOSED': + return 'pr-state-closed'; + case 'DRAFT': + return 'pr-state-draft'; + case 'MERGE_QUEUED': + return 'pr-state-queued'; + case 'OPEN': + default: + return 'pr-state-open'; + } +} diff --git a/addons/isl/src/codeReview/CodeReviewInfo.ts b/addons/isl/src/codeReview/CodeReviewInfo.ts index 40e08d901e0b8..3cc3f341a425d 100644 --- a/addons/isl/src/codeReview/CodeReviewInfo.ts +++ b/addons/isl/src/codeReview/CodeReviewInfo.ts @@ -90,10 +90,20 @@ export const branchingDiffInfos = atomFamilyWeak((branchName: string) => export const allDiffSummaries = atom | null>>({value: null}); export const diffIdsByBranchName = atom>(new Map()); +/** + * Current GitHub user login (username) from the authenticated user. + * Used to determine if a PR/stack is "external" (authored by someone else). + */ +export const currentGitHubUser = atom(undefined); registerDisposable( allDiffSummaries, serverAPI.onMessageOfType('fetchedDiffSummaries', event => { + // Store the current GitHub user if provided + if (event.currentUser) { + writeAtom(currentGitHubUser, event.currentUser); + } + writeAtom(diffIdsByBranchName, existing => { if (event.summaries.error) { return existing; diff --git a/addons/isl/src/codeReview/PRStacksAtom.ts b/addons/isl/src/codeReview/PRStacksAtom.ts new file mode 100644 index 0000000000000..1171e50a62309 --- /dev/null +++ b/addons/isl/src/codeReview/PRStacksAtom.ts @@ -0,0 +1,170 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import type {DiffSummary} from '../types'; + +import {atom} from 'jotai'; +import {atomWithStorage} from 'jotai/utils'; +import {allDiffSummaries} from './CodeReviewInfo'; + +/** + * Stack labels stored in localStorage. + * Maps stack ID to custom label. + */ +export const stackLabelsAtom = atomWithStorage>( + 'isl.prStackLabels', + {}, +); + +/** + * Hidden stacks stored in localStorage. + * Set of stack IDs that are hidden. + */ +export const hiddenStacksAtom = atomWithStorage( + 'isl.hiddenPrStacks', + [], +); + +/** + * Represents a stack of PRs grouped together. + */ +export type PRStack = { + /** Unique identifier for this stack (usually the top PR number) */ + id: string; + /** The top PR number (first in the stack list) */ + topPrNumber: number; + /** All PR summaries in the stack, ordered top-to-bottom (first = top of stack) */ + prs: DiffSummary[]; + /** Whether this stack has multiple PRs or is just a single PR */ + isStack: boolean; + /** Main contributor (author) for this stack */ + mainAuthor?: string; + /** Avatar URL of the main contributor */ + mainAuthorAvatarUrl?: string; +}; + +/** + * Groups PRs into stacks based on their stackInfo. + * + * PRs with matching stackInfo are grouped together. PRs without stackInfo + * are treated as single-PR stacks. + * + * Stack ordering: + * - First entry in stackInfo = top of stack (newest commits) + * - Last entry in stackInfo = closest to trunk (oldest commits) + */ +export const prStacksAtom = atom(get => { + const allDiffs = get(allDiffSummaries); + + if (allDiffs.error || allDiffs.value == null) { + return []; + } + + const diffsMap = allDiffs.value; + const stacks: PRStack[] = []; + const processedPrNumbers = new Set(); + + // Process each PR and group by stack + for (const [diffId, summary] of diffsMap.entries()) { + if (processedPrNumbers.has(diffId)) { + continue; + } + + const stackInfo = getStackInfo(summary); + + if (stackInfo && stackInfo.length > 1) { + // This PR is part of a multi-PR stack + // Build the stack from the stackInfo + const stackPrs: DiffSummary[] = []; + let topPrNumber: number | null = null; + + for (const entry of stackInfo) { + const prDiffId = String(entry.prNumber); + const prSummary = diffsMap.get(prDiffId); + + if (prSummary) { + stackPrs.push(prSummary); + processedPrNumbers.add(prDiffId); + + if (topPrNumber === null) { + topPrNumber = entry.prNumber; + } + } + } + + if (stackPrs.length > 0 && topPrNumber !== null) { + // Get main author from the first PR (top of stack) + const firstPr = stackPrs[0]; + const mainAuthor = + firstPr.type === 'github' ? firstPr.author : undefined; + const mainAuthorAvatarUrl = + firstPr.type === 'github' ? firstPr.authorAvatarUrl : undefined; + + stacks.push({ + id: `stack-${topPrNumber}`, + topPrNumber, + prs: stackPrs, + isStack: stackPrs.length > 1, + mainAuthor, + mainAuthorAvatarUrl, + }); + } + } else { + // Single PR (no stack info or single-entry stack) + const prNumber = parseInt(diffId, 10); + processedPrNumbers.add(diffId); + + // Get author from the PR + const mainAuthor = summary.type === 'github' ? summary.author : undefined; + const mainAuthorAvatarUrl = + summary.type === 'github' ? summary.authorAvatarUrl : undefined; + + stacks.push({ + id: `single-${diffId}`, + topPrNumber: prNumber, + prs: [summary], + isStack: false, + mainAuthor, + mainAuthorAvatarUrl, + }); + } + } + + // Sort stacks by top PR number (descending - newest first) + stacks.sort((a, b) => b.topPrNumber - a.topPrNumber); + + return stacks; +}); + +/** + * Extract stackInfo from a DiffSummary. + * Only GitHub PRs have stackInfo. + */ +function getStackInfo( + summary: DiffSummary, +): Array<{isCurrent: boolean; prNumber: number}> | undefined { + if (summary.type === 'github' && 'stackInfo' in summary) { + return summary.stackInfo; + } + return undefined; +} + +/** + * Atom to get just the count of stacks. + */ +export const prStacksCountAtom = atom(get => { + const stacks = get(prStacksAtom); + return stacks.length; +}); + +/** + * Atom to get count of multi-PR stacks (excludes single PRs). + */ +export const multiPrStacksCountAtom = atom(get => { + const stacks = get(prStacksAtom); + return stacks.filter(s => s.isStack).length; +}); diff --git a/addons/isl/src/operations/PullStackOperation.ts b/addons/isl/src/operations/PullStackOperation.ts new file mode 100644 index 0000000000000..6c1d92e8a7f7c --- /dev/null +++ b/addons/isl/src/operations/PullStackOperation.ts @@ -0,0 +1,39 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {Operation} from './Operation'; + +/** + * Operation to pull an entire PR stack using `sl pr get`. + * + * This operation discovers and imports a full stack of PRs based on the + * stack information in the PR body. + */ +export class PullStackOperation extends Operation { + static opName = 'PullStack'; + + constructor( + private prNumber: number, + private goto: boolean = true, + ) { + super('PullStackOperation'); + } + + getArgs() { + const args = ['pr', 'get', String(this.prNumber)]; + if (this.goto) { + args.push('--goto'); + } + return args; + } + + getDescriptionForDisplay() { + return { + description: `Pull stack for PR #${this.prNumber}`, + }; + } +} diff --git a/addons/isl/src/types.ts b/addons/isl/src/types.ts index c7c26ad9a59f9..0115d4b4d172f 100644 --- a/addons/isl/src/types.ts +++ b/addons/isl/src/types.ts @@ -1120,7 +1120,7 @@ export type ServerToClientMessage = | {type: 'repoInfo'; info: RepoInfo; cwd?: string} | {type: 'repoError'; error: RepositoryError | undefined} | {type: 'fetchedAvatars'; avatars: Map; authors: Array} - | {type: 'fetchedDiffSummaries'; summaries: Result>} + | {type: 'fetchedDiffSummaries'; summaries: Result>; currentUser?: string} | {type: 'fetchedDiffComments'; diffId: DiffId; comments: Result>} | {type: 'fetchedLandInfo'; topOfStack: DiffId; landInfo: Result} | {type: 'confirmedLand'; result: Result} diff --git a/eden/contrib/reviewstack.dev/functions/_oauth/callback.ts b/eden/contrib/reviewstack.dev/functions/_oauth/callback.ts new file mode 100644 index 0000000000000..85174b63f7f99 --- /dev/null +++ b/eden/contrib/reviewstack.dev/functions/_oauth/callback.ts @@ -0,0 +1,37 @@ +interface GitHubTokenResponse { + access_token?: string; + error?: string; + error_description?: string; +} + +export const onRequestGet: PagesFunction<{ CLIENT_ID: string; CLIENT_SECRET: string }> = async (context) => { + const url = new URL(context.request.url); + const code = url.searchParams.get('code'); + + if (!code) { + return new Response('Missing code parameter', { status: 400 }); + } + + const tokenResponse = await fetch('https://github.com/login/oauth/access_token', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Accept': 'application/json', + }, + body: JSON.stringify({ + client_id: context.env.CLIENT_ID, + client_secret: context.env.CLIENT_SECRET, + code, + }), + }); + + const data: GitHubTokenResponse = await tokenResponse.json(); + + if (data.error || !data.access_token) { + const errorMsg = encodeURIComponent(data.error_description || data.error || 'Unknown error'); + return Response.redirect(`${url.origin}/?error=${errorMsg}`, 302); + } + + // Redirect back to app with token in hash (not exposed to server logs) + return Response.redirect(`${url.origin}/#token=${data.access_token}`, 302); +}; diff --git a/eden/contrib/reviewstack.dev/functions/_oauth/login.ts b/eden/contrib/reviewstack.dev/functions/_oauth/login.ts new file mode 100644 index 0000000000000..a857fae33fe3f --- /dev/null +++ b/eden/contrib/reviewstack.dev/functions/_oauth/login.ts @@ -0,0 +1,9 @@ +export const onRequestGet: PagesFunction<{ CLIENT_ID: string }> = async (context) => { + const redirectUri = new URL('/_oauth/callback', context.request.url).toString(); + const githubAuthUrl = new URL('https://github.com/login/oauth/authorize'); + githubAuthUrl.searchParams.set('client_id', context.env.CLIENT_ID); + githubAuthUrl.searchParams.set('redirect_uri', redirectUri); + githubAuthUrl.searchParams.set('scope', 'user repo'); + + return Response.redirect(githubAuthUrl.toString(), 302); +}; diff --git a/eden/contrib/reviewstack.dev/src/LazyLoginDialog.tsx b/eden/contrib/reviewstack.dev/src/LazyLoginDialog.tsx index 3b0e9eaa295d3..68db9258f983c 100644 --- a/eden/contrib/reviewstack.dev/src/LazyLoginDialog.tsx +++ b/eden/contrib/reviewstack.dev/src/LazyLoginDialog.tsx @@ -17,7 +17,9 @@ export default function LazyLoginDialog({ }) { const {hostname} = window.location; const LoginComponent = - hostname === 'reviewstack.netlify.app' || hostname === 'reviewstack.dev' + hostname === 'reviewstack.netlify.app' || + hostname === 'reviewstack.dev' || + hostname === 'reviews.qlax.dev' ? NetlifyLoginDialog : DefaultLoginDialog; diff --git a/eden/contrib/reviewstack.dev/src/NetlifyLoginDialog.tsx b/eden/contrib/reviewstack.dev/src/NetlifyLoginDialog.tsx index 102d2eae92102..529c87d10a82c 100644 --- a/eden/contrib/reviewstack.dev/src/NetlifyLoginDialog.tsx +++ b/eden/contrib/reviewstack.dev/src/NetlifyLoginDialog.tsx @@ -11,16 +11,10 @@ import type {CustomLoginDialogProps} from 'reviewstack/src/LoginDialog'; import Footer from './Footer'; import InlineCode from './InlineCode'; import {Box, Button, Heading, Text, TextInput} from '@primer/react'; -import Authenticator from 'netlify-auth-providers'; -import React, {useCallback, useState} from 'react'; +import React, {useCallback, useEffect, useState} from 'react'; import AppHeader from 'reviewstack/src/AppHeader'; import Link from 'reviewstack/src/Link'; -/** - * See https://docs.github.com/en/developers/apps/building-oauth-apps/scopes-for-oauth-apps - */ -const GITHUB_OAUTH_SCOPE = ['user', 'repo'].join(' '); - export default function NetlifyLoginDialog(props: CustomLoginDialogProps): React.ReactElement { return ( @@ -58,17 +52,32 @@ function EndUserInstructions(props: CustomLoginDialogProps): React.ReactElement const {setTokenAndHostname} = props; const [isButtonDisabled, setButtonDisabled] = useState(false); const [errorMessage, setErrorMessage] = useState(null); - const onClick = useCallback(async () => { - setButtonDisabled(true); - try { - const token = await fetchGitHubToken(); + + // Check for OAuth token in URL hash on mount (from OAuth callback) + useEffect(() => { + const hash = window.location.hash; + const tokenMatch = hash.match(/token=([^&]+)/); + if (tokenMatch) { + const token = tokenMatch[1]; + // Clear the hash from URL + window.history.replaceState(null, '', window.location.pathname + window.location.search); setTokenAndHostname(token, 'github.com'); - } catch (e) { - const message = e instanceof Error ? e.message : 'error fetching OAuth token'; - setErrorMessage(message); } - setButtonDisabled(false); - }, [setButtonDisabled, setErrorMessage, setTokenAndHostname]); + // Check for error in URL params (from failed OAuth) + const params = new URLSearchParams(window.location.search); + const error = params.get('error'); + if (error) { + setErrorMessage(error); + // Clear error from URL + window.history.replaceState(null, '', window.location.pathname); + } + }, [setTokenAndHostname]); + + const onClick = useCallback(() => { + setButtonDisabled(true); + // Redirect to OAuth login endpoint + window.location.href = '/_oauth/login'; + }, []); return ( @@ -222,23 +231,3 @@ function H3({children}: {children: React.ReactNode}): React.ReactElement { ); } -function fetchGitHubToken(): Promise { - return new Promise((resolve, reject) => { - const authenticator = new Authenticator({}); - authenticator.authenticate( - {provider: 'github', scope: GITHUB_OAUTH_SCOPE}, - (error: Error | null, data: {token: string} | null) => { - if (error) { - reject(error); - } else { - const token = data?.token; - if (typeof token === 'string') { - resolve(token); - } else { - reject(new Error('token missing in OAuth response')); - } - } - }, - ); - }); -} diff --git a/eden/contrib/reviewstack/src/PullRequest.tsx b/eden/contrib/reviewstack/src/PullRequest.tsx index 906cac6f341e2..fe2205f21546e 100644 --- a/eden/contrib/reviewstack/src/PullRequest.tsx +++ b/eden/contrib/reviewstack/src/PullRequest.tsx @@ -106,9 +106,11 @@ function PullRequestDetails() { const stack = pullRequestStack.contents; const { bodyHTML } = pullRequest; - let pullRequestBodyHTML; + let pullRequestBodyHTML: string; switch (stack.type) { case 'no-stack': + case 'graphite': + // Graphite stack info is in a comment, not the PR body, so no stripping needed pullRequestBodyHTML = bodyHTML; break; case 'sapling': diff --git a/eden/contrib/reviewstack/src/PullRequestChangeCount.tsx b/eden/contrib/reviewstack/src/PullRequestChangeCount.tsx index d7e93e8acf375..4510355573af8 100644 --- a/eden/contrib/reviewstack/src/PullRequestChangeCount.tsx +++ b/eden/contrib/reviewstack/src/PullRequestChangeCount.tsx @@ -5,23 +5,46 @@ * LICENSE file in the root directory of this source tree. */ -import {gitHubPullRequest} from './recoil'; -import {CounterLabel} from '@primer/react'; +import {gitHubPullRequest, pullRequestSLOC} from './recoil'; +import {CounterLabel, Tooltip} from '@primer/react'; import {useRecoilValue} from 'recoil'; export default function PullRequestChangeCount(): React.ReactElement | null { const pullRequest = useRecoilValue(gitHubPullRequest); + const slocInfo = useRecoilValue(pullRequestSLOC); if (pullRequest == null) { return null; } const {additions, deletions} = pullRequest; + const {significantLines, generatedFileCount} = slocInfo; + + const tooltipText = + generatedFileCount > 0 + ? `${significantLines} significant lines (excludes ${generatedFileCount} generated file${generatedFileCount === 1 ? '' : 's'})` + : `${significantLines} significant lines`; return ( <> - +{additions} - -{deletions} + +{additions} + + -{deletions} + + {significantLines > 0 && ( + + + {significantLines} sig + + + )} ); } diff --git a/eden/contrib/reviewstack/src/SplitDiffFileHeader.tsx b/eden/contrib/reviewstack/src/SplitDiffFileHeader.tsx index 7d1befb2ce2c1..8b16d12ed1ebd 100644 --- a/eden/contrib/reviewstack/src/SplitDiffFileHeader.tsx +++ b/eden/contrib/reviewstack/src/SplitDiffFileHeader.tsx @@ -6,16 +6,18 @@ */ import {ChevronDownIcon, ChevronRightIcon} from '@primer/octicons-react'; -import {Box, Text, Tooltip} from '@primer/react'; +import {Box, Label, Text, Tooltip} from '@primer/react'; export function FileHeader({ path, open, onChangeOpen, + isGenerated, }: { path: string; open?: boolean; onChangeOpen?: (open: boolean) => void; + isGenerated?: boolean; }) { // Even though the enclosing will have border-radius set, we // have to define it again here or things don't look right. @@ -79,6 +81,19 @@ export function FileHeader({ )} {filePathParts} + {isGenerated && ( + + + + )} ); } diff --git a/eden/contrib/reviewstack/src/SplitDiffView.tsx b/eden/contrib/reviewstack/src/SplitDiffView.tsx index d3df75f5b9351..c6c11f5ec84da 100644 --- a/eden/contrib/reviewstack/src/SplitDiffView.tsx +++ b/eden/contrib/reviewstack/src/SplitDiffView.tsx @@ -23,7 +23,9 @@ import SplitDiffRow from './SplitDiffRow'; import {diffAndTokenize, lineRange} from './diffServiceClient'; import {DiffSide} from './generated/graphql'; import {grammars, languages} from './generated/textmate/TextMateGrammarManifest'; +import {GeneratedStatus} from './github/types'; import { + fileGeneratedStatus, gitHubPullRequestLineToPositionForFile, gitHubPullRequestSelectedVersionIndex, gitHubPullRequestVersions, @@ -38,7 +40,7 @@ import {UnfoldIcon} from '@primer/octicons-react'; import {Box, Spinner, Text} from '@primer/react'; import {diffChars} from 'diff'; import React, {useCallback, useEffect, useState} from 'react'; -import {useRecoilValue, useRecoilValueLoadable, waitForAll} from 'recoil'; +import {constSelector, useRecoilValue, useRecoilValueLoadable, waitForAll} from 'recoil'; import organizeLinesIntoGroups from 'shared/SplitDiffView/organizeLinesIntoGroups'; import { applyTokenizationToLine, @@ -81,7 +83,6 @@ export default function SplitDiffView({ after, isPullRequest, }: Props): React.ReactElement { - const [open, setOpen] = useState(true); const scopeName = getFilepathClassifier().findScopeNameForPath(path); const colorMode = useRecoilValue(primerColorMode); const loadable = useRecoilValueLoadable( @@ -104,14 +105,32 @@ export default function SplitDiffView({ isPullRequest ? gitHubPullRequestVersions : nullAtom, isPullRequest ? gitHubPullRequestSelectedVersionIndex : nullAtom, isPullRequest ? gitHubPullRequestLineToPositionForFile(path) : nullAtom, + // Check if file is generated to default collapse state for generated files + isPullRequest ? fileGeneratedStatus(path) : constSelector(GeneratedStatus.Manual), ]), ); - const [{patch, tokenization}, allThreads, newCommentInputCallbacks, commitIDs] = - loadable.getValue(); + const [ + {patch, tokenization}, + allThreads, + newCommentInputCallbacks, + commitIDs, + _versions, + _selectedVersionIndex, + _lineToPosition, + generatedStatus, + ] = loadable.getValue(); + const isGenerated = generatedStatus === GeneratedStatus.Generated; + // Default to collapsed for generated files + const [open, setOpen] = useState(!isGenerated); return ( - setOpen(open)} /> + setOpen(open)} + isGenerated={isGenerated} + /> {open && ( (1500); + +/** + * localStorage key prefix for caching .gitattributes patterns per repo. + */ +const GITATTRIBUTES_CACHE_KEY_PREFIX = 'reviewstack:gitattributes:'; + +/** + * Detect if a file is generated by scanning its content for markers. + * Checks the first 1024 bytes for @generated or @partially-generated tags. + */ +export function detectGeneratedFromContent(content: string | null): GeneratedStatus { + if (content == null) { + return GeneratedStatus.Manual; + } + + // Only scan first 1024 bytes + const header = content.slice(0, 1024); + + if (header.includes('@generated')) { + return GeneratedStatus.Generated; + } + + if (header.includes('@partially-generated')) { + return GeneratedStatus.PartiallyGenerated; + } + + return GeneratedStatus.Manual; +} + +/** + * Detect if a file is generated based on its blob content. + * Results are cached by blob OID. + */ +export function detectGeneratedFromBlob(blob: Blob): GeneratedStatus { + // Check cache first + const cached = generatedStatusCache.get(blob.oid); + if (cached !== undefined) { + return cached; + } + + // Binary files are not considered generated (can't scan them) + if (blob.isBinary) { + generatedStatusCache.set(blob.oid, GeneratedStatus.Manual); + return GeneratedStatus.Manual; + } + + const status = detectGeneratedFromContent(blob.text); + generatedStatusCache.set(blob.oid, status); + return status; +} + +/** + * Check if a file path matches any of the default generated patterns. + */ +export function matchesDefaultGeneratedPattern(filePath: string): boolean { + return DEFAULT_GENERATED_PATTERNS.some(pattern => pattern.test(filePath)); +} + +/** + * Convert a gitattributes glob pattern to a RegExp. + * Supports: + * - ** matches any path segment(s) + * - * matches any characters except / + * - ? matches any single character + * - Leading / anchors to repo root + */ +export function gitAttributePatternToRegex(pattern: string): RegExp { + let regexStr = ''; + let i = 0; + + // Handle leading / (anchor to root) + const anchorToRoot = pattern.startsWith('/'); + if (anchorToRoot) { + regexStr += '^'; + i = 1; + } + + while (i < pattern.length) { + const char = pattern[i]; + + if (char === '*') { + if (pattern[i + 1] === '*') { + // ** matches any path including / + if (pattern[i + 2] === '/') { + // **/ at start or after / means zero or more directories + regexStr += '(?:.*/)?'; + i += 3; + } else { + // ** matches everything + regexStr += '.*'; + i += 2; + } + } else { + // * matches any characters except / + regexStr += '[^/]*'; + i += 1; + } + } else if (char === '?') { + // ? matches any single character except / + regexStr += '[^/]'; + i += 1; + } else if (char === '.') { + regexStr += '\\.'; + i += 1; + } else if (char === '/') { + regexStr += '/'; + i += 1; + } else { + // Escape other regex special characters + regexStr += char.replace(/[\\^$+{}[\]|()]/g, '\\$&'); + i += 1; + } + } + + // If pattern doesn't start with /, it can match at any depth + if (!anchorToRoot) { + regexStr = '(?:^|/)' + regexStr; + } + + // Anchor to end + regexStr += '$'; + + return new RegExp(regexStr); +} + +/** + * Parse .gitattributes file content and extract linguist-generated patterns. + * + * Format: each line is "pattern attribute=value" or "pattern attribute" + * We look for: linguist-generated, linguist-generated=true, -linguist-generated + */ +export function parseGitAttributes(content: string): GitAttributePattern[] { + const patterns: GitAttributePattern[] = []; + + for (const line of content.split('\n')) { + const trimmed = line.trim(); + + // Skip comments and empty lines + if (trimmed === '' || trimmed.startsWith('#')) { + continue; + } + + // Split into pattern and attributes + // Pattern can contain spaces if quoted, but we'll handle simple cases + const match = trimmed.match(/^(\S+)\s+(.+)$/); + if (!match) { + continue; + } + + const [, pattern, attributesStr] = match; + + // Check for linguist-generated attribute + // Formats: linguist-generated, linguist-generated=true, -linguist-generated + if (/\blinguist-generated\b/.test(attributesStr)) { + const isNegated = attributesStr.includes('-linguist-generated'); + const isFalse = /linguist-generated=false/.test(attributesStr); + + patterns.push({ + pattern, + isGenerated: !isNegated && !isFalse, + }); + } + } + + return patterns; +} + +/** + * Check if a file path matches any gitattributes pattern. + * Returns the matched pattern's isGenerated value, or null if no match. + */ +export function matchGitAttributePatterns( + filePath: string, + patterns: GitAttributePattern[], +): boolean | null { + // Process patterns in order - last match wins (gitattributes semantics) + let result: boolean | null = null; + + for (const {pattern, isGenerated} of patterns) { + const regex = gitAttributePatternToRegex(pattern); + if (regex.test(filePath)) { + result = isGenerated; + } + } + + return result; +} + +/** + * Get the generated status for a file, checking (in order): + * 1. .gitattributes patterns + * 2. File content (@generated tag) + * 3. Default patterns + */ +export function getGeneratedStatus( + filePath: string, + gitAttributePatterns: GitAttributePattern[], + blob: Blob | null, +): GeneratedStatus { + // 1. Check gitattributes patterns first + const gitAttrMatch = matchGitAttributePatterns(filePath, gitAttributePatterns); + if (gitAttrMatch === true) { + return GeneratedStatus.Generated; + } + if (gitAttrMatch === false) { + // Explicitly marked as not generated + return GeneratedStatus.Manual; + } + + // 2. Check blob content for @generated tag + if (blob != null) { + const blobStatus = detectGeneratedFromBlob(blob); + if (blobStatus !== GeneratedStatus.Manual) { + return blobStatus; + } + } + + // 3. Check default patterns + if (matchesDefaultGeneratedPattern(filePath)) { + return GeneratedStatus.Generated; + } + + return GeneratedStatus.Manual; +} + +/** + * Cache .gitattributes patterns in localStorage. + */ +export function cacheGitAttributePatterns( + owner: string, + repo: string, + commitOid: GitObjectID, + patterns: GitAttributePattern[], +): void { + const key = `${GITATTRIBUTES_CACHE_KEY_PREFIX}${owner}/${repo}:${commitOid}`; + try { + localStorage.setItem(key, JSON.stringify(patterns)); + } catch { + // localStorage might be full or unavailable + } +} + +/** + * Retrieve cached .gitattributes patterns from localStorage. + */ +export function getCachedGitAttributePatterns( + owner: string, + repo: string, + commitOid: GitObjectID, +): GitAttributePattern[] | null { + const key = `${GITATTRIBUTES_CACHE_KEY_PREFIX}${owner}/${repo}:${commitOid}`; + try { + const cached = localStorage.getItem(key); + if (cached != null) { + return JSON.parse(cached) as GitAttributePattern[]; + } + } catch { + // localStorage might be unavailable or corrupted + } + return null; +} + +/** + * Clear old .gitattributes cache entries to prevent localStorage bloat. + * Keeps only the most recent entries per repo. + */ +export function cleanupGitAttributesCache(maxEntriesPerRepo = 5): void { + try { + const entries: Array<{key: string; repo: string; time: number}> = []; + + for (let i = 0; i < localStorage.length; i++) { + const key = localStorage.key(i); + if (key?.startsWith(GITATTRIBUTES_CACHE_KEY_PREFIX)) { + const repoMatch = key.match(/^reviewstack:gitattributes:([^:]+):/); + if (repoMatch) { + entries.push({ + key, + repo: repoMatch[1], + time: i, // Use index as rough time ordering + }); + } + } + } + + // Group by repo and keep only most recent + const byRepo = new Map(); + for (const entry of entries) { + const repoEntries = byRepo.get(entry.repo) ?? []; + repoEntries.push(entry); + byRepo.set(entry.repo, repoEntries); + } + + for (const repoEntries of byRepo.values()) { + if (repoEntries.length > maxEntriesPerRepo) { + // Sort by time (most recent last) and remove old entries + repoEntries.sort((a, b) => a.time - b.time); + for (let i = 0; i < repoEntries.length - maxEntriesPerRepo; i++) { + localStorage.removeItem(repoEntries[i].key); + } + } + } + } catch { + // Ignore cleanup failures + } +} diff --git a/eden/contrib/reviewstack/src/github/types.ts b/eden/contrib/reviewstack/src/github/types.ts index d091931291618..b8457d1b05ffe 100644 --- a/eden/contrib/reviewstack/src/github/types.ts +++ b/eden/contrib/reviewstack/src/github/types.ts @@ -173,3 +173,42 @@ export type PaginationParams = last: number; before?: PageInfo['startCursor']; }; + +/** + * Status indicating whether a file is generated (machine-created) or manual (human-written). + * Used for calculating "significant lines of code" by excluding generated files. + */ +export enum GeneratedStatus { + /** File is manually written by humans */ + Manual = 0, + /** File contains both generated and manual sections (has @partially-generated tag) */ + PartiallyGenerated = 1, + /** File is fully generated (has @generated tag or matches linguist-generated pattern) */ + Generated = 2, +} + +/** + * A pattern from .gitattributes that marks files as generated. + * See: https://github.com/github/linguist/blob/master/docs/overrides.md + */ +export interface GitAttributePattern { + /** Glob pattern (e.g., "*.generated.ts", "libs/graphql/**") */ + pattern: string; + /** Whether files matching this pattern are generated */ + isGenerated: boolean; +} + +/** + * Significant Lines of Code information for a pull request. + * Excludes generated files from the count to show meaningful diff statistics. + */ +export interface SLOCInfo { + /** Lines changed in non-generated files (additions + deletions) */ + significantLines: number; + /** Total lines changed across all files */ + totalLines: number; + /** Number of files detected as generated */ + generatedFileCount: number; + /** Paths of files detected as generated */ + generatedFiles: string[]; +} diff --git a/eden/contrib/reviewstack/src/graphiteStack.test.ts b/eden/contrib/reviewstack/src/graphiteStack.test.ts new file mode 100644 index 0000000000000..f962ec0063fa6 --- /dev/null +++ b/eden/contrib/reviewstack/src/graphiteStack.test.ts @@ -0,0 +1,157 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {isGraphiteStackComment, parseGraphiteStackComment} from './graphiteStack'; + +const GRAPHITE_COMMENT = `\ +> [!WARNING] +> This pull request is not mergeable via GitHub because a downstack PR is open. + +* **#3818** 👈 +* **#3815** +* **#3814** +* \`main\` + +--- + +This stack of pull requests is managed by **Graphite**. +`; + +const GRAPHITE_COMMENT_MIDDLE_OF_STACK = `\ +* **#3818** +* **#3815** 👈 +* **#3814** +* \`main\` + +This stack of pull requests is managed by **Graphite**. +`; + +const GRAPHITE_COMMENT_BOTTOM_OF_STACK = `\ +* **#3818** +* **#3815** +* **#3814** 👈 +* \`main\` + +This stack of pull requests is managed by **Graphite**. +`; + +const GRAPHITE_COMMENT_SINGLE_PR = `\ +* **#100** 👈 +* \`develop\` + +This stack of pull requests is managed by **Graphite**. +`; + +const NON_GRAPHITE_COMMENT = `\ +This is a regular comment that doesn't contain any Graphite stack info. +Here are some PR references: #123 #456 +`; + +describe('isGraphiteStackComment', () => { + test('returns true for Graphite stack comment', () => { + expect(isGraphiteStackComment(GRAPHITE_COMMENT)).toBe(true); + }); + + test('returns false for non-Graphite comment', () => { + expect(isGraphiteStackComment(NON_GRAPHITE_COMMENT)).toBe(false); + }); + + test('returns false for empty string', () => { + expect(isGraphiteStackComment('')).toBe(false); + }); +}); + +describe('parseGraphiteStackComment', () => { + test('parses stack with current PR at top', () => { + const result = parseGraphiteStackComment(GRAPHITE_COMMENT); + expect(result).toEqual({ + stack: [3818, 3815, 3814], + currentStackEntry: 0, + baseBranch: 'main', + }); + }); + + test('parses stack with current PR in middle', () => { + const result = parseGraphiteStackComment(GRAPHITE_COMMENT_MIDDLE_OF_STACK); + expect(result).toEqual({ + stack: [3818, 3815, 3814], + currentStackEntry: 1, + baseBranch: 'main', + }); + }); + + test('parses stack with current PR at bottom', () => { + const result = parseGraphiteStackComment(GRAPHITE_COMMENT_BOTTOM_OF_STACK); + expect(result).toEqual({ + stack: [3818, 3815, 3814], + currentStackEntry: 2, + baseBranch: 'main', + }); + }); + + test('parses single PR stack', () => { + const result = parseGraphiteStackComment(GRAPHITE_COMMENT_SINGLE_PR); + expect(result).toEqual({ + stack: [100], + currentStackEntry: 0, + baseBranch: 'develop', + }); + }); + + test('returns null for non-Graphite comment', () => { + const result = parseGraphiteStackComment(NON_GRAPHITE_COMMENT); + expect(result).toBe(null); + }); + + test('returns null for empty string', () => { + const result = parseGraphiteStackComment(''); + expect(result).toBe(null); + }); + + test('returns null for Graphite comment without current PR marker', () => { + const commentWithoutMarker = `\ +* **#3818** +* **#3815** +* \`main\` + +This stack of pull requests is managed by **Graphite**. +`; + const result = parseGraphiteStackComment(commentWithoutMarker); + expect(result).toBe(null); + }); + + test('handles comment without base branch', () => { + const commentWithoutBase = `\ +* **#3818** 👈 +* **#3815** + +This stack of pull requests is managed by **Graphite**. +`; + const result = parseGraphiteStackComment(commentWithoutBase); + expect(result).toEqual({ + stack: [3818, 3815], + currentStackEntry: 0, + baseBranch: null, + }); + }); + + test('handles carriage return characters', () => { + const commentWithCR = `\ +* **#3818** 👈\r +* **#3815** \r +* \`main\`\r +\r +This stack of pull requests is managed by **Graphite**.\r +`; + const result = parseGraphiteStackComment(commentWithCR); + expect(result).toEqual({ + stack: [3818, 3815], + currentStackEntry: 0, + baseBranch: 'main', + }); + }); +}); diff --git a/eden/contrib/reviewstack/src/graphiteStack.ts b/eden/contrib/reviewstack/src/graphiteStack.ts new file mode 100644 index 0000000000000..23f7cf94dfcfb --- /dev/null +++ b/eden/contrib/reviewstack/src/graphiteStack.ts @@ -0,0 +1,103 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Graphite (https://graphite.dev) posts a comment on each PR in a stack + * with links to all PRs in the stack. This module provides utilities for + * parsing those comments. + * + * Example Graphite comment format: + * + * ``` + * * **#3818** 👈 + * * **#3815** + * * **#3814** + * * `main` + * + * ... + * This stack of pull requests is managed by **Graphite** + * ``` + */ + +export type GraphiteStackBody = { + /** PR numbers in stack order (top of stack first) */ + stack: number[]; + /** Index of the current PR in the stack (marked with 👈) */ + currentStackEntry: number; + /** Base branch name (e.g., "main") */ + baseBranch: string | null; +}; + +const GRAPHITE_SIGNATURE = 'This stack of pull requests is managed by'; + +/** + * Check if a comment body is a Graphite stack comment. + */ +export function isGraphiteStackComment(body: string): boolean { + return body.includes(GRAPHITE_SIGNATURE); +} + +/** + * Parse a Graphite stack comment and extract PR numbers and current position. + * Returns null if the comment is not a valid Graphite stack comment. + */ +export function parseGraphiteStackComment(body: string): GraphiteStackBody | null { + if (!isGraphiteStackComment(body)) { + return null; + } + + const stack: number[] = []; + let currentStackEntry = -1; + let baseBranch: string | null = null; + + // Match PR number lines: * **#123** ... + // The 👈 emoji marks the current PR + const prLineRegex = /^\* \*\*#(\d+)\*\*/gm; + const baseLineRegex = /^\* `([^`]+)`/gm; + + const lines = body.split('\n'); + + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + + // Check for PR number line + const prMatch = /^\* \*\*#(\d+)\*\*/.exec(line); + if (prMatch) { + const prNumber = parseInt(prMatch[1], 10); + stack.push(prNumber); + + // Check if this is the current PR (marked with 👈) + if (line.includes('👈')) { + currentStackEntry = stack.length - 1; + } + continue; + } + + // Check for base branch line + const baseMatch = /^\* `([^`]+)`/.exec(line); + if (baseMatch) { + baseBranch = baseMatch[1]; + continue; + } + } + + // Must have at least one PR and a current entry + if (stack.length === 0) { + return null; + } + + // If no 👈 marker found, we can't determine current position + if (currentStackEntry === -1) { + return null; + } + + return { + stack, + currentStackEntry, + baseBranch, + }; +} diff --git a/eden/contrib/reviewstack/src/queries/PullRequestTimelineItemFragment.graphql b/eden/contrib/reviewstack/src/queries/PullRequestTimelineItemFragment.graphql index f4b765c7cecf6..cb09a94f58344 100644 --- a/eden/contrib/reviewstack/src/queries/PullRequestTimelineItemFragment.graphql +++ b/eden/contrib/reviewstack/src/queries/PullRequestTimelineItemFragment.graphql @@ -50,6 +50,7 @@ fragment PullRequestTimelineItemFragment on PullRequestTimelineItems { author { ...ActorFragment } + body bodyHTML } ... on RenamedTitleEvent { diff --git a/eden/contrib/reviewstack/src/recoil.ts b/eden/contrib/reviewstack/src/recoil.ts index 96fb80e85e449..2bf89341964e1 100644 --- a/eden/contrib/reviewstack/src/recoil.ts +++ b/eden/contrib/reviewstack/src/recoil.ts @@ -29,8 +29,10 @@ import type { Commit, DateTime, ForcePushEvent, + GitAttributePattern, GitObjectID, ID, + SLOCInfo, Version, VersionCommit, } from './github/types'; @@ -39,6 +41,12 @@ import type {RecoilValueReadOnly} from 'recoil'; import {lineToPosition} from './diffServiceClient'; import {DiffSide, PullRequestReviewState, UserHomePageQuery} from './generated/graphql'; +import { + cacheGitAttributePatterns, + getCachedGitAttributePatterns, + getGeneratedStatus, + parseGitAttributes, +} from './generatedFiles'; import CachingGitHubClient, {openDatabase} from './github/CachingGitHubClient'; import GraphQLGitHubClient from './github/GraphQLGitHubClient'; import {diffCommits, diffCommitWithParent} from './github/diff'; @@ -50,6 +58,7 @@ import { gitHubUsername, } from './github/gitHubCredentials'; import queryGraphQL from './github/queryGraphQL'; +import {GeneratedStatus} from './github/types'; import {stackedPullRequest, stackedPullRequestFragments} from './stackState'; import {getPathForChange, getTreeEntriesForChange, groupBy, groupByDiffSide} from './utils'; import {atom, atomFamily, constSelector, selector, selectorFamily, waitForAll} from 'recoil'; @@ -1307,3 +1316,198 @@ function createClient( const client = new GraphQLGitHubClient(hostname, organization, repository, token); return new CachingGitHubClient(db, client, organization, repository); } + +// ============================================================================= +// SLOC (Significant Lines of Code) selectors +// ============================================================================= + +/** + * Fetches and parses the .gitattributes file from the repo root. + * Uses localStorage cache to avoid re-fetching for the same commit. + */ +export const gitAttributesPatterns = selector({ + key: 'gitAttributesPatterns', + get: async ({get}) => { + const orgAndRepo = get(gitHubOrgAndRepo); + const commits = get(gitHubPullRequestCommits); + const client = get(gitHubClient); + + if (orgAndRepo == null || commits.length === 0 || client == null) { + return []; + } + + const {org, repo} = orgAndRepo; + // Use the latest commit's OID + const latestCommit = commits[commits.length - 1]; + const commitOid = latestCommit.oid; + + // Check localStorage cache first + const cached = getCachedGitAttributePatterns(org, repo, commitOid); + if (cached != null) { + return cached; + } + + // Fetch .gitattributes from the repo root + try { + // Get the tree for the head commit to find .gitattributes + const commit = await client.getCommit(commitOid); + if (commit == null) { + return []; + } + + // Find .gitattributes in the root tree + const gitattributesEntry = commit.tree.entries.find( + entry => entry.name === '.gitattributes' && entry.type === 'blob', + ); + + if (gitattributesEntry == null) { + // No .gitattributes file - cache empty result + cacheGitAttributePatterns(org, repo, commitOid, []); + return []; + } + + // Fetch the blob content + const blob = await client.getBlob(gitattributesEntry.oid); + if (blob == null || blob.isBinary || blob.text == null) { + cacheGitAttributePatterns(org, repo, commitOid, []); + return []; + } + + // Parse the .gitattributes content + const patterns = parseGitAttributes(blob.text); + cacheGitAttributePatterns(org, repo, commitOid, patterns); + return patterns; + } catch { + // On error, return empty patterns + return []; + } + }, +}); + +/** + * Get the generated status for a specific file in the current PR diff. + * Checks .gitattributes patterns, file content, and default patterns. + */ +export const fileGeneratedStatus = selectorFamily({ + key: 'fileGeneratedStatus', + get: + (filePath: string) => + ({get}) => { + const gitAttrPatterns = get(gitAttributesPatterns); + const diff = get(gitHubPullRequestVersionDiff); + + if (diff == null) { + return GeneratedStatus.Manual; + } + + // Find the file in the diff to get its blob OID + const fileChange = diff.diff.find(change => getPathForChange(change) === filePath); + + // Get the blob for content scanning + let blob: Blob | null = null; + if (fileChange != null) { + const entries = getTreeEntriesForChange(fileChange); + // Prefer the "after" version, fall back to "before" for deletions + const blobOid = entries.after?.oid ?? entries.before?.oid; + if (blobOid != null) { + blob = get(gitHubBlob(blobOid)); + } + } + + return getGeneratedStatus(filePath, gitAttrPatterns, blob); + }, +}); + +/** + * Calculate SLOC (Significant Lines of Code) for the current PR. + * Excludes generated files from the count. + * + * Note: This selector intentionally calls getGeneratedStatus() directly instead + * of using the fileGeneratedStatus selectorFamily to avoid creating deep + * dependency chains that can cause performance issues with many files. + */ +export const pullRequestSLOC = selector({ + key: 'pullRequestSLOC', + get: ({get}) => { + const pullRequest = get(gitHubPullRequest); + const diff = get(gitHubPullRequestVersionDiff); + // Load gitAttributesPatterns once, not per-file via fileGeneratedStatus + const gitAttrPatterns = get(gitAttributesPatterns); + + if (pullRequest == null || diff == null) { + return { + significantLines: 0, + totalLines: 0, + generatedFileCount: 0, + generatedFiles: [], + }; + } + + // Use the PR-level additions/deletions as the total + // Note: This counts all lines, not just non-blank lines + const totalLines = pullRequest.additions + pullRequest.deletions; + + let generatedLines = 0; + const generatedFiles: string[] = []; + + for (const change of diff.diff) { + const filePath = getPathForChange(change); + const entries = getTreeEntriesForChange(change); + + // Get blob for generated status detection (prefer after, fall back to before) + const blobOid = entries.after?.oid ?? entries.before?.oid; + const blob = blobOid != null ? get(gitHubBlob(blobOid)) : null; + + // Check if file is generated using the function directly to avoid + // deep dependency chains from fileGeneratedStatus selectorFamily + const generatedStatus = getGeneratedStatus(filePath, gitAttrPatterns, blob); + if (generatedStatus === GeneratedStatus.Generated) { + generatedFiles.push(filePath); + + // Count diff lines (additions + deletions) for this generated file + // This must be compatible with how totalLines is calculated (PR additions + deletions) + let fileAdditions = 0; + let fileDeletions = 0; + + if (change.type === 'add' && entries.after != null) { + // New file: all non-blank lines are additions + const afterBlob = get(gitHubBlob(entries.after.oid)); + if (afterBlob?.text != null) { + fileAdditions = afterBlob.text.split('\n').filter(line => line.trim() !== '').length; + } + } else if (change.type === 'remove' && entries.before != null) { + // Deleted file: all non-blank lines are deletions + const beforeBlob = get(gitHubBlob(entries.before.oid)); + if (beforeBlob?.text != null) { + fileDeletions = beforeBlob.text.split('\n').filter(line => line.trim() !== '').length; + } + } else if (change.type === 'modify') { + // Modified file: approximate by comparing before/after line counts + const beforeBlob = entries.before != null ? get(gitHubBlob(entries.before.oid)) : null; + const afterBlob = entries.after != null ? get(gitHubBlob(entries.after.oid)) : null; + + const beforeLines = + beforeBlob?.text?.split('\n').filter(line => line.trim() !== '').length ?? 0; + const afterLines = + afterBlob?.text?.split('\n').filter(line => line.trim() !== '').length ?? 0; + + fileAdditions = Math.max(0, afterLines - beforeLines); + fileDeletions = Math.max(0, beforeLines - afterLines); + } + + generatedLines += fileAdditions + fileDeletions; + } + } + + // Significant lines = total - generated + // This is an approximation since we're using PR-level totals + const significantLines = Math.max(0, totalLines - generatedLines); + + return { + significantLines, + totalLines, + generatedFileCount: generatedFiles.length, + generatedFiles, + }; + }, +}); diff --git a/eden/contrib/reviewstack/src/stackState.ts b/eden/contrib/reviewstack/src/stackState.ts index f5163a06ee509..dac2bcd2b9f12 100644 --- a/eden/contrib/reviewstack/src/stackState.ts +++ b/eden/contrib/reviewstack/src/stackState.ts @@ -6,9 +6,11 @@ */ import type {StackPullRequestFragment} from './generated/graphql'; +import type {GraphiteStackBody} from './graphiteStack'; import type {SaplingPullRequestBody} from './saplingStack'; import {pullRequestNumbersFromBody} from './ghstackUtils'; +import {parseGraphiteStackComment} from './graphiteStack'; import {gitHubClient, gitHubPullRequest} from './recoil'; import {parseSaplingStackBody} from './saplingStack'; import {selector, waitForAll} from 'recoil'; @@ -22,6 +24,10 @@ type StackedPullRequest = type: 'ghstack'; stack: number[]; } + | { + type: 'graphite'; + body: GraphiteStackBody; + } | { type: 'no-stack'; }; @@ -31,6 +37,8 @@ export const stackedPullRequest = selector({ get: ({get}) => { const pullRequest = get(gitHubPullRequest); const body = pullRequest?.body; + + // Check PR body for Sapling or ghstack stack info if (body != null) { const saplingStack = parseSaplingStackBody(body); if (saplingStack != null) { @@ -43,6 +51,17 @@ export const stackedPullRequest = selector({ } } + // Check timeline comments for Graphite stack info + const timelineItems = pullRequest?.timelineItems?.nodes ?? []; + for (const item of timelineItems) { + if (item?.__typename === 'IssueComment' && item.body != null) { + const graphiteStack = parseGraphiteStackComment(item.body); + if (graphiteStack != null) { + return {type: 'graphite', body: graphiteStack}; + } + } + } + return {type: 'no-stack'}; }, }); @@ -60,6 +79,9 @@ const stackedPullRequestNumbers = selector({ case 'ghstack': { return stacked.stack; } + case 'graphite': { + return stacked.body.stack; + } } }, }); diff --git a/eden/scm/sapling/ext/github/__init__.py b/eden/scm/sapling/ext/github/__init__.py index 4fde178b404c4..9839bfec5126b 100644 --- a/eden/scm/sapling/ext/github/__init__.py +++ b/eden/scm/sapling/ext/github/__init__.py @@ -16,6 +16,7 @@ from . import ( follow, + get_stack, github_repo_util, import_pull_request, link, @@ -75,8 +76,8 @@ def pull_request_command(ui, repo, *args, **opts): subcmd = pull_request_command.subcommand( categories=[ ( - "Create or update pull requests, using `pull` to import a PR, if necessary", - ["submit", "pull"], + "Create or update pull requests, using `pull` or `get` to import PRs", + ["submit", "pull", "get"], ), ( "Manually manage associations with pull requests", @@ -108,7 +109,11 @@ def submit_cmd(ui, repo, *args, **opts): Pull request(s) will be created against ``default``. If ``default`` is a fork, they will be created against default's - upstream repository. + upstream repository by default. To create PRs on your fork instead, + set:: + + [github] + submit-to-upstream = false Returns 0 on success. """ @@ -139,6 +144,42 @@ def pull_cmd(ui, repo, *args, **opts): return import_pull_request.get_pr(ui, repo, *args, **opts) +@subcmd( + "get", + [ + ( + "g", + "goto", + False, + _("goto the target pull request after importing the stack"), + ), + ( + "", + "downstack", + False, + _("only fetch PRs from target towards trunk (skip upstack)"), + ), + ], + _("PULL_REQUEST"), +) +def get_cmd(ui, repo, *args, **opts): + """import an entire PR stack into your working copy + + The PULL_REQUEST can be specified as either a URL: + `https://github.com/facebook/sapling/pull/321` + or just the PR number within the GitHub repository identified by + `sl config paths.default`. + + Unlike `sl pr pull` which imports only a single PR, this command + discovers and imports the entire stack of PRs based on the stack + information in the PR body. + + Use --downstack to fetch only PRs from the target towards trunk, + skipping any upstack (descendant) PRs. + """ + return get_stack.get(ui, repo, *args, **opts) + + @subcmd( "link", [("r", "rev", "", _("revision to link"), _("REV"))], diff --git a/eden/scm/sapling/ext/github/get_stack.py b/eden/scm/sapling/ext/github/get_stack.py new file mode 100644 index 0000000000000..7f60df557a29a --- /dev/null +++ b/eden/scm/sapling/ext/github/get_stack.py @@ -0,0 +1,146 @@ +# 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. + +"""Implementation of `sl pr get` command to pull entire PR stacks from GitHub. + +This command is similar to `sl pr pull` but fetches an entire stack of PRs +instead of just a single PR. + +Usage: + sl pr get 123 # Pull stack for PR #123 + sl pr get https://github.com/.../pull/123 + sl pr get 123 --goto # Pull and checkout target + sl pr get 123 --downstack # Only fetch trunk→target (skip upstack) +""" + +import asyncio + +from sapling import error +from sapling.hg import updatetotally +from sapling.i18n import _ +from sapling.node import bin + +from .consts.stackheader import STACK_HEADER_PREFIX as GHSTACK_HEADER_PREFIX +from .pull_request_arg import parse_pull_request_arg +from .pullrequest import PullRequestId +from .pullrequeststore import PullRequestStore +from .stack_discovery import discover_stack_from_pr, get_head_nodes + + +def get(ui, repo, *args, **opts): + """Pull an entire PR stack from GitHub. + + The PULL_REQUEST can be specified as either a URL: + `https://github.com/facebook/sapling/pull/321` + or just the PR number within the GitHub repository identified by + `sl config paths.default`. + + This command differs from `sl pr pull` in that it fetches the entire + stack of PRs, not just a single PR. + """ + if len(args) == 0: + raise error.Abort( + _("PR URL or number must be specified. See '@prog@ pr get -h'.") + ) + + pr_arg = args[0] + pr_id = parse_pull_request_arg(pr_arg, repo=repo) + if pr_id is None: + raise error.Abort( + _("Could not parse pull request arg: '%s'. Specify PR by URL or number.") + % pr_arg + ) + + is_goto = opts.get("goto") + downstack_only = opts.get("downstack") + + return asyncio.run( + _get_stack(ui, repo, pr_id=pr_id, is_goto=is_goto, downstack_only=downstack_only) + ) + + +async def _get_stack( + ui, + repo, + pr_id: PullRequestId, + is_goto: bool, + downstack_only: bool, +) -> int: + """Internal implementation of stack fetch. + + Returns 0 on success. + """ + ui.status(_("discovering stack for PR #%d...\n") % pr_id.number) + + # Check for ghstack PRs which need different handling + from .gh_submit import get_pull_request_details + + initial_result = await get_pull_request_details(pr_id) + if initial_result.is_err(): + raise error.Abort( + _("could not get pull request details: %s") % initial_result.unwrap_err() + ) + + initial_details = initial_result.unwrap() + if _looks_like_ghstack_pull_request(initial_details.body): + raise error.Abort( + _( + "This pull request appears to be part of a ghstack stack.\n" + "Try running the following instead:\n" + " sl ghstack checkout %s" + ) + % pr_id.as_url() + ) + + # Discover the full stack + try: + stack_result = await discover_stack_from_pr(pr_id, downstack_only=downstack_only) + except RuntimeError as e: + raise error.Abort(_("stack discovery failed: %s") % str(e)) + + num_prs = len(stack_result.entries) + if num_prs == 1: + ui.status(_("found single PR (no stack)\n")) + else: + mode = "downstack" if downstack_only else "full stack" + ui.status(_("found %d PRs in %s\n") % (num_prs, mode)) + + # Get all head nodes for pulling + head_nodes = get_head_nodes(stack_result) + + # Pull all commits + ui.status(_("pulling %d commit(s)...\n") % len(head_nodes)) + repo.pull(headnodes=tuple(head_nodes)) + + # Link all commits to their respective PRs + store = PullRequestStore(repo) + target_node = None + + for entry in stack_result.entries: + node = bin(entry.head_oid) + store.map_commit_to_pull_request(node, entry.pr_id) + + if entry.is_target: + target_node = node + + ui.status(_("imported #%d as %s\n") % (entry.number, entry.head_oid[:12])) + + ui.status(_("successfully imported %d PR(s)\n") % num_prs) + + # Goto target if requested + if is_goto and target_node is not None: + target_entry = stack_result.entries[stack_result.target_index] + updatetotally(ui, repo, target_node, None) + ui.status(_("now at #%d\n") % target_entry.number) + + return 0 + + +def _looks_like_ghstack_pull_request(body: str) -> bool: + """Check if a PR body indicates it was created by ghstack.""" + for line in body.splitlines(): + if line.startswith(GHSTACK_HEADER_PREFIX): + return True + return False diff --git a/eden/scm/sapling/ext/github/gh_submit.py b/eden/scm/sapling/ext/github/gh_submit.py index 6004ef4959eec..9bc32e07ba556 100644 --- a/eden/scm/sapling/ext/github/gh_submit.py +++ b/eden/scm/sapling/ext/github/gh_submit.py @@ -45,16 +45,21 @@ class Repository: # then we only traverse one link in the chain, so this could still be None. upstream: Optional["Repository"] = None - def get_base_branch(self) -> str: - """If this is a fork, returns the default_branch of the upstream repo.""" - if self.upstream: + def get_base_branch(self, use_upstream: bool = True) -> str: + """If this is a fork and use_upstream is True, returns the default_branch + of the upstream repo; otherwise returns this repo's default_branch.""" + if use_upstream and self.upstream: return self.upstream.default_branch else: return self.default_branch - def get_upstream_owner_and_name(self) -> Tuple[str, str]: - """owner and name to use when creating a pull request""" - if self.upstream: + def get_upstream_owner_and_name(self, use_upstream: bool = True) -> Tuple[str, str]: + """owner and name to use when creating a pull request. + + If use_upstream is True and this is a fork, returns the upstream's + owner and name. Otherwise returns this repo's owner and name. + """ + if use_upstream and self.upstream: return (self.upstream.owner, self.upstream.name) else: return (self.owner, self.name) diff --git a/eden/scm/sapling/ext/github/stack_discovery.py b/eden/scm/sapling/ext/github/stack_discovery.py new file mode 100644 index 0000000000000..18d1a28e6f2ea --- /dev/null +++ b/eden/scm/sapling/ext/github/stack_discovery.py @@ -0,0 +1,175 @@ +# 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. + +"""Stack discovery logic for `sl pr get` command. + +This module discovers the full stack of PRs from a given PR's body by parsing +the Sapling footer format. + +Stack ordering: +- The stack list in PR bodies is **top-to-bottom** +- First entry = top of stack (newest commits) +- Last entry = closest to trunk (oldest commits) +""" + +import asyncio +from dataclasses import dataclass +from typing import List, Optional + +from .gh_submit import get_pull_request_details, PullRequestDetails +from .pull_request_body import parse_stack_information +from .pullrequest import PullRequestId + + +@dataclass +class StackEntry: + """Represents a single PR in the stack.""" + + pr_id: PullRequestId + """The pull request identifier.""" + + number: int + """The PR number.""" + + head_oid: str + """The head commit OID for this PR.""" + + is_target: bool + """True if this is the target PR that the user requested.""" + + details: PullRequestDetails + """The full PR details from GitHub.""" + + +@dataclass +class StackDiscoveryResult: + """Result of stack discovery.""" + + entries: List[StackEntry] + """Ordered list of stack entries (top-to-bottom, first = top of stack).""" + + target_index: int + """Index of the target PR in the entries list.""" + + +async def discover_stack_from_pr( + pr_id: PullRequestId, + downstack_only: bool = False, +) -> StackDiscoveryResult: + """Discover the full stack from a PR's body. + + Args: + pr_id: The target PR to start discovery from. + downstack_only: If True, only fetch PRs from target towards trunk + (target index to end of list). + + Returns: + StackDiscoveryResult with ordered entries and target index. + + The stack is ordered top-to-bottom as it appears in the PR body: + - entries[0] is the top of the stack (newest commits) + - entries[-1] is closest to trunk (oldest commits) + """ + # First, get the target PR details + result = await get_pull_request_details(pr_id) + if result.is_err(): + raise RuntimeError(f"could not get pull request details: {result.unwrap_err()}") + + target_details = result.unwrap() + body = target_details.body + + # Parse stack information from the PR body + stack_entries = parse_stack_information(body) + + if not stack_entries: + # No stack info in body - treat as single-PR stack + entry = StackEntry( + pr_id=pr_id, + number=pr_id.number, + head_oid=target_details.head_oid, + is_target=True, + details=target_details, + ) + return StackDiscoveryResult(entries=[entry], target_index=0) + + # Find the target PR in the stack list + # Stack entries are List[Tuple[is_current: bool, number: int]] + target_index = None + for i, (is_current, number) in enumerate(stack_entries): + if is_current or number == pr_id.number: + target_index = i + break + + if target_index is None: + # Target not found in stack - maybe stack info is stale + # Fall back to single-PR behavior + entry = StackEntry( + pr_id=pr_id, + number=pr_id.number, + head_oid=target_details.head_oid, + is_target=True, + details=target_details, + ) + return StackDiscoveryResult(entries=[entry], target_index=0) + + # Determine which PRs to fetch based on downstack_only flag + if downstack_only: + # Fetch from target index to end (towards trunk) + entries_to_fetch = stack_entries[target_index:] + adjusted_target_index = 0 + else: + # Fetch all entries + entries_to_fetch = stack_entries + adjusted_target_index = target_index + + # Fetch all PR details in parallel + async def fetch_entry(index: int, is_current: bool, number: int) -> StackEntry: + entry_pr_id = PullRequestId( + hostname=pr_id.hostname, + owner=pr_id.owner, + name=pr_id.name, + number=number, + ) + + # Use cached details for target PR + if number == pr_id.number: + details = target_details + else: + result = await get_pull_request_details(entry_pr_id) + if result.is_err(): + raise RuntimeError( + f"could not get details for PR #{number}: {result.unwrap_err()}" + ) + details = result.unwrap() + + is_target = index == adjusted_target_index if downstack_only else is_current or number == pr_id.number + + return StackEntry( + pr_id=entry_pr_id, + number=number, + head_oid=details.head_oid, + is_target=is_target, + details=details, + ) + + entries = await asyncio.gather( + *[ + fetch_entry(i, is_current, number) + for i, (is_current, number) in enumerate(entries_to_fetch) + ] + ) + + return StackDiscoveryResult(entries=list(entries), target_index=adjusted_target_index) + + +def get_head_nodes(result: StackDiscoveryResult) -> List[bytes]: + """Get the head commit nodes for all entries in the stack. + + Returns: + List of binary node IDs for all stack entry head commits. + """ + from sapling.node import bin + + return [bin(entry.head_oid) for entry in result.entries] diff --git a/eden/scm/sapling/ext/github/submit.py b/eden/scm/sapling/ext/github/submit.py index a9775c91d3c3e..c0d905971b255 100644 --- a/eden/scm/sapling/ext/github/submit.py +++ b/eden/scm/sapling/ext/github/submit.py @@ -32,8 +32,11 @@ def submit(ui, repo, *args, **opts) -> int: """Create or update GitHub pull requests.""" github_repo = check_github_repo(repo) is_draft = opts.get("draft") + submit_to_upstream = ui.configbool("github", "submit-to-upstream", True) return asyncio.run( - update_commits_in_stack(ui, repo, github_repo, is_draft=is_draft) + update_commits_in_stack( + ui, repo, github_repo, is_draft=is_draft, submit_to_upstream=submit_to_upstream + ) ) @@ -147,7 +150,7 @@ async def get_partitions(ui, repo, store, filter) -> List[List[CommitData]]: async def update_commits_in_stack( - ui, repo, github_repo: GitHubRepo, is_draft: bool + ui, repo, github_repo: GitHubRepo, is_draft: bool, submit_to_upstream: bool = True ) -> int: parents = repo.dirstate.parents() if parents[0] == nullid: @@ -166,11 +169,11 @@ async def update_commits_in_stack( use_placeholder_strategy = ui.configbool("github", "placeholder-strategy") if use_placeholder_strategy: params = await create_placeholder_strategy_params( - ui, partitions, github_repo, origin + ui, partitions, github_repo, origin, submit_to_upstream ) else: params = await create_serial_strategy_params( - ui, partitions, github_repo, origin + ui, partitions, github_repo, origin, submit_to_upstream ) max_pull_requests_to_create = ui.configint("github", "max-prs-to-create", "5") @@ -223,6 +226,7 @@ def get_gitdir() -> str: store, ui, is_draft, + submit_to_upstream, ) else: assert isinstance(params, SerialStrategyParams) @@ -233,6 +237,7 @@ def get_gitdir() -> str: store, ui, is_draft, + submit_to_upstream, ) # Now that each pull request has a named branch pushed to GitHub, we can @@ -248,7 +253,8 @@ def get_gitdir() -> str: 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 + partitions, index, workflow, pr_numbers_and_num_commits, repository, ui, + submit_to_upstream ) for index in range(len(partitions)) ] + [ @@ -271,12 +277,13 @@ async def rewrite_pull_request_body( pr_numbers_and_num_commits: List[Tuple[int, int]], repository: Repository, ui, + submit_to_upstream: bool = True, ) -> None: # If available, use the head branch of the previous partition as the base # of this branch. Recall that partitions is ordered from the top of the # stack to the bottom. partition = partitions[index] - base = repository.get_base_branch() + base = repository.get_base_branch(submit_to_upstream) if workflow == SubmitWorkflow.SINGLE and index < len(partitions) - 1: base = none_throws(partitions[index + 1][0].head_branch_name) @@ -363,6 +370,7 @@ async def create_serial_strategy_params( partitions: List[List[CommitData]], github_repo: GitHubRepo, origin: str, + submit_to_upstream: bool = True, ) -> SerialStrategyParams: # git push --force any heads that need updating, creating new branch names, # if necessary. @@ -393,7 +401,9 @@ async def create_serial_strategy_params( repository = await get_repository_for_origin( origin, github_repo.hostname ) - upstream_owner, upstream_name = repository.get_upstream_owner_and_name() + upstream_owner, upstream_name = repository.get_upstream_owner_and_name( + submit_to_upstream + ) result = await gh_submit.guess_next_pull_request_number( github_repo.hostname, upstream_owner, upstream_name ) @@ -443,14 +453,15 @@ async def create_pull_requests_serially( store: PullRequestStore, ui, is_draft: bool, + submit_to_upstream: bool = True, ) -> None: """Creates a new pull request for each entry in the `commits` list. Each CommitData in `commits` will be updated such that its `.pr` field is set appropriately. """ - head_ref_prefix = f"{repository.owner}:" if repository.is_fork else "" - owner, name = repository.get_upstream_owner_and_name() + head_ref_prefix = f"{repository.owner}:" if repository.is_fork and submit_to_upstream else "" + owner, name = repository.get_upstream_owner_and_name(submit_to_upstream) hostname = repository.hostname # Create the pull requests in order serially to give us the best chance of @@ -458,7 +469,7 @@ async def create_pull_requests_serially( commits_to_update = [] parent = None for commit, branch_name in commits: - base = repository.get_base_branch() + base = repository.get_base_branch(submit_to_upstream) if workflow == SubmitWorkflow.SINGLE and parent: base = none_throws(parent.head_branch_name) @@ -521,6 +532,7 @@ async def create_placeholder_strategy_params( partitions: List[List[CommitData]], github_repo: GitHubRepo, origin: str, + submit_to_upstream: bool = True, ) -> PlaceholderStrategyParams: refs_to_update: List[str] = [] commits_that_need_pull_requests: List[CommitNeedsPullRequest] = [] @@ -559,7 +571,7 @@ async def create_placeholder_strategy_params( if commits_that_need_pull_requests: repository = await get_repository_for_origin(origin, github_repo.hostname) issue_numbers = await _create_placeholder_issues( - repository, len(commits_that_need_pull_requests) + repository, len(commits_that_need_pull_requests), submit_to_upstream ) for commit_needs_pr, number in zip( commits_that_need_pull_requests, issue_numbers @@ -590,15 +602,16 @@ async def create_pull_requests_from_placeholder_issues( store: PullRequestStore, ui, is_draft: bool, + submit_to_upstream: bool = True, ) -> None: """Creates a new pull request for each entry in the `commits` list. Each entry in `commits` is a (CommitData, branch_name, issue_number). Each CommitData will be updated such that its `.pr` field is set appropriately. """ - head_ref_prefix = f"{repository.owner}:" if repository.is_fork else "" - owner, name = repository.get_upstream_owner_and_name() - base_branch_for_repo = repository.get_base_branch() + head_ref_prefix = f"{repository.owner}:" if repository.is_fork and submit_to_upstream else "" + owner, name = repository.get_upstream_owner_and_name(submit_to_upstream) + base_branch_for_repo = repository.get_base_branch(submit_to_upstream) hostname = repository.hostname async def create_pull_request(params: PullRequestParams): @@ -659,9 +672,13 @@ async def create_pull_request(params: PullRequestParams): await asyncio.gather(*[create_pull_request(c) for c in commits]) -async def _create_placeholder_issues(repository: Repository, num: int) -> List[int]: +async def _create_placeholder_issues( + repository: Repository, num: int, submit_to_upstream: bool = True +) -> List[int]: """create the specified number of placeholder issues in parallel""" - upstream_owner, upstream_name = repository.get_upstream_owner_and_name() + upstream_owner, upstream_name = repository.get_upstream_owner_and_name( + submit_to_upstream + ) issue_number_results = await asyncio.gather( *[ gh_submit.create_pull_request_placeholder_issue( diff --git a/eden/scm/tests/test-ext-github-pr-get.t b/eden/scm/tests/test-ext-github-pr-get.t new file mode 100644 index 0000000000000..9a9a3ab574e00 --- /dev/null +++ b/eden/scm/tests/test-ext-github-pr-get.t @@ -0,0 +1,56 @@ +#require no-eden + +#inprocess-hg-incompatible + + $ eagerepo + $ enable github + +Test help output for `sl pr get` + + $ hg help pr get + hg pr get PULL_REQUEST + + import an entire PR stack into your working copy + + The PULL_REQUEST can be specified as either a URL: + 'https://github.com/facebook/sapling/pull/321' or just the PR number + within the GitHub repository identified by 'sl config paths.default'. + + Unlike 'sl pr pull' which imports only a single PR, this command discovers + and imports the entire stack of PRs based on the stack information in the + PR body. + + Use --downstack to fetch only PRs from the target towards trunk, skipping + any upstack (descendant) PRs. + + Options: + + -g --goto goto the target pull request after importing the stack + --downstack only fetch PRs from target towards trunk (skip upstack) + + (some details hidden, use --verbose to show complete help) + +Test error without argument + + $ hg init repo + $ cd repo + $ echo a > a1 + $ hg ci -Am addfile + adding a1 + + $ hg pr get + abort: PR URL or number must be specified. See 'hg pr get -h'. + [255] + +Test error for non-github repo + + $ hg pr get 123 + abort: This does not appear to be a GitHub repo + [255] + +Test pr get appears in pr subcommand list + + $ hg help pr | grep "get" + get import an entire PR stack into your working copy + + $ cd ..