[PATCH] D147284: docs: Document procedure for updating pull requests

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/GitHub.rst:34
+`create a pull request from the fork <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork>`_.
+
+
----------------
Nit: single blank line, not double.


================
Comment at: llvm/docs/GitHub.rst:38
+^^^^^^^^^^^^^^^^^^^^^^
+When updating a pull request, you may push additional "fix up" commits to
+your branch instead of force pushing.  This makes it easier for github to
----------------
"may" sounds like simply granting permission to, whereas I think we want to be encouraging, but not strictly mandating it, to avoid losing context etc.


================
Comment at: llvm/docs/GitHub.rst:39
+When updating a pull request, you may push additional "fix up" commits to
+your branch instead of force pushing.  This makes it easier for github to
+track the context of previous review comments.
----------------



================
Comment at: llvm/docs/GitHub.rst:42
+
+If you choose to do this, you must squash and merge before committing and
+you must use the pull request title and description as the commit message.
----------------
Simplification that works regardless of the exact wording for the previous block.


================
Comment at: llvm/docs/GitHub.rst:47
+approving the commit.
+
 
----------------
Nit: single blank line, not double.


================
Comment at: llvm/docs/GitHub.rst:37-40
+If you choose to do this, you must squash and merge before committing and
+you must use the pull request title and description as the commit message.
+This will allow reviewers to review the commit message before approving the
+commit.
----------------
tstellar wrote:
> jhenderson wrote:
> > I agree with the idea of being able to push "fix up" commits to the PR and using Squash & Merge. It might be worth emphasising that the default final commit message will match exactly the PR title and body. I've been known to include additional comments in my initial PR description, to help reviewers, that don't necessarily want to be part of the final commit message, and I'm sure others have too.
> Is this better?
Yep, thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147284/new/

https://reviews.llvm.org/D147284