docs: add info about working with fixup commits #39110

gkalpak · 2020-10-04T19:40:28Z

Using fixup commits when addressing review feedback can considerably improve the review experience on subsequent reviews.

This commit adds information and guidelines for contributors on how to work with fixup commits.

Fixes #33042.

Using fixup commits when addressing review feedback can considerably improve the review experience on subsequent reviews. This commit adds information and guidelines for contributors on how to work with fixup commits. Fixes angular#33042

petebacondarwin

Nice bit of work @gkalpak - you are getting ahead of the fixit coming up a the end of the month!

I wonder if there is room in this PR for discussion of whether the PR author should reorder fixup commits manually to ensure that they can be squashed without conflicts or whether they should leave them at the end of the PR commit list to make it more clear to reviewers what has changed.

I think we could also have a discussion about squashing fixup commits from previous rounds of review so that the only fixup commits in a PR are the ones that have not been approved yet?

CONTRIBUTING.md

docs/FIXUP_COMMITS.md

petebacondarwin · 2020-10-05T06:57:09Z

docs/FIXUP_COMMITS.md

+See the [`git` docs](https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---autosquash) for more details.
+
+
+### Configuring `git` to auto-squash by default


I would not recommend setting this default. When working with PRs I often rebase interactively but do not want to autosquash. For example, if a PR was built on top of another one, and you want to remove those commits when the upstream PR has been merged to master. Or when re-wording commit messages in a PR after a review finds typos.

I am not recommeding it, but I wanted to have it mentioned.
_{(FWIW, I do have this set to true 🤷‍♂️)}

Are you saying it would be better to remove this section or word it differently (how?) to make it not sound as if we are recommending it?

Definitely not remove it, but make it a bit less prominent so that it doesn't sound like a recommendation/requirement.

petebacondarwin

Nice fixup commit @gkalpak :-D

gkalpak · 2020-10-05T11:46:51Z

I wonder if there is room in this PR for discussion of whether the PR author should reorder fixup commits manually to ensure that they can be squashed without conflicts or whether they should leave them at the end of the PR commit list to make it more clear to reviewers what has changed.

I think we could also have a discussion about squashing fixup commits from previous rounds of review so that the only fixup commits in a PR are the ones that have not been approved yet?

I originally had an "Advanced tips" section discussing these things, but I ended up removing it.
Personally, I would love for the process to be more opinionated on the way these things are handled, but I removed the section because:

These things are somewhat controversial/bikeshedable (i.e. I don't feel everyone on the team uses the same conventions).
It looked a little scary for inexperienced contributors.
These mainly apply to multi-commit PRs, which are rare (esp. for contributors, which usually address smaller/simpler issues).

That said, I am all for discussing this, standardizing the process and enhacing the docs 🚀

petebacondarwin · 2020-10-05T11:48:12Z

That said, I am all for discussing this, standardizing the process and enhacing the docs 🚀

I think a discussion to be had during the next fixit!

AndrewKushnir · 2020-10-06T00:02:12Z

CONTRIBUTING.md

-   * Make the required updates.
-   * Re-run the Angular test suites to ensure tests are still passing.
-   * Rebase your branch and force push to your GitHub repository (this will update your Pull Request):
+#### Addressing review feedback


That's a great section, thanks for adding this @gkalpak 👍
Another very common thing that we can include into this section (in the future PRs) is how to change commit message (git commit --amend) as it's often time needed as a result of the lint CI failure and/or reviewer comment.

Sure! There is already a note below with instructions on how to amend commits (for the purpose of updating the commit message):

Fixup commits (as shown above) are preferred when addressing review feedback, but in some cases you may need to amend the original commit instead of creating a fixup commit (for example, if you want to update the commit message).
To amend the last commit and update the Pull Request:

git commit --all --amend git push --force-with-lease

Are you suggesting expanding on that (or maybe making it its own section (with a sub-heading, etc.))?

Yeah, I was thinking if we can make it a separate sub-section, so that we can refer to it (and provide a link to PR authors when needed). This is a very common thing that we request the commit message to be updated (especially common for docs-related commits) when other changes are ok.

For reference: #39215

Using fixup commits when addressing review feedback can considerably improve the review experience on subsequent reviews. This commit adds information and guidelines for contributors on how to work with fixup commits. Fixes #33042 PR Close #39110

…UTING.md` A common review request is updating the commit message of a commit. Since this is something that is not straight forward for inexperienced contributors, it is useful to be able to point a contributor to some docs outlining the process. This commit adds such a section in `CONTRIBUTING.md` (as discussed in angular#39110 (comment)).

…UTING.md` (#39215) A common review request is updating the commit message of a commit. Since this is something that is not straight forward for inexperienced contributors, it is useful to be able to point a contributor to some docs outlining the process. This commit adds such a section in `CONTRIBUTING.md` (as discussed in #39110 (comment)). PR Close #39215

angular-automatic-lock-bot · 2020-12-08T16:32:28Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Oct 4, 2020

gkalpak added action: merge The PR is ready for merge by the caretaker comp: docs target: patch This PR is targeted for the next patch release labels Oct 4, 2020

ngbot bot modified the milestone: needsTriage Oct 4, 2020

gkalpak marked this pull request as ready for review October 4, 2020 19:41

docs: add info about working with fixup commits

f36f4d7

Using fixup commits when addressing review feedback can considerably improve the review experience on subsequent reviews. This commit adds information and guidelines for contributors on how to work with fixup commits. Fixes angular#33042

gkalpak force-pushed the docs-fixup-commits branch from e6fae8f to f36f4d7 Compare October 4, 2020 20:07

pullapprove bot requested a review from AndrewKushnir October 4, 2020 20:08

petebacondarwin reviewed Oct 5, 2020

View reviewed changes

fixup! docs: add info about working with fixup commits

1812576

petebacondarwin approved these changes Oct 5, 2020

View reviewed changes

fixup! docs: add info about working with fixup commits

9d6f120

AndrewKushnir approved these changes Oct 6, 2020

View reviewed changes

josephperrott closed this in 6e0a8eb Oct 6, 2020

gkalpak deleted the docs-fixup-commits branch October 9, 2020 13:48

gkalpak mentioned this pull request Oct 10, 2020

docs: add linkable section about updating commit messages in CONTRIBUTING.md #39215

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 8, 2020

		See the [`git` docs](https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---autosquash) for more details.


		### Configuring `git` to auto-squash by default

docs: add info about working with fixup commits #39110

docs: add info about working with fixup commits #39110

Uh oh!

Conversation

gkalpak commented Oct 4, 2020

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petebacondarwin commented Oct 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Dec 8, 2020

Uh oh!

Uh oh!

gkalpak commented Oct 5, 2020 •

edited

Loading