From d552a7e4178567c0e55bda53c6688980aa59b249 Mon Sep 17 00:00:00 2001 From: David Spickett Date: Fri, 18 Oct 2024 10:14:32 +0100 Subject: [PATCH 1/4] [llvm][docs] Clean up the "Landing Your Change" section of the GitHub docs * Note up front that the author may not have permissions to use the merge button and should ask a reviewer to do those steps. * Make it clear that a single commit PR can be landed with a single button click. * There are in fact 3 ways to land a multi-commit PR. * Order the ways in increasing amount of overhead for the PR author. * Put them in bullet point sections so they are visualy separate. --- llvm/docs/GitHub.rst | 91 +++++++++++++++++++++++++------------------- 1 file changed, 51 insertions(+), 40 deletions(-) diff --git a/llvm/docs/GitHub.rst b/llvm/docs/GitHub.rst index bf6e915d4458f..8421e513da43e 100644 --- a/llvm/docs/GitHub.rst +++ b/llvm/docs/GitHub.rst @@ -135,62 +135,73 @@ you won't encounter merge conflicts when landing the PR. Landing your change ------------------- -When your PR has been accepted you can use the web interface to land your change. -If you have created multiple commits to address feedback at this point you need -to consolidate those commits into one commit. There are two different ways to -do this: -`Interactive rebase `_ -with fixup's. This is the recommended method since you can control the final -commit message and inspect that the final commit looks as you expect. When -your local state is correct, remember to force-push to your branch and press -the merge button afterwards. +When your PR has been accepted you can merge your changes. -Use the button `Squash and merge` in GitHub's web interface, if you do this -remember to review the commit message when prompted. +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 +the steps here but ask one of your reviewers to click the merge button on your +behalf. -Afterwards you can select the option `Delete branch` to delete the branch -from your fork. +If the PR is a single commit, all you need to do is click the merge button in +GitHub's web interface. -You can also merge via the CLI by switching to your branch locally and run: +If your PR contains multiple commits, you need to consolidate those commits into +one commit. There are three different ways to do this, shown here with the most +commonly used first: -:: +* Use the button `Squash and merge` in GitHub's web interface, if you do this + remember to review the commit message when prompted. - gh pr merge --squash --delete-branch + Afterwards you can select the option `Delete branch` to delete the branch + from your fork. -If you observe an error message from the above informing you that your pull -request is not mergeable, then that is likely because upstream has been -modified since your pull request was authored in a way that now results in a -merge conflict. You must first resolve this merge conflict in order to merge -your pull request. In order to do that: +* `Interactive rebase `_ + with fixups. This is the recommended method since you can control the final + commit message and check that the final commit looks as you expect. When + your local state is correct, remember to force-push to your branch and press + the merge button in GitHub's web interface afterwards. -:: +* Merge using the GitHub command line interface. Switch to your branch locally + and run: - git fetch upstream - git rebase upstream/main + :: -Then fix the source files causing merge conflicts and make sure to rebuild and -retest the result. Then: + gh pr merge --squash --delete-branch -:: + If you observe an error message from the above informing you that your pull + request is not mergeable, then that is likely because upstream has been + modified since your pull request was authored in a way that now results in a + merge conflict. You must first resolve this merge conflict in order to merge + your pull request. In order to do that: - git add - git rebase --continue + :: -Finally, you'll need to force push to your branch one more time before you can -merge: + git fetch upstream + git rebase upstream/main -:: + Then fix the source files causing merge conflicts and make sure to rebuild and + retest the result. Then: - git push -f - gh pr merge --squash --delete-branch + :: + + git add + git rebase --continue + + Finally, you'll need to force push to your branch one more time before you can + merge: + + :: + + git push -f + gh pr merge --squash --delete-branch -This force push may ask if you intend to push hundreds, or potentially -thousands of patches (depending on how long it's been since your pull request -was initially authored vs. when you intended to merge it). Since you're pushing -to a branch in your fork, this is ok and expected. Github's UI for the pull -request will understand that you're rebasing just your patches, and display -this result correctly with a note that a force push did occur. + This force push may ask if you intend to push hundreds, or potentially + thousands of patches (depending on how long it's been since your pull request + was initially authored vs. when you intended to merge it). Since you're pushing + to a branch in your fork, this is ok and expected. Github's UI for the pull + request will understand that you're rebasing just your patches, and display + this result correctly with a note that a force push did occur. Problems After Landing Your Change From 84d69c540382c6cfb76755384489fb03bdb893d4 Mon Sep 17 00:00:00 2001 From: David Spickett Date: Mon, 21 Oct 2024 13:15:55 +0100 Subject: [PATCH 2/4] expand -f --- 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 8421e513da43e..307cca6ca3654 100644 --- a/llvm/docs/GitHub.rst +++ b/llvm/docs/GitHub.rst @@ -389,7 +389,7 @@ checks: ninja check # Push the rebased changes to your fork. - git push origin my_change -f + git push origin my_change --force Once your PR is approved, rebased, and tests are passing, click `Squash and Merge` on your PR in the GitHub web interface. From be05affdf8d6f4d921875e9ded57f8c75032ca71 Mon Sep 17 00:00:00 2001 From: David Spickett Date: Mon, 21 Oct 2024 13:17:21 +0100 Subject: [PATCH 3/4] more -f -> --force --- llvm/docs/GitHub.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/llvm/docs/GitHub.rst b/llvm/docs/GitHub.rst index 307cca6ca3654..2d7b594bd3aed 100644 --- a/llvm/docs/GitHub.rst +++ b/llvm/docs/GitHub.rst @@ -193,7 +193,7 @@ commonly used first: :: - git push -f + git push --force gh pr merge --squash --delete-branch This force push may ask if you intend to push hundreds, or potentially @@ -294,7 +294,7 @@ checks: ninja check # Push the rebased changes to your fork. - git push origin my_change -f + git push origin my_change --force # Now merge it gh pr merge --squash --delete-branch From f065292484e79a01abfe36449ac6f062f791a11c Mon Sep 17 00:00:00 2001 From: David Spickett Date: Wed, 23 Oct 2024 09:48:30 +0100 Subject: [PATCH 4/4] add force-with-lease note --- llvm/docs/GitHub.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/llvm/docs/GitHub.rst b/llvm/docs/GitHub.rst index 2d7b594bd3aed..ce4308022bf9f 100644 --- a/llvm/docs/GitHub.rst +++ b/llvm/docs/GitHub.rst @@ -133,6 +133,11 @@ or in some dependent code. After your PR is reviewed and accepted, you want to rebase your branch to ensure you won't encounter merge conflicts when landing the PR. +.. note:: + This guide assumes that the PR branch only has 1 author. If you are + collaborating with others on a single branch, be careful how and when you push + changes. ``--force-with-lease`` may be useful in this situation. + Landing your change -------------------