From cc738a3f5061c07cf0db76066813dc55da86965e Mon Sep 17 00:00:00 2001 From: David Spickett Date: Wed, 23 Oct 2024 10:25:19 +0100 Subject: [PATCH 1/2] [llvm][docs] Add Approvals section to GitHub guide Based on feedback that when reading the document as a guide, it's odd that it skips right from updating the PR to merging it. The section is a link to the existing Code Review guide's text on the topic. I have updated that to mention required reviewers, which some subprojects do use (libcxx is one) but most don't. Also we use the words "accepted" and "approved" interchangeably, so I've swapped one instance so it's consistent between paragraphs. --- llvm/docs/CodeReview.rst | 8 +++++++- llvm/docs/GitHub.rst | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/llvm/docs/CodeReview.rst b/llvm/docs/CodeReview.rst index 56798ae4faf0c..15e3e018e1158 100644 --- a/llvm/docs/CodeReview.rst +++ b/llvm/docs/CodeReview.rst @@ -142,12 +142,18 @@ from specific performance tests), please explain as many of these up front as possible. This allows the patch author and reviewers to make the most efficient use of their time. +.. _lgtm_how_a_patch_is_accepted: + LGTM - How a Patch Is Accepted ------------------------------ A patch is approved to be committed when a reviewer accepts it, and this is almost always associated with a message containing the text "LGTM" (which -stands for Looks Good To Me). Only approval from a single reviewer is required. +stands for Looks Good To Me). + +Only approval from a single reviewer is required, unless the pull request +has required reviewers. In which case, you must have approval from all of those +reviewers. When providing an unqualified LGTM (approval to commit), it is the responsibility of the reviewer to have reviewed all of the discussion and diff --git a/llvm/docs/GitHub.rst b/llvm/docs/GitHub.rst index ce4308022bf9f..75cdfa6abe021 100644 --- a/llvm/docs/GitHub.rst +++ b/llvm/docs/GitHub.rst @@ -138,10 +138,16 @@ you won't encounter merge conflicts when landing the PR. collaborating with others on a single branch, be careful how and when you push changes. ``--force-with-lease`` may be useful in this situation. +Approvals +--------- + +Before merging a PR you must have the required approvals. See +:ref:`lgtm_how_a_patch_is_accepted` for more details. + Landing your change ------------------- -When your PR has been accepted you can merge your changes. +When your PR has been apporoved you can merge your changes. If you do not have write permissions for the repository, the merge button in GitHub's web interface will be disabled. If this is the case, continue following From 066ab11554db969e117e33579b9a5fa3ea7e46db Mon Sep 17 00:00:00 2001 From: David Spickett Date: Wed, 23 Oct 2024 13:55:12 +0100 Subject: [PATCH 2/2] typo --- llvm/docs/GitHub.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/docs/GitHub.rst b/llvm/docs/GitHub.rst index 75cdfa6abe021..d785d9da9a7f4 100644 --- a/llvm/docs/GitHub.rst +++ b/llvm/docs/GitHub.rst @@ -147,7 +147,7 @@ Before merging a PR you must have the required approvals. See Landing your change ------------------- -When your PR has been apporoved you can merge your changes. +When your PR has been approved you can merge your changes. If you do not have write permissions for the repository, the merge button in GitHub's web interface will be disabled. If this is the case, continue following