This is an archive of the discontinued LLVM Phabricator instance.

Differential D147284

docs: Document procedure for updating pull requests
ClosedPublic

Authored by tstellar on Mar 30 2023, 4:04 PM.

Details

Reviewers
ldionne
jhenderson
Commits
rG245cb1f7ef99: docs: Document procedure for updating pull requests
Summary

See discussion in #56637.

Diff Detail

Event Timeline

tstellar created this revision.Mar 30 2023, 4:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 4:04 PM
tstellar requested review of this revision.Mar 30 2023, 4:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 4:04 PM
Harbormaster completed remote builds in B222870: Diff 509845.Mar 30 2023, 4:05 PM
tstellar edited the summary of this revision. (Show Details)Mar 30 2023, 4:15 PM
jhenderson added a subscriber: jhenderson.Mar 30 2023, 11:58 PM
jhenderson added inline comments.
llvm/docs/GitHub.rst
28
37–40

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.

tstellar mentioned this in D147276: docs: Document policy around GitHub branches.Apr 19 2023, 6:34 PM
tstellar updated this revision to Diff 516609.Apr 24 2023, 8:33 PM

Add documentation about creating pull requests from a personal fork.

Harbormaster completed remote builds in B227873: Diff 516609.Apr 24 2023, 8:34 PM
tstellar updated this revision to Diff 516611.Apr 24 2023, 8:36 PM
tstellar marked an inline comment as done.

Address some review comments.

llvm/docs/GitHub.rst
37–40

Is this better?

Harbormaster completed remote builds in B227874: Diff 516611.Apr 24 2023, 8:37 PM
jhenderson added inline comments.Apr 25 2023, 1:24 AM
llvm/docs/GitHub.rst
34

Nit: single blank line, not double.

37–40

Yep, thanks.

38

"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.

39
42

Simplification that works regardless of the exact wording for the previous block.

47

Nit: single blank line, not double.

ldionne accepted this revision.May 4 2023, 11:42 AM
ldionne added a subscriber: ldionne.

I like this, I think it makes sense and it mirrors our current workflow of having a linear history.

llvm/docs/GitHub.rst
44
This revision is now accepted and ready to land.May 4 2023, 11:42 AM
tstellar updated this revision to Diff 519593.May 4 2023, 12:05 PM
tstellar marked 5 inline comments as done.

Fix some typos and other review comments.

tstellar updated this revision to Diff 519596.May 4 2023, 12:14 PM

Address one more comment.

tstellar marked an inline comment as done.May 4 2023, 12:14 PM
Harbormaster completed remote builds in B230055: Diff 519596.May 4 2023, 1:30 PM
jhenderson accepted this revision.May 5 2023, 12:01 AM

LGTM.

This revision was landed with ongoing or failed builds.May 12 2023, 11:35 PM
Closed by commit rG245cb1f7ef99: docs: Document procedure for updating pull requests (authored by tstellar). · Explain Why
This revision was automatically updated to reflect the committed changes.
tstellar added a commit: rG245cb1f7ef99: docs: Document procedure for updating pull requests.

Revision Contents

PathSize
llvm/
docs/
24 lines

Diff 521876

llvm/docs/GitHub.rst

Show All 13 Lines
Branches Branches
======== ========
Do not create any branches in the llvm/llvm-project repository. This repository Do not create any branches in the llvm/llvm-project repository. This repository
is reserved for official project branches only. We may relax this rule in is reserved for official project branches only. We may relax this rule in
the future if needed to support "stacked" pull request, but in that case only the future if needed to support "stacked" pull request, but in that case only
branches being used for "stacked" pull requests will be allowed. branches being used for "stacked" pull requests will be allowed.
Pull Requests
=============
The LLVM Project does not currently accept pull requests for the llvm/llvm-project
repository. However, there is a
`plan <https://discourse.llvm.org/t/code-review-process-update/63964>`_ to move
to pull requests in the future. This section documents the pull request
policies LLVM will be adopting once the project starts using them.
jhendersonUnsubmitted
Done
ReplyInline Actions
to pull requests in the future. This section documents the pull request
- policies will be adopting once the project starts using them.
+ policies LLVM will be adopting once the project starts using them.
Updating Pull Requests
jhenderson:
Creating Pull Requests
^^^^^^^^^^^^^^^^^^^^^^
For pull requests, please push a branch to your fork of the llvm-project and
`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>`_.
jhendersonUnsubmitted
Done
ReplyInline Actions

Nit: single blank line, not double.

jhenderson: Nit: single blank line, not double.
Updating Pull Requests
^^^^^^^^^^^^^^^^^^^^^^
When updating a pull request, you should push additional "fix up" commits to
your branch instead of force pushing. This makes it easier for GitHub to
jhendersonUnsubmitted
Done
ReplyInline Actions
^^^^^^^^^^^^^^^^^^^^^^
- When updating a pull request, you may push additional "fix up" commits to
+ When updating a pull request, you should 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.

jhenderson: "may" sounds like simply granting permission to, whereas I think we want to be encouraging, but…
track the context of previous review comments.
jhendersonUnsubmitted
Done
ReplyInline Actions
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
+ your branch instead of force pushing. This makes it easier for GitHub to
track the context of previous review comments.
jhenderson:
jhendersonUnsubmitted
Not Done
ReplyInline Actions

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.

jhenderson: I agree with the idea of being able to push "fix up" commits to the PR and using Squash & Merge.
tstellarAuthorUnsubmitted
Done
ReplyInline Actions

Is this better?

tstellar: Is this better?
jhendersonUnsubmitted
Not Done
ReplyInline Actions

Yep, thanks.

jhenderson: Yep, thanks.
If you do this, you must squash and merge before committing and
you must use the pull request title and description as the commit message.
jhendersonUnsubmitted
Done
ReplyInline Actions
track the context of previous review comments.
- If you choose to do this, you must squash and merge before committing and
+ If you 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.

jhenderson: Simplification that works regardless of the exact wording for the previous block.
The default commit message for a squashed pull request is the pull request
description, so this will allow reviewers to review the commit message before
ldionneUnsubmitted
Done
ReplyInline Actions
you must use the pull request title and description as the commit message.
- The default commit message for a squashed pull request is the pull reqeust
+ The default commit message for a squashed pull request is the pull request
description, so this will allow reviewers to review the commit message before
ldionne:
approving the commit.
Releases Releases
jhendersonUnsubmitted
Done
ReplyInline Actions

Nit: single blank line, not double.

jhenderson: Nit: single blank line, not double.
======== ========
Backporting Fixes to the Release Branches Backporting Fixes to the Release Branches
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
You can use special comments on issues to make backport requests for the You can use special comments on issues to make backport requests for the
release branches. This is done by making a comment containing one of the release branches. This is done by making a comment containing one of the
following commands on any issue that has been added to one of the "X.Y.Z Release" following commands on any issue that has been added to one of the "X.Y.Z Release"
milestones. milestones.
Show All 18 Lines