Skip to content

Commit

Permalink
Update dev docs for GitHub Squash and merge option (astropy#15100)
Browse files Browse the repository at this point in the history
* Update dev docs for GitHub Squash and merge option

- Apply suggestions from @eerovaher code review
- Merge when_to_rebase into dev workflow
- Fix sphinx warnings and restore accidentally remove content
- Fix another sphinx warning

* Add checkbox to PR template.
Move section to maintainer workflow. Clarify policy.

* Fix links in PR checklist

---------

Co-authored-by: Eero Vaher <eero.vaher@gmail.com>
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
  • Loading branch information
3 people committed Aug 10, 2023
1 parent 382e870 commit a7f55f5
Show file tree
Hide file tree
Showing 6 changed files with 110 additions and 106 deletions.
4 changes: 4 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,7 @@ If this pull request is unrelated to any issues, please remove
the following line. -->

Fixes #<Issue Number>

<!-- Optional opt-out -->

- [ ] By checking this box, the PR author has requested that maintainers do **NOT** use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.
2 changes: 1 addition & 1 deletion .github/workflows/open_actions.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ jobs:
- [ ] Do the proposed changes follow the [Astropy coding guidelines](https://docs.astropy.org/en/latest/development/codeguide.html)?
- [ ] Are tests added/updated as required? If so, do they follow the [Astropy testing guidelines](https://docs.astropy.org/en/latest/development/testguide.html)?
- [ ] Are docs added/updated as required? If so, do they follow the [Astropy documentation guidelines](https://docs.astropy.org/en/latest/development/docguide.html#astropy-documentation-rules-and-guidelines)?
- [ ] Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see ["When to rebase and squash commits"](https://docs.astropy.org/en/latest/development/when_to_rebase.html).
- [ ] Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for [rebase](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#rebase-if-necessary) and [squash](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#squash-if-necessary).
- [ ] Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the [bot](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#pre-commit).
- [ ] Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
- [ ] Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
Expand Down
1 change: 0 additions & 1 deletion docs/development/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ as a whole, see :doc:`vision`.
workflow/development_workflow
workflow/virtual_pythons
workflow/get_devel_version
when_to_rebase
codeguide
docguide
style-guide
Expand Down
60 changes: 0 additions & 60 deletions docs/development/when_to_rebase.rst

This file was deleted.

111 changes: 69 additions & 42 deletions docs/development/workflow/development_workflow.rst
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ Pre-requisites

Before following the steps in this document you need:

+ an account on `GitHub`_
+ a local copy of the astropy source. Instructions for doing that, including the
* an account on `GitHub`_
* a local copy of the astropy source. Instructions for doing that, including the
basics you need for setting up git and GitHub, are at :ref:`get_devel`.

Strongly Recommended, but not required
Expand Down Expand Up @@ -535,27 +535,30 @@ for further information.

.. _rebase:

Rebase, but only if asked
*************************
Rebase if necessary
*******************

Sometimes the maintainers of Astropy may ask a pull request to be *rebased*
or *squashed* in the process of reviewing a pull request for merging into
the main Astropy *main* repository.
Rebasing means taking your changes and applying them to the latest
version of the ``main`` branch of the official ``astropy`` repository as though that was the
version you had originally branched from. Each individual commit remains
visible, but with new commit hashes.

The decisions of when to request a *squash* or *rebase* are left to
individual maintainers. These may be requested to reduce the number of
visible commits saved in the repository history, or because of code changes
in Astropy in the meantime. A rebase may be necessary to allow the Continuous
Integration tests to run. Both involve rewriting the `git`_ history, meaning
that commit hashes will change, which is why you should do it only if asked.
When to rebase
==============

Conceptually, rebasing means taking your changes and applying them to the latest
version of the development branch of the official Astropy as though that was the
version you had originally branched from. Each individual commit remains
visible, but with new metadata/commit hashes. Squashing commits changes the
metadata/commit hash, and also removes separate visibility of individual
commits; a new commit and commit message will only contain a textual
list of the earlier commits.
Pull requests **must** be rebased (but not necessarily squashed to a single
commit) if at least one of the following conditions is met:

* There are conflicts with ``main``.
* There are commits in ``main`` (after the PR branch point) needed for continuous
integration tests to run correctly.
* There are merge commits from ``astropy/main`` in the PR commit history (merge
commits from PRs to the user's fork are fine).

.. _howto_rebase:

How to rebase
=============

It is easier to make mistakes rebasing than other areas of `git`_, so before you
start make a branch to serve as a backup copy of your work::
Expand All @@ -572,11 +575,6 @@ is prevented, and a ``--force`` option will be required.
way that preserves the commit history of both. The purpose of rebasing is
rewriting the commit history of your branch, not preserving it.

.. _howto_rebase:

How to rebase
*************

Behind the scenes, `git`_ is deleting the changes and branch you made, making the
changes others made to the development branch of Astropy, then re-making your
branch from the development branch and applying your changes to your branch.
Expand All @@ -590,27 +588,56 @@ Now, do the rebase::

git rebase astropy/main

You are more likely to run into *conflicts* here — places where the changes you
made conflict with changes that someone else made — than anywhere else. Ask for
help if you need it. Instructions are available on how to
`resolve merge conflicts after a Git rebase <https://help.github.com/en/articles/resolving-merge-conflicts-after-a-git-rebase>`_.
You are more likely to run into *conflicts* here — places where the changes you made
conflict with changes that someone else made — than anywhere else. Ask for help if you
need it. Instructions are available on how to `resolve merge conflicts after a Git
rebase
<https://help.github.com/en/articles/resolving-merge-conflicts-after-a-git-rebase>`_.

.. _squash-if-necessary:

Squash if necessary
*******************

Squashing refers to combining multiple commits into a single commit. This can be done
using the ``git rebase`` command or via :ref:`github-squash-and-merge`.

An Astropy maintainer will be happy to guide you through this process.

When to squash
==============

If a pull request contains commits with large (approximately > 10KB) intermediate
changes which are ultimately removed or modified in the PR, the intermediate diffs
**must** be squashed. An example is adding a large data file in one commit and then
removing it in a subsequent commit. The goal is to avoid an unnecessary increase in the
size of the ``astropy`` repository.

Squashing commits is **encouraged** when there are numerous commits which do not add
value to the commit history. Most small to medium pull requests can be done with a few
commits. Some large or intricate pull requests may require more commits to capture the
logical progression of the changes.

In general, commits that reflect a specific atomic change (e.g., "Fixed bug revealed by
tests for feature X") may have value for the history.

Commits that are good candidates for squashing include but not limited to:

* Content that gets removed later, most commonly changes in the implementation or
temporary debugging code or, especially, data files (see above).
* Non-specific commits; e.g., "Added more code and fixed stuff."
* Fixes of typos, linting fixes, or other inconsequential changes.
* Commit messages that violate the code of conduct.

.. _howto_squash:

How to squash
*************

Typically we ask to *squash* when there was a fair amount of trial
and error, but the final patch remains quite small, or when files were added
and removed (especially binary files or files that should not remain in the
repository) or if the number of commits in the history is disproportionate
compared to the work being carried out (for example 30 commits gradually
refining a final 10-line change). Conceptually this is equivalent to
exporting the final diff from a feature branch, then starting a new branch and
applying only that patch.

Many of us find that is it actually easiest to squash using rebase. In particular,
you can rebase and squash within the existing branch using::
=============

In many cases, squashing can be done by a maintainer using the :ref:`github-squash-and-merge` button on
the GitHub pull request page. If this is not possible, we typically squash using `git
rebase --interactive <https://git-scm.com/docs/git-rebase#_interactive_mode>`_. In
particular, you can rebase and squash within the existing branch using::

git fetch astropy
git rebase -i astropy/main
Expand Down
38 changes: 36 additions & 2 deletions docs/development/workflow/maintainer_workflow.rst
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,44 @@ Integrating changes via the web interface
Pull requests should always be merged via GitHub.

Make sure that pull requests do not contain a messy history with merges, etc.
If this is the case, ask the author to squash the commits or
:ref:`take over the pull request <maintainer-pr-takeover>`.
If this is the case and a simple :ref:`github-squash-and-merge` would do and
you are comfortable with it, use it. Otherwise, you can work with the author,
following instructions at :ref:`squash-if-necessary`. If the author is not
responsive or gives consent, you can also :ref:`take over the pull request <maintainer-pr-takeover>`.
If necessary, you may also :ref:`request a rebase <rebase>`.

.. _github-squash-and-merge:

Github 'Squash and merge'
=========================

.. note::

Before you use this button, first make sure the PR author has not opted out
by checking the opt-out box that comes with the PR template!

Use of the `GitHub 'Squash and merge' button
<https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits>`_
is available to all maintainers. This will squash out all commits in the pull request into
a single commit and auto-generate a commit message which can be edited.

You must be careful to clean up the final commit message before pushing the button:

* Remove repetitive, irrelevant, or inappropriate commit messages.
Merge commits, if any, do not have to be mentioned.
* Fix typo, as needed.
* Fill in details about the pull request, as needed.
* Remove all the directives to skip CI (``[ci skip]`` or ``[skip ci]``).
* Ensure co-authors are properly credited at the bottom of the final message.
* Make sure it is the correct button! Your browser remembers a previous selection even from
a different GitHub repository, which might or might not be the button you want to push.

Use of the 'Squash and merge' button is **encouraged** when you and the
contributor agree that squashing to a single commit is desirable. Using the GitHub
button instead of direct ``git`` commands can reduce overall effort, if done correctly.

You may use 'Squash and merge' in conjunction with :ref:`maintainer-pr-auto-merge`.

.. _maintainer-pr-auto-merge:

Auto-merge
Expand Down

0 comments on commit a7f55f5

Please sign in to comment.