Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a note about not modifying the git history to the quick guide #588

Merged
merged 4 commits into from
May 24, 2020

Conversation

DahlitzFlorian
Copy link
Contributor

Closes: #579

  • Add a note to the quick about not modifying the git history
  • Explicitly state not using force-pushing at point "7. Review and address comments on your Pull Request"

@aeros What do you think about it?

Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @DahlitzFlorian. I think it has the right general idea, but I would recommend a couple of changes:

pullrequest.rst Outdated Show resolved Hide resolved
pullrequest.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the latest revisions, it looks good to me.

However, I would like to give some time for others to take a look at the PR prior to merging it; especially since it's adding to a part of the devguide that's highly visible. Would you care to take a look @Mariatta and/or @willingc?

pullrequest.rst Outdated Show resolved Hide resolved
@terryjreedy
Copy link
Member

The one sentence is an improvement; I suggested another to consider.

The other (usually bad) reason people do force-pushes is to synchronize a branch to master. This is normally not needed and may destroy the PR by introducing 100s of spurious commits. But I don't know if recent changes have properly promoted 'git merge origin/master' as the proper way or not.

@willingc
Copy link
Collaborator

@DahlitzFlorian Thanks for the PR. The existing wording in the PR is fine.

Personally, I don't find force pushes to a branch to be an issue when doing a rebase of your branch to the current head of master. I agree that changing commits and squashing make it more difficult to review.

I often use the following on many projects to avoid merge conflicts and continue development:

git push origin pr-branch  # origin is my github account and a PR is opened.
# Time goes by then I do the following
git fetch upstream   # upstream is cpython repo
git rebase upstream/master
git push origin pr-branch -f 

This method brings all commits of the PR to the HEAD of master. In my personal workflow, I never use merge.

pullrequest.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as-is or with my suggested comments. Thanks @DahlitzFlorian.

pullrequest.rst Outdated Show resolved Hide resolved
@aeros
Copy link
Contributor

aeros commented May 22, 2020

Great points @terryjreedy and @willingc.

The other (usually bad) reason people do force-pushes is to synchronize a branch to master. This is normally not needed and may destroy the PR by introducing 100s of spurious commits.

I've certainly done this at least once in one of my early PRs before I discovered the approach that Carol mentioned w/ rebasing. I only do that though when the lifespan of a PR is particularly long, otherwise I just deal with the conflicts directly.

@aeros aeros merged commit e0460a9 into python:master May 24, 2020
@aeros
Copy link
Contributor

aeros commented May 24, 2020

Thanks again for working on this @DahlitzFlorian! :-)

@willingc
Copy link
Collaborator

Thank you @DahlitzFlorian. I appreciate your help.

@DahlitzFlorian DahlitzFlorian deleted the fix-issue-579 branch May 24, 2020 17:05
AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Jun 17, 2022
…thonGH-588)


Co-authored-by: Kyle Stanley <aeros167@gmail.com>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify force-push usage for CPython PRs
5 participants