Skip to content

stop recommend rebase for new contributors #496

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

Merged
merged 2 commits into from
Jun 16, 2019
Merged

stop recommend rebase for new contributors #496

merged 2 commits into from
Jun 16, 2019

Conversation

methane
Copy link
Member

@methane methane commented Jun 13, 2019

We should not recommend "rebase" and "push -f" to new users.

These command requires high skill when they cause trouble.

We should not recommend "rebase" and "push -f" to new users.
Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

I think it makes sense.
We squash on merging anyway

gitbootcamp.rst Outdated
you run ``git merge upstream/master``.

When it happens, you need to resolve conflict. See following articles for
how to resolve conflicts.
Copy link
Member Author

Choose a reason for hiding this comment

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

Could anyone check my English?

  • happens vs happened
  • "happens, " -- this comma is usual?
  • "for how to..." -- is this usual wording?

Copy link
Member

Choose a reason for hiding this comment

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

  • You're using "happens" correctly.

  • The common after "happens" is a typical usage.

  • I'd say something more like "See these articles about resolving conflicts:". The "for how to" is a little awkward.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@asvetlov asvetlov requested a review from Mariatta June 13, 2019 13:57
@Mariatta
Copy link
Member

Hmm, I don't like the side effect of merging from upstream: the merge "activity" appears in other people's PR on GitHub web UI.

Example screenshot:
Screen Shot 2019-06-13 at 9 06 06 AM

It's quite noisy when viewing PRs. Rebasing is a cleaner approach IMO.
But if everyone else is ok with merging, then I'm fine with it.

@methane
Copy link
Member Author

methane commented Jun 14, 2019

Hmm, I don't like the side effect of merging from upstream: the merge "activity" appears in other people's PR on GitHub web UI.

Would you please link? I want to confirm what happened there.

@willingc
Copy link
Collaborator

It may be better to recommend to new contributors that they should be developing on a branch in their account for each feature. As for rebase, I rarely use git pull or git merge when working with repos on GitHub. I have taught the following workflow to new open source contributors for years with good success: https://www.slideshare.net/willingc/yes-you-can-git It uses a git fetch git rebase approach to reduce merge conflicts for new users.

I don't object to adding a note for new contributors about rebase though.

@methane
Copy link
Member Author

methane commented Jun 16, 2019

It uses a git fetch git rebase approach to reduce merge conflicts for new users.

git rebase doesn't reduce conflict. It even increase conflict sometime.

@willingc
Copy link
Collaborator

git rebase doesn't reduce conflict. It even increase conflict sometime.

@methane The explicit git fetch/git rebase combined with specific remote naming, branch usage, and update ordering approach that I teach does reduce merge conflicts significantly.

However, I do agree with you that using rebase when not following the particular workflow that I use can have unexpected results.

I recommend a few things for this section:

As @asvetlov mentions, it doesn't really matter since we squash.

Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

If I have ever used rebase, it was because my fork and local repository was ahead or beside upstream. For normal updates when behind, I fetch and merge to each workspace, and to PR branches when working on them. I only get merge conflicts in idlelib when an old PR branch touches code affected by a different merged PR.

In other words, the revised doc matches what I do and have done successfully, as a git non-expert, for 2 years.

@willingc
Copy link
Collaborator

the revised doc matches what I do and have done successfully, as a git non-expert, for 2 years.

Good to hear @terryjreedy. I'll approve this PR as stands and add the other notes in a separate PR.

@willingc willingc merged commit 43615a5 into python:master Jun 16, 2019
@methane methane deleted the no-rebase branch June 16, 2019 22:53
@methane
Copy link
Member Author

methane commented Jun 16, 2019

FWIW, I leave two comments about why I think merge is better.

If the guide is for contributing to general OSS, it is recommended to keep commits in the pull request branch clean. That's why many people recommend rebase. But we're using "Squash and merge" workflow, cleaning commits in pull request branch is not so important. Keeping history clean is complex than just using merge.

When people are expert and keep history in the branch clean with commit --amend and rebase -i, git rebase origin/master may be easy as git merge origin/master. But when people change same part of the code several times and the same part is changed in origin/master too, git rebase origin/master forces the user to resolve conflict several times.

There are many documents about git rebase and merge on the internet. In our document, let's focus on people using git only for contributing to cpython.

@methane
Copy link
Member Author

methane commented Jun 17, 2019

And github creates much noise when rebase && push -f is used. I don't know why.

@methane
Copy link
Member Author

methane commented Jun 18, 2019

@Mariatta I found the issue you reported. And it may be caused by rebase workflow!

See this log: https://github.com/ma8ma/cpython/commits/9e1425d3d9249f89850274b99c2178b29c9064f6?after=9e1425d3d9249f89850274b99c2178b29c9064f6+349

image

After 3.8a1 is released, ma8ma creates different commit object (see "ma8ma committed" in the history).
Why? I don't know. But note that master branch is not straight. See this graph:

image

Maybe, git pull --rebase upstream/master accidentally create "straight" commit graph. And ma8ma might use git push -f origin master, as the previous devguide said. But I'm not sure.

Anyway, merge workflow doesn't create such "rewrote" commits.

@Mariatta
Copy link
Member

Huh, very interesting.. Alright. Thanks for following through with this, @methane.

@methane methane mentioned this pull request Oct 31, 2019
AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Jun 17, 2022
* stop recommend rebase for new contributors

We should not recommend "rebase" and "push -f" to new users.

* fix English and rest syntax
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.

7 participants