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] DOC: Added rebasing and squashing section to contributing.rst #6176

Closed
wants to merge 3 commits into from
Closed

[MRG] DOC: Added rebasing and squashing section to contributing.rst #6176

wants to merge 3 commits into from

Conversation

devashishd12
Copy link
Contributor

Added rebasing and squashing section to contributing.rst as agreed here. Used this comment to add to the section.

@devashishd12 devashishd12 changed the title DOC: Added rebasing and squashing section to contributing.rst [MRG] DOC: Added rebasing and squashing section to contributing.rst Jan 17, 2016
@devashishd12
Copy link
Contributor Author

@amueller @rvraghav93 could you please take a look at this? Thanks!

@@ -207,6 +207,51 @@ and Cython optimizations.
<http://astropy.readthedocs.org/en/latest/development/workflow/development_workflow.html>`_
sections.

Rebasing and squashing commits
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Member

Choose a reason for hiding this comment

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

Is this the correct format for header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it gives this kind of a header.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for checking!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking. Maybe it would be better to have a heading like this?

@raghavrv
Copy link
Member

I think we can structure it something like this -

Resolving merge conflicts...
------------------

Why they happen - 1 or 2 lines

What to do when that happens - two options

Why we choose rebase over merge

How to rebase properly

External link/man page to rebase

Squashing commits
-------------------
Why we prefer less, but meaningful (number of) commits - (To keep history clean and to help backtrack regressions to a single commit)

How to squash

How to force push and update the branch

Some external links on interactive rebase/squashing

@devashishd12 devashishd12 changed the title [MRG] DOC: Added rebasing and squashing section to contributing.rst [WIP] DOC: Added rebasing and squashing section to contributing.rst Jan 20, 2016
@devashishd12
Copy link
Contributor Author

@rvraghav93 is this a bit better?

@raghavrv
Copy link
Member

This looks much better. I will take a good look at it in some time


2. Make a backup just in case::

$ git branch my-feature-branch-bak my-feature-branch
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? There is reflog in extreme cases no? It would be better if we have it simple I feel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes true....

@devashishd12
Copy link
Contributor Author

@rvraghav93 I was thinking of putting the interactive rebasing method before the cherry-pick one..... Also I just copied the gist of squashing the commits that's why I kept the link to the scipy tut intact. What do you think?

@devashishd12
Copy link
Contributor Author

ping @rvraghav93. Sorry for disturbing but should I proceed with the above changes? Could you please have a look at this if you have the time?

@raghavrv
Copy link
Member

Apologies for the late response! I'll take a look at it now and let you know


$ git checkout my-feature-branch

2. Rebase on master::
Copy link
Member

Choose a reason for hiding this comment

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

You should maybe add a line on updating master before?

@raghavrv
Copy link
Member

This looks very good. Would help a lot of people new to git :)

Ping @amueller and @jnothman for reviews

@devashishd12
Copy link
Contributor Author

Thanks a lot for reviewing! I'll address the comments and make the changes asap 👍

@devashishd12
Copy link
Contributor Author

I've addressed the comments. Hope this is better now :)

@raghavrv
Copy link
Member

See if you can use your documentation to squash the commits of this PR ;)

@devashishd12
Copy link
Contributor Author

Hahahaha! I'll do that now!

@amueller
Copy link
Member

amueller commented Feb 9, 2016

is this ready for review? If so, please rename.

@amueller
Copy link
Member

amueller commented Feb 9, 2016

I think we should talk first about squashing, then rebasing. That really lowers the chance of having rebase conflicts.

@devashishd12
Copy link
Contributor Author

@amueller oh sorry I'll rename it now. Yes this is ready for review.

@devashishd12 devashishd12 changed the title [WIP] DOC: Added rebasing and squashing section to contributing.rst [MRG] DOC: Added rebasing and squashing section to contributing.rst Feb 10, 2016
@devashishd12
Copy link
Contributor Author

@amueller I have addressed the comments and changed the order of squash and rebase. Could you please check? Thanks!

^^^^^^^^^^^^^^^^^

To keep the history clean and to help backtrack regressions to a single commit we
prefer less (but meaningful) number of commits.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd reword this to we prefer to have less commits that are more meaningful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

umm I'm not quite sure about this.... It still retains the same meaning but makes it unnecessarily verbose. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think it should be we prefer less (but more meaningful) commits.


.. note::

Detailed guide on solving merge conflicts can be found on `git-scm.com
Copy link
Contributor

Choose a reason for hiding this comment

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

Detailed guide... -> A detailed guide...

@devashishd12
Copy link
Contributor Author

@nelson-liu I've addressed the comments. Good catches!

@nelson-liu
Copy link
Contributor

@dsquareindia why did you close this? I think it's a great improvement; the part about rebasing is a common question e.g. #6446 (comment)

@nelson-liu
Copy link
Contributor

@dsquareindia would you mind if i cherry-picked your commit and opened a new pr? I think this addition is quite necessary.

@devashishd12
Copy link
Contributor Author

Sorry for that; I'll open it again. @nelson-liu I'll be happy to continue work on this if needed :)

@devashishd12 devashishd12 reopened this Apr 25, 2016
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Another way to squash commits is through interactive rebasing which is one the most powerful
features of ``rebase``. If you want to squash the latest 3 commits together (this number can
Copy link
Contributor

Choose a reason for hiding this comment

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

use $ git log --oneline to see past commits?

@nelson-liu
Copy link
Contributor

could you also rebase on master? maybe use your own instructions ;)

@devashishd12
Copy link
Contributor Author

@nelson-liu sorry for keeping this PR idle. You can cherry pick this if you want and open a new pr 👍 . Won't be getting a chance to work on this anytime soon...

@jnothman
Copy link
Member

I think this needs to be cut back to discuss rebasing etc while working on a PR. We no longer generally want contributors to do squashing, in preference for the "squash and merge" facility provided by github.

@nelson-liu
Copy link
Contributor

@jnothman I agree and I've seen it happen a few times recently. maybe having this in the docs + perhaps adding it to the PR template would be good tasks for the scipy sprint?

@amueller
Copy link
Member

solved by #7541 IMHO. Feel free to reopen / update. I think rewriting the same docs over and over again is probably not the point.

@amueller amueller closed this Oct 13, 2016
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

5 participants