Skip to content

Checklist For Merging A Pull Request

Jump to bottom
Casey Carter edited this page Aug 27, 2024 · 31 revisions

Before Moving To "Ready To Merge"

  • If the PR is an update of a feature, and the feature in question has benchmarks, make sure to run those benchmarks against the existing implementation.
    • If the feature is performance critical, you may ask the contributor to also add benchmarks; alternatively, add benchmarks yourself.
    • See Benchmarks for how to run benchmarks.

When Merging

  • Merge GitHub and MSVC PRs at the same time, after both of their tests have passed.
    • When mirroring multiple GitHub PRs, the MSVC PR should be a semi-linear merge. Any NON-GitHub changes should be separate commits in the MSVC PR.
    • When updating stl/debugger/STL.natvis, simultaneously update VS's src/vc/CppEE/src/Visualizers/STL.natvis.
  • Verify that the PR is linked to the issues that it resolves.
    • When linking issues using keywords, remember that the Words Of Power are important and they must be repeated when closing multiple issues.
    • Additionally, tag closed issues as fixed. Leave the other tags unchanged. We use fixed to mark issues that were solved via commits, as opposed to being resolved without a commit.
    • If any DevCom/VSO bugs were linked, resolve them appropriately. Reply to customers on DevCom, and set the Release/Milestone/Target fields in VSO.
  • Proofread the commit message.
    • The title should be clear, i.e. suitable for scanning in git log, gitk, etc.
      • Note that if the PR contains a single commit, GitHub will initially use that commit's title instead of the PR's title.
    • The body can be empty (i.e. the PR number records what happened and why), or it can contain a small or large amount of additional information. However, it shouldn't be a messy concatenation of commit messages during development (e.g. "fixed typo", "code review feedback", etc.).
    • Double-check all numbers (e.g. paper numbers and issue numbers) to catch common mistakes like reversing digits.
  • Thank the contributor, especially if they're a first-time contributor! 😸
  • Update the wiki's Changelog.
    • If a primary feature was implemented simultaneously with patch papers and/or LWG issue resolutions, list them as sub-bullets. The patch papers and LWG issues should have been mentioned in the GitHub issue for the primary feature; you can copy and modify the Markdown there.
  • If the new feature is unsupported in /permissive mode, i.e., it uses one of the strict_concepts_meow.lst test matrices, add an entry to the pertinent "Library" table in the Microsoft-internal C++23 (and later) issues under permissive wiki.
  • Archive the PR's card which should now be in the "Done" column of the STL Code Reviews project.

When Adding Features

  • Microsoft-internal: Update stl.xlsx. (Note to contributors: this spreadsheet contains the same information as our cxx20 tagged issues; Excel simply provides an easy-to-scan dashboard.)
    • Change the Status filter to display patch papers; restore this when you're done.
    • Change the Status from missing to the release that this will ship in. (Note that we update the Status for patch papers, but leave their Notes, Dev, and GitHub columns as saying patch.)
    • Update the Notes. We use [GH] for GitHub contributors and [14] (sometimes [17]) for unconditional features.
    • Record the Dev. This is a GitHub or Microsoft username. It should be hyperlinked to the GitHub PR that was merged.
    • Cut and paste the updated rows to sort them by their updated Status (followed by Paper, except that we try to group patches by their primary paper, so this is a manual sort).
    • Note: Certain copy-paste operations seem to clutter up the extensive Conditional Formatting, but cutting-and-pasting rows never seems to be harmful.

How To Mirror PRs

PRs with submodule changes require special handling, not described here.

  1. Update each PR to indicate that you're processing it.
    • Self-assign it, and
    • Post a comment like "I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed."
  2. Copy each PR's title and number: 
    <fstream>: fstream::close shouldn't corrupt heap (after putback) #2189
LWG-3554 Add const charT* overloads for chrono::parse #2261
weak_ptr conversions can sometimes avoid locking #2282
More precise check message for binary_semaphore #2293
update working draft revision to N4901 #2297
filesystem: two assignments of the same value #2304
<ranges>: Fix views::reverse for ranges::reverse_view lvalues #2313
  3. Transform TITLE #NNNN to GH-NNNN TITLE:
    • Use multi-cursor editing or similar techniques. Don't retype numbers - that poses digit transposition risks, etc.
    • Keep this list - you'll need it later.
    GH-2189 <fstream>: fstream::close shouldn't corrupt heap (after putback)
GH-2261 LWG-3554 Add const charT* overloads for chrono::parse
GH-2282 weak_ptr conversions can sometimes avoid locking
GH-2293 More precise check message for binary_semaphore
GH-2297 update working draft revision to N4901
GH-2304 filesystem: two assignments of the same value
GH-2313 <ranges>: Fix views::reverse for ranges::reverse_view lvalues
  4. Remove any double quotes " from the titles, to avoid headaches later.
  5. You can sort or arbitrarily rearrange the PRs at this time.
    • Sorted order is a good default, but sometimes it's desirable to merge a toolset update first, or group related PRs together.
  6. Extract the PR numbers, copy them, and prepend gh to the copies. Join each list with spaces: 
    2189 2261 2282 2293 2297 2304 2313
gh2189 gh2261 gh2282 gh2293 gh2297 gh2304 gh2313
    • VSCode has a command for this, Join Lines. Creating a keyboard shortcut can be helpful: 
      {
  "key": "alt+f9",
  "command": "editor.action.joinLines",
  "when": "editorTextFocus && !editorReadonly"
},
  7. One-time step per machine:
    1. Install gh, the GitHub CLI tool.
    2. gh auth login
    3. Press Enter to select GitHub.com, the default.
    4. Press Enter to select HTTPS, the default.
    5. Answer Y when asked "Authenticate Git with your GitHub credentials?"
    6. Choose one:
      • You can "Login with a web browser", the default. Follow the displayed instructions.
      • You can "Paste an authentication token". Follow the displayed instructions. You will additionally need to "Configure SSO" and Authorize the Microsoft organization.
  8. Ensure that:
    • Your GitHub repo is clean.
    • main is up to date.
    • You don't have any branches named ghNNNN or MSVC_PR.
  9. In the following commands, replace 000 and gh000 with the space-separated lists that you prepared earlier: 
    for %I in (000) do (gh pr checkout %I --branch gh%I && git merge --no-edit main && git reset --soft main && git commit -m "GH-%I")
git switch --create MSVC_PR main
git cherry-pick gh000
git log --reverse --oneline --no-decorate main..MSVC_PR
    • For example: 
      :: EXAMPLE: for %I in (2189 2261 2282 2293 2297 2304 2313) do (gh pr checkout %I --branch gh%I && git merge --no-edit main && git reset --soft main && git commit -m "GH-%I")
:: EXAMPLE: git switch --create MSVC_PR main
:: EXAMPLE: git cherry-pick gh2189 gh2261 gh2282 gh2293 gh2297 gh2304 gh2313
:: EXAMPLE: git log --reverse --oneline --no-decorate main..MSVC_PR
  10. What these commands do:
    • The for loop checks out each PR into a branch named ghNNNN (regardless of what the contributor's branch was named), merges main into it (to pick up any recent activity), then squashes the PR into a single commit (regardless of its complexity and history).
      • There shouldn't be any merge conflicts during this step, assuming that GitHub's web UI isn't displaying any conflicts. If a PR has conflicts with main, you should discard everything that these commands have produced, update the PR normally, then restart this mirroring process.
      • This squashing produces fine-grained commits for the MSVC repo, mirroring how each PR will be squashed into the GitHub repo's main. However, this squashing should never be force-pushed into any GitHub PR.
    • git switch --create MSVC_PR main creates a working branch named MSVC_PR, starting where main is. (If we omitted main, we'd be starting with the last squashed PR, which we wouldn't want to do.)
    • The git cherry-pick command creates a chain of commits, in the order you previously selected.
      • Merge conflicts during this step are reasonably common - this happens when multiple PRs are editing the same lines or nearby lines. Carefully resolve them, remembering things like preserving sorted order during adjacent-add conflicts. Record (in a text file) each file and its PR, so you can note this in the MSVC-PR's description. You'll also need to exactly replicate each merge conflict resolution when physically merging PRs into GitHub main.
      • If any merge conflicts are too wild/scary, just omit that PR from this merge batch. (That is, update the PR to indicate that you're no longer processing it, move it back to Work In Progress, and restart this mirroring process without that PR. After the other PRs are merged, the conflicting PR can be updated normally, verified by PR builds.)
      • Note: Until you're very familiar with this mirroring process, the safest response to any disruption is to restart it, instead of trying to salvage half-finished work and potentially damaging PRs.
      • Resolving merge conflicts during cherry-picking is what allows overlapping PRs to be merged in a single batch, accelerating our work. We're going to transfer these commits over to the MSVC repo with git apply but that command doesn't resolve conflicts.
    • Finally, the git log command prints out the commit hashes and PR numbers, which we'll use to create diffs: 
      8aa1c637 GH-2189
c558fa4c GH-2261
02913316 GH-2282
25812d66 GH-2293
210fd78b GH-2297
1200abbd GH-2304
5a33f7c6 GH-2313
  11. Using multi-cursor editing or similar techniques, transform HASH GH-NNNN to git diff HASH~1 HASH > C:\Temp\GH-NNNN.patch:
    • The directory C:\Temp is unimportant - use anything you want.
    git diff 8aa1c637~1 8aa1c637 > C:\Temp\GH-2189.patch
git diff c558fa4c~1 c558fa4c > C:\Temp\GH-2261.patch
git diff 02913316~1 02913316 > C:\Temp\GH-2282.patch
git diff 25812d66~1 25812d66 > C:\Temp\GH-2293.patch
git diff 210fd78b~1 210fd78b > C:\Temp\GH-2297.patch
git diff 1200abbd~1 1200abbd > C:\Temp\GH-2304.patch
git diff 5a33f7c6~1 5a33f7c6 > C:\Temp\GH-2313.patch
  12. Now we're going to work in your MSVC repo. Ensure that it's clean: 
    (if exist binaries rd /s /q binaries) && (if exist Intermediate rd /s /q Intermediate) && git clean -x -f --exclude=/init.props
git switch prod/fe && git pull && git submodule update && init
  13. git switch --create dev/YOUR_ALIAS/ANY_BRANCH_NAME
  14. Using multi-cursor editing or similar techniques, transform GH-NNNN TITLE to git apply --directory=src/vctools/crt/github C:\Temp\GH-NNNN.patch && git add --all && git commit -m "GH-NNNN TITLE": 
    git apply --directory=src/vctools/crt/github C:\Temp\GH-2189.patch && git add --all && git commit -m "GH-2189 <fstream>: fstream::close shouldn't corrupt heap (after putback)"
git apply --directory=src/vctools/crt/github C:\Temp\GH-2261.patch && git add --all && git commit -m "GH-2261 LWG-3554 Add const charT* overloads for chrono::parse"
git apply --directory=src/vctools/crt/github C:\Temp\GH-2282.patch && git add --all && git commit -m "GH-2282 weak_ptr conversions can sometimes avoid locking"
git apply --directory=src/vctools/crt/github C:\Temp\GH-2293.patch && git add --all && git commit -m "GH-2293 More precise check message for binary_semaphore"
git apply --directory=src/vctools/crt/github C:\Temp\GH-2297.patch && git add --all && git commit -m "GH-2297 update working draft revision to N4901"
git apply --directory=src/vctools/crt/github C:\Temp\GH-2304.patch && git add --all && git commit -m "GH-2304 filesystem: two assignments of the same value"
git apply --directory=src/vctools/crt/github C:\Temp\GH-2313.patch && git add --all && git commit -m "GH-2313 <ranges>: Fix views::reverse for ranges::reverse_view lvalues"
    • git add --all picks up added, modified, and deleted files. git commit -a wouldn't pick up added files.
    • Advanced topic: Reverse mirroring, from MSVC to GitHub, is also possible. The inverse of --directory=src/vctools/crt/github is -p5.
  15. Use gitk to inspect the result.
  16. If any MSVC-internal changes are necessary, organize them in one or more separate commits, with subjects saying "NON-GitHub: TITLE".
  17. You can now perform local builds, run the stlimport test suites, and create an MSVC-PR.
  18. Use the GH-NNNN TITLE list to prepare Markdown-formatted PR notes. As previously mentioned, you should highlight any merge conflict resolutions.
  19. When changes affect ASAN (e.g. in <string> and <vector>), add asandev to the reviewers for the MSVC PR.
  20. Select "semi-linear merge" when merging to the MSVC repo (unless you're mirroring only one GitHub PR). This preserves the fine-grained commit structure, and makes it possible/easy to cherry-pick later.
  21. After merging all PRs to the GitHub repo, it's a good idea to verify that GitHub and MSVC are binary-identical. The easy way to do this is to copy each repo, excluding their submodules, to separate working directories. Then git diff --no-index GITHUB_DIR MSVC_DIR will detect any differences. (git feature request: the ability to exclude specified subdirectories would allow this comparison to be performed in-place.)
Clone this wiki locally