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

Revamp the contributor guide #473

Merged

Conversation

greg0ire
Copy link
Member

While some sections feel redundant, other sections were missing, like
the one about choosing the right branch.

This only addresses the first question of #472

@mpdude please review.

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

I'd like to see more reviewers for this.

source/contribute.rst Outdated Show resolved Hide resolved
@mpdude
Copy link

mpdude commented Jan 24, 2023

I‘ll have to review the branch selection rules later, with a clear mind.

In general, I'd say there is a lot of information in this chapter that should be routine for people familiar with git.

Do we really need to provide all that? Do we anticipate contributions from users who haven’t done all that before?

@greg0ire greg0ire force-pushed the how-to-choose-a-branch-improvements branch 2 times, most recently from db7abfa to a10c250 Compare January 24, 2023 22:52
@greg0ire
Copy link
Member Author

Do we really need to provide all that? Do we anticipate contributions from users who haven’t done all that before?

There's indeed pretty basic stuff, so we could remove more than I already did. That being said, here are the most common issues I see:

  • people who develop directly on 2.3.x instead of creating a topic branch, and are then are asked to retarget on 2.4.x
  • people who don't know they can force push and instead close their PR and open a new one when asked to retarget their branch
  • bad commit messages (typically fix for #1234, where they don't explain what they did and how that's supposed to fix anything)
  • good commit message body, and commit message subject as suggested by github, so typically "Update path/to/file.php"
  • non-atomic commits, with unexplained changed mixed with changes relating to the main topic of the pull request
  • PR that passes the build, but with commits that don't pass the build individually
  • commits that don't bring extra understanding of what's being done but rather show the struggle ("fix cs")

@greg0ire
Copy link
Member Author

greg0ire commented Mar 6, 2023

ping @mpdude 🙂

source/contribute.rst Outdated Show resolved Hide resolved
greg0ire added 2 commits March 6, 2023 23:30
I personally use origin for the official repository. Let us avoid
"origin" entirely, as it might lead to some confusion.
Not everyone wants to contribute directly, some people might want to toy
with the repository before deciding to fork.
@mpdude
Copy link

mpdude commented Mar 7, 2023

Still have to go through the textual changes.

Regarding the other points you brought up:

When we ask people to re-target PRs, can we expect/assume they find these instructions?

Would it help if we set up a canned reply in GitHub, so that when we ask people to change the target branch we could include a pointer to this documentation?

Regarding bad commit messages, meaningless commits etc.: Would it make sense to discuss squash-merging commits like Symfony does? That would help to hide “how the sausage is being made” and have a clean history with one commit per change (= PR).

source/contribute.rst Outdated Show resolved Hide resolved
@greg0ire greg0ire force-pushed the how-to-choose-a-branch-improvements branch 2 times, most recently from 1515a5e to 7bc40bc Compare March 7, 2023 22:26
source/contribute.rst Show resolved Hide resolved
source/contribute.rst Outdated Show resolved Hide resolved

The general rules are as follows:

- Patch releases should be as stable as possible. They fix bugs, but in
Copy link

Choose a reason for hiding this comment

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

typo: "in"

Comment on lines 94 to 108
If you are touching something outside the source directory (usually
``src``, sometimes ``lib``), and that thing is not ``composer.json``,
you should probably target the lowest branch.
There is no way that your contribution is going to create a bug for
the end user, so let us get it on all 3 branches with merges up.
Examples are: documentation, tests, build scripts, tooling configuration.

If you are touching something inside the source directory, you may
still contribute to the lowest branch if it is a bug fix, and if you
are touching comments that are not phpdoc comments.

If the comments are phpdoc comments… it depends. If the phpdoc was
inaccurate, you should target the lowest branch. If the phpdoc was
imprecise but technically correct, you should target the next minor
branch.
Copy link

Choose a reason for hiding this comment

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

Things that can go on the patch release branch include:

  • Bugfixes 🥹
  • Adding tests, especially for bugs fixed
  • Updates, corrections or improvements to non-code assets like documentation, build scripts or tooling configuration
  • Updates, corrections or improvements to code comments that are not phpdoc comments (docblock type hints etc.)
  • Fixes to incorrect phpdoc comments
  • When phpdoc comments are imprecise but not wrong technically, target the next minor release branch instead.

Comment on lines 110 to 120
If you are doing a refactoring and you think you didn't change the
behavior of the code, you should still target the next minor branch
unless the refactoring is necessary for a bugfix. Let us not take any
unnecessary risks.

If you want to deprecate something, you should target the next minor
branch: upgrading to a new patch version should not result in any
additional deprecations.

If you want to add a new feature, you should target the next minor
branch, because it might have unintentional side effects that break existing code.
Copy link

Choose a reason for hiding this comment

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

The next minor version branch may include:

  • Refactorings, unless they are necessary for bugfixes. This is to avoid unnecessary risks.
  • Adding deprecations – remember to update the UPGRADE.md file as well!
  • Adding new features and/or public APIs

Copy link

Choose a reason for hiding this comment

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

What about changing minimum dependency requirements?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing minimum dependency requirements is OK on a patch branch if it's absolutely necessary to fix a bug. I don't think that's often the case, and I think we don't have to mention this here.

Comment on lines 122 to 126
If you want to remove or change a public API, you should target the next
major branch. If you change a public API, you should provide an upgrade
path for the end user. Ideally, you should make the new API available on
the next minor branch, deprecate the old API, and remove it in the next
major.
Copy link

Choose a reason for hiding this comment

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

On the next major version branch, usually only deprecations notices, deprecated features and necessary compatibility layers are being removed.

Avoid suprising changes in public APIs on the next major version branch if anyhow possible. In order to provide a smooth upgrade path to users, the preferred solution is to add deprecations and possibly alternative APIs in a preceding minor version, and only remove the deprecations and compatibility layers in the next major release.

source/contribute.rst Show resolved Hide resolved
Comment on lines 322 to 402
1. Fetch all new commits: `git fetch doctrine``.
2. Rebase on what you fetched interactively:
``git rebase --interactive doctrine/2.3.x # use -i for short``
3. A window will show up with many lines, remove every line but the last
ones, which should correspond to your commits.
4. If you run into a conflict, fix it with ``git mergetool``, then
continue on your merry way with ``git rebase --continue``.
5. Force push to overwrite the previous version : ``git push --force``.
Copy link

Choose a reason for hiding this comment

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

Rebases will lose approvals. Just sayin'.

Copy link
Member Author

Choose a reason for hiding this comment

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

The paragraph is "typically because the build had an issue", and I personally don't approve pull requests that have a red build.

@greg0ire greg0ire force-pushed the how-to-choose-a-branch-improvements branch from 7bc40bc to f06ca12 Compare March 8, 2023 07:55
@greg0ire greg0ire requested a review from mpdude March 8, 2023 19:50
must be available in the minor release that adds the deprecation for
the old feature.

With that in mind:
Copy link

Choose a reason for hiding this comment

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

Here we have two sentences that end with a colon

about that
<https://docs.github.com/en/get-started/quickstart/contributing-to-projects>`_
first.

Copy link

Choose a reason for hiding this comment

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

When following that documentation, you'll end up with a clone of your fork, so origin refers to that. Also, in these instructions, only the default branch from the base repo will be included in the clone.

So, maybe we should add instructions here to add doctrine as a second remote pointing to the upstream (original) Doctrine repo, and to pull all branches from there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or to rename it… should be less confusing.

Copy link

Choose a reason for hiding this comment

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

Remember, we are writing this mostly for an audience with little to no experience in contributing on GitHub. So we should try to leave them with a setup that is as “mainstream” as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know that there is a mainstream setup. Some people use origin for their forks, some other for the upstream repository… I don't have statistics on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Or to rename it… should be less confusing.

I think I misunderstood this, I thought you wanted to rename origin to something else.

I agree, there are people who clone the original repo and add their fork as a second remote, or they clone their fork and add upstream or similar.

Either way, wouldn't it be safe if we suggest to add a doctrine remote pointing to the original Doctrine project's repo, and use that remote name in all other git commands?

Copy link

Choose a reason for hiding this comment

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

I just see that in fact, you do that – rename origin to doctrine. That may be wrong, depending on how people set up their clone. And if they follow the GitHub instruction linked above, it will definetly be wrong, since those instructions end up with origin= the fork.

Copy link

Choose a reason for hiding this comment

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

@greg0ire I think this point still needs to be addressed:

Simply renaming origin to doctrine may not be correct. Depending on how people obtained their clone, origin may be their fork or the original (upstream) repo.

When closely following the instructions linked in the previous sentence, then origin will be the fork, so the rename suggested here is definetly wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I addressed it now. I suggested starting afresh with a git remote rm

Comment on lines 131 to 171
Time might have elapsed since the last time you contributed or since you
cloned the repository. You might want to fetch the latest changes from
the ``doctrine`` remote:

.. code-block:: console

$ git fetch doctrine
Copy link

Choose a reason for hiding this comment

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

Let's move that a bit down

Copy link
Member Author

Choose a reason for hiding this comment

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

Disagree, see #473 (comment)

-------------------------

New pull requests are created with the repository's default branch as
base branch, and that might not be what you want. Make sure to pick the
Copy link

Choose a reason for hiding this comment

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

The base branch should be what you chose according to the guidelines mentioned in the beginning of this chapter. It is also the branch name that you provided when you created your topic branch.

New pull requests are created with the repository's default branch as
base branch, and that might not be what you want. Make sure to pick the
correct branch when creating the pull request. If you do not, it can
still be changed afterwards.
Copy link

Choose a reason for hiding this comment

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

That'a a bit hidden – on GitHub, you have to click "Edit" next to the pull request title to change the title and base branch

.. code-block:: console
Sometimes, you will need to rebase your branch on the latest changes,
typically because the build had an issue unrelated to your changes, and
that issue has been fixed after you created your branch.
Copy link

@mpdude mpdude Mar 9, 2023

Choose a reason for hiding this comment

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

Basically, a ebase takes all changes on your topic branch and moves them to another starting point. This starting point was the Doctrine branch that you chose when you created your topic branch, at that point in time. The rebase will move your changes to be based on the current state of this branch.

Copy link

Choose a reason for hiding this comment

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

Comment on lines 323 to 324
3. A window will show up with many lines, remove every line but the last
ones, which should correspond to your commits.
Copy link

Choose a reason for hiding this comment

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

Hm, that's pretty vague, and every line removed potentially is a lost commit. A nice footgun, eh?

As long as you're only rebasing on the same branch that you initially started out with, does it have to be an interactive rebase at all?

Copy link

Choose a reason for hiding this comment

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

(Assuming we're writing all this for people rather new to git and contributing on GitHub, so we should not talk them into complicated situations where they might lose work and/or get frustrated)

Copy link
Member Author

Choose a reason for hiding this comment

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

As long as you're only rebasing on the same branch that you initially started out with, does it have to be an interactive rebase at all?

That's the thing, newbies will often pick the wrong branch, and might have to rebase on another branch, and commit they don't own. I'll reword with "commits that you did not author"

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah but the paragraph is not about this use case. You're right, let's make this non-interactive, I've been mixing things up.

Copy link

Choose a reason for hiding this comment

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

Cool

Copy link

Choose a reason for hiding this comment

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

Let’s have a dedicated section about changing the target branch and the rebase operations that are necessary for this.

Additionally, prepare a canned reply that we can use in PRs whenever we ask people to change the target branch. This should point to that documentation, mention the correct branch and tell people that they need not close the PR but can update it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed.

Copy link

@mpdude mpdude left a comment

Choose a reason for hiding this comment

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

IMHO this is already in a very good shape, keep going!

Added a next round of feedback – sorry I missed some of the points last time.

@mpdude
Copy link

mpdude commented Mar 9, 2023

Regarding #473 (comment)

  • bad commit messages (typically fix for #1234, where they don't explain what they did and how that's supposed to fix anything)
  • good commit message body, and commit message subject as suggested by github, so typically "Update path/to/file.php"
  • non-atomic commits, with unexplained changed mixed with changes relating to the main topic of the pull request
  • PR that passes the build, but with commits that don't pass the build individually
  • commits that don't bring extra understanding of what's being done but rather show the struggle ("fix cs")

IMHO squash-merging PRs could solve all those problems. We could also ask contributors to keep the initial PR description an up-to-date description and summary of the change and use that as the commit message for the merge. Symfony does it that way as well.

Where and how could this best be discussed with the relevant decision-makers from the Doctrine core team?

Comment on lines +270 to +277
The default branch is the branch you see when you enter the repository
page on GitHub.

.. image:: ../images/default-branch.png
:alt: The default branch
:style: margin-bottom: 20px

In this DBAL example, it's the branch with the name **2.11.x**. The
branch name reflects the current lowest supported version of a
repository.
Copy link

Choose a reason for hiding this comment

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

Relevant information?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you asking if this sentence is relevant, or do you want me to add relevant information?

Copy link

Choose a reason for hiding this comment

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


Sync your local 2.11.x branch:
1. Fetch all new commits: `git fetch doctrine``.
Copy link

Choose a reason for hiding this comment

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

  1. Switch to the branch you'd like to rebase.

1. Fetch all new commits: `git fetch doctrine``.
2. Rebase on what you fetched:
``git rebase doctrine/2.3.x``
3. If you run into a conflict, fix it with ``git mergetool``, then
Copy link

Choose a reason for hiding this comment

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

Never used git mergetool – does that include git add for the resolved conflicts?

Copy link
Member Author

@greg0ire greg0ire Mar 9, 2023

Choose a reason for hiding this comment

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

Yes. I'll try to rework this.

against ``2.3.x`` but you are told to change it to ``2.4.x``. In that
case, if ``2.4.x`` does not have all the commits that ``2.3.x`` has, you
need to drop these commits while rebasing: it is not your responsibility
to merge them up.
Copy link

Choose a reason for hiding this comment

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

Good point, I was not aware of that!

Copy link

@mpdude mpdude Mar 9, 2023

Choose a reason for hiding this comment

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

Wait, subconsciously I was. 

git checkout my-topic
git rebase doctrine/2.3.x --onto doctrine/2.4.x

No need to pick the right commits. Am I missing something? 

I should read your full changes before jumping in, you've mentioned this below.

Isn't that much easier and safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're probably right, let's just suggest that one method.

Copy link

@mpdude mpdude Apr 14, 2023

Choose a reason for hiding this comment

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

When using that method, the "commits you did not author" thingy is not a issue at all, right? If so, we need not mention it.


Another case where you need a rebase is when you want to change the
target branch of your PR. For instance, you might have created your PR
against 2.3.x but you are told to change it to 2.4.x. In that
case, the following command will pick all changes that you made against the 2.3.x branch, and re-apply them on the current 2.4.x branch.

$ git fetch doctrine
$ git rebase --onto doctrine/2.4.x doctrine/2.3.x your-topic-branch
$ git push --force

After that, you also need to update the GitHub pull request to point to the new target branch. You can do so by clicking on the "Edit" button next to the pull request title. After you changed the target branch, the pull request should only show your commits and changes, but this time they are based on the new target branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

When using that method, the "commits you did not author" thingy is not a issue at all, right? If so, we need not mention it.

Not an issue indeed 👍

``rebase --continue``. At any stage, you can abort the rebase with
``rebase --abort`` unlike nasty merges which will leave files strewn
everywhere.
On upstream changes
Copy link

Choose a reason for hiding this comment

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

"on" or "onto"? Not a native speaker myself. Git CLI suggests onto.

Copy link
Member Author

Choose a reason for hiding this comment

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

From man git rebase: "it may not be the best idea to keep rebasing on top of the upstream"

``drop``.
4. If you run into a conflict, fix it with ``git mergetool``, then
continue on your merry way with ``git rebase --continue``.
5. Force push to overwrite the previous version : ``git push --force``.

Copy link

Choose a reason for hiding this comment

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

  1. Update the Pull Request target in GitHub

Copy link
Member Author

Choose a reason for hiding this comment

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

Already mentioned above (and I think it's best to do it before pushing)

@mpdude
Copy link

mpdude commented Mar 9, 2023

Sorry for all the nitpicking, I owe you a beer once we happen to meet IRL

@greg0ire greg0ire force-pushed the how-to-choose-a-branch-improvements branch from e4bfc57 to 34c23d0 Compare March 9, 2023 15:27
@greg0ire
Copy link
Member Author

greg0ire commented Mar 9, 2023

Can't wait for the next EU-FOSSA hackathon!

@mpdude
Copy link

mpdude commented Mar 9, 2023

Oh good ol' times

@greg0ire
Copy link
Member Author

greg0ire commented Apr 13, 2023

@mpdude IMO this is ready, can I get a formal approval from you? @SenseException , would it be sufficient for you or do you wish to get other maintainers to review this? I'd hate to let this rot…

@SenseException
Copy link
Member

Yes, @mpdude fits perfectly as second reviewer. His approval should make this getting merged. 👍

Comment on lines 50 to 65
When cloning, you will end up with a remote called ``origin``. This can
be confusing because sometimes, people use ``origin`` to refer to their
fork.

To avoid confusion, we recommend you to rename the ``origin`` remote to
``doctrine``:

.. code-block:: console

$ git clone [email protected]:username/dbal.git
$ git remote rename origin doctrine

- Enter the dbal directory and add the **doctrine** remote
Also, please ensure your Github fork is known as ``fork`` locally:

.. code-block:: console

$ cd dbal
$ git remote add doctrine git://github.com/doctrine/dbal.git
$ git remote add fork [email protected]:<your-name-here>/orm.git
Copy link

@mpdude mpdude Apr 14, 2023

Choose a reason for hiding this comment

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

Suggestion for this entire section:

To make the example commands given in the rest of this page less ambiguous, we will add two explicit names for remote Git repositories: `doctrine` for the original Doctrine project you're contributing to, and `fork` for your fork of it.

.. code-block:: console
    $ [`cd` into the directory created by `git clone ...`]
    $ git remote add doctrine [email protected]:doctrine/<name-of-project>.git
    $ git remote add fork [email protected]:<your-github-name>/<name-of-project>.git

Comment on lines 170 to 173
.. note::

Here, we assume ``doctrine`` points to the upstream repository, and
not your fork. If you are not sure, you can check with ``git remote -v``.
Copy link

@mpdude mpdude Apr 14, 2023

Choose a reason for hiding this comment

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

We don't need this note since we've added the remote name in the beginning section.

"Remember, we assume that doctrine points to the original Doctrine project repository and not to your fork, as described in the initial section of this page. If unsure, you can check with git remote -v.

Finished topic branches should be pushed to **origin** for a
**maintainer** to review and pull into **doctrine** as appropriate:
1. Switch to the branch you would like to rebase.
2. Fetch all new commits: `git fetch doctrine``.
Copy link

Choose a reason for hiding this comment

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

Is this missing a backtick?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is 👍

@mpdude
Copy link

mpdude commented Apr 14, 2023

@greg0ire Done. Can you easily follow up on the new comments I made? Otherwise, I'll walk you through them one by one.

Not much left – let's push this over the finish line soon!

@greg0ire greg0ire force-pushed the how-to-choose-a-branch-improvements branch from 34c23d0 to 4c9c51a Compare April 14, 2023 10:13
@greg0ire
Copy link
Member Author

Can you easily follow up on the new comments I made? Otherwise, I'll walk you through them one by one.

I think I did, but I also think I might have easily missed some comments. Can you mark the resolved ones as such?

@mpdude
Copy link

mpdude commented Apr 14, 2023

For some reason, I cannot mark (everything) as "resolved" 🤔 .

Let's see:

@greg0ire greg0ire force-pushed the how-to-choose-a-branch-improvements branch from 4c9c51a to f75f309 Compare April 14, 2023 13:25
date with the following command:
To make the example commands given in the rest of this page less
ambiguous, we will add two explicit names for remote Git repositories:
`doctrine` for the original Doctrine project you're contributing to, and
Copy link

Choose a reason for hiding this comment

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

.rst need double backticks here and in the following lines?

While some sections feel redundant, other sections were missing, like
the one about choosing the right branch.
@greg0ire greg0ire force-pushed the how-to-choose-a-branch-improvements branch from f75f309 to 7302b61 Compare April 14, 2023 13:30
@mpdude
Copy link

mpdude commented Apr 14, 2023

All comments addressed ✅ .

Awesome work @greg0ire 🥇 and sorry it took so many iterations. I really like the result!

@greg0ire greg0ire merged commit 0c9ba9c into doctrine:master Apr 14, 2023
@greg0ire greg0ire deleted the how-to-choose-a-branch-improvements branch April 14, 2023 13:43
@greg0ire
Copy link
Member Author

Thanks for the thorough review!

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.

3 participants