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

[MRG + 2] add link in the contributing doc for rebasing on master #7541

Merged
merged 1 commit into from
Oct 6, 2016

Conversation

TomDLT
Copy link
Member

@TomDLT TomDLT commented Sep 30, 2016

@amueller
Copy link
Member

Thanks. Should we also link to the page in general as recommended reading, in addition to the official git docs?

@TomDLT
Copy link
Member Author

TomDLT commented Sep 30, 2016

Done

@amueller
Copy link
Member

lgtm

@amueller amueller changed the title [MRG] add link in the contributing doc for rebasing on master [MRG + 1] add link in the contributing doc for rebasing on master Sep 30, 2016
@NelleV
Copy link
Member

NelleV commented Sep 30, 2016

I am so against enforcing rebasing… I actually had to help people fix git rebasing problem in the early days of scikit-learn during our first sprint ever and it was not a good feeling.

@NelleV
Copy link
Member

NelleV commented Sep 30, 2016

The text is fine, btw so +1.

@NelleV NelleV changed the title [MRG + 1] add link in the contributing doc for rebasing on master [MRG + 2] add link in the contributing doc for rebasing on master Sep 30, 2016
@amueller
Copy link
Member

I think we should ask people to rebase only in case of conflicts, in which case it's better then merging master.

@NelleV
Copy link
Member

NelleV commented Sep 30, 2016

I agree it is better but beginners typically make the mistake of rebasing master onto their branch instead of rebasing their branch onto master ( 😟) and the history is then all screwed up (😢) so you need to reset the HEAD to the instance when it was still correct, which means explaining reflog (😭)…

All of this is fine for people who have used git a fair amount of time, but unfortunately, it is typically beginners that mess up rebases and end up in the case where you have to run a number of obscure commands to fix the issue. I typically never use merge (unless with the --ff-only option), but I also never teach rebase to my students.

@TomDLT
Copy link
Member Author

TomDLT commented Sep 30, 2016

So how do you suggest to solve conflicts with master branch?

@NelleV
Copy link
Member

NelleV commented Sep 30, 2016

Merging is less risky.
I am actually curious what would merging master and then "Squash & merge" do. It seems like it could either screw up bad or be the same than rebasing + squash & merge.

@NelleV
Copy link
Member

NelleV commented Sep 30, 2016

My tests seem to imply screw up very badly. I don't think github` will have a squash and merge button for cases were the master branch has been merged into the working branch.

@jnothman
Copy link
Member

jnothman commented Oct 1, 2016

I had thought squash and merge should work fine... after all it's just
going to merge in the diff that it shows.

On 1 October 2016 at 08:57, Nelle Varoquaux notifications@github.com
wrote:

My tests seem to imply screw up very badly. I don't think github` will
have a squash and merge button for cases were the master branch has been
merged into the working branch.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7541 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz6769qpChFGKk1ME3MvyP418oO8slks5qvZPKgaJpZM4KK9M6
.

@NelleV
Copy link
Member

NelleV commented Oct 2, 2016

I didn't test through the interface, but doing it manually on my computer so maybe I messed up. It'd be interesting to know.

@amueller
Copy link
Member

amueller commented Oct 5, 2016

I would expect there to be a screw-up. You have two ancestors and you want to rebase one of them on top of the other? that's kinda weird.

@amueller
Copy link
Member

amueller commented Oct 5, 2016

@NelleV
Copy link
Member

NelleV commented Oct 6, 2016

Well… Unless someone tests and demonstrates the github interface deals with this properly, let's merge the PR as is and change it later if we discover that squash and merge works fine if master has been merged in the branch of the PR.

@NelleV NelleV merged commit 3beec40 into scikit-learn:master Oct 6, 2016
amueller pushed a commit to amueller/scikit-learn that referenced this pull request Oct 14, 2016
@TomDLT TomDLT deleted the git_help branch December 20, 2016 14:24
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants