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.

Sign up for GitHub

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

Jump to bottom

Clarify merge strategy #3323

Merged
merged 2 commits into from
Oct 8, 2018
Merged

Clarify merge strategy #3323

merged 2 commits into from
Oct 8, 2018

Conversation

asvetlov
Copy link
Member

@asvetlov asvetlov commented Oct 5, 2018

Add a hint to avoid unnecessary git rebase when working on a PR.

@codecov-io
Copy link

codecov-io commented Oct 5, 2018 •
edited
Loading

Codecov Report

Merging #3323 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3323      +/-   ##
==========================================
+ Coverage      98%   98.04%   +0.03%     
==========================================
  Files          43       43              
  Lines        8020     8067      +47     
  Branches     1354     1379      +25     
==========================================
+ Hits         7860     7909      +49     
- Misses         66       67       +1     
+ Partials       94       91       -3
Impacted Files Coverage Δ
aiohttp/web_protocol.py 93.8% <0%> (+1.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93395eb...24ed30b. Read the comment docs.

webknjaz
webknjaz reviewed Oct 5, 2018
View reviewed changes
docs/contributing.rst Outdated

The project uses *Squash-and-Merge* strategy for *GitHub Merge* button.

Basically it means that there is **no need to rebase*** a Pull Request against
Copy link
Member

@webknjaz webknjaz Oct 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

*** -> **

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'd rephrase to you don't have to. Because it's a person's choice. I personally like to be doing this a lot.

webknjaz
webknjaz reviewed Oct 5, 2018
View reviewed changes
docs/contributing.rst Outdated

Basically it means that there is **no need to rebase*** a Pull Request against
*master* branch. Just ``git merge`` *master* into your working copy (a fork) if
needed. The Pull Request is automatically squashed into the single commit on the PR
Copy link
Member

Choose a reason for hiding this comment

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

... once the PR is accepted ...

@kxepal
Copy link
Member

kxepal commented Oct 5, 2018 •
edited
Loading

Well, rebase and master merge may produce quite different results, especially for changes against similar things. I would prefer to suggest rebase always on top of master to avoid any confusions. However, still final diff would be good to review for some stranges after any operation.

@asvetlov
Copy link
Member Author

asvetlov commented Oct 7, 2018

  1. We always squash PRs on merging.
  2. Rebase on top of master is harder than git merge master: very often it requires resolving more conflicts.
  3. After squashing all PR commits disappear, from master's point of view merge and rebase produces the same output.
  4. Given all this, I think that rebasing in our workflow is pointless: more work for the same result.
  5. The proposal targets on making life easier for newbies. Git jedies can do what they want, the jedi doesn't need such instructions at all :)

@webknjaz
Copy link
Member

webknjaz commented Oct 7, 2018

Take a look at this nice workflow: https://www.gentoo.org/glep/glep-0066.html

@asvetlov
Copy link
Member Author

asvetlov commented Oct 7, 2018

Sorry, I don't follow.
Do you propose to change the workflow? If yes -- please open a new issue.
This PR documents the current status quo, that's it.

@webknjaz
Copy link
Member

webknjaz commented Oct 7, 2018

It's more like sharing a learning resource. I think we could borrow some git configuration advices and instructions from there.

@asvetlov asvetlov merged commit efd6073 into master Oct 8, 2018
@asvetlov asvetlov deleted the clarify-merge-strategy branch October 8, 2018 05:54
@asvetlov
Copy link
Member Author

asvetlov commented Oct 8, 2018

Ok.
I've merged the PR.
If you want to discuss Gentoo workflow -- please open a new issue.

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@webknjaz webknjaz webknjaz left review comments

Assignees
No one assigned
Labels
outdated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@asvetlov @codecov-io @kxepal @webknjaz