As it's getting a bit exhaustive to repeat these bits on GitHub over and over again,
I though I summarize a couple of notes that hopefully are interesting for the DLang maintainers.

Please take this as a friendly summary and personal advice of most GH-related process improvements that have happened over the last three months. It's (mostly) not intended as a rule book.


There's a (new) formal "Approve" button
=======================================

- GH formalized the review process
- Please use a the "approve" feature instead of LGTM comment

This is important, because all PRs require an approval!
So please approve before you auto-merge (it won't work otherwise).

In the same way GH also allows to attach a "request for changes" on a PR.
-> If you have a serious remark, please use the "request a change" feature instead of a plain comment as these "request" will be nicely shown as warning at the end of a PR (GH will even block the merge of a PR until the criticized bits are fixed). Moreover "changes requested" will also be shown on the summary of all PRs and helps others when they browse the PR list.

Review workflow (squashed commits & write access to PRs)
========================================================

The ideal workflow is that a PR gets commits appended until its final approval, s.t. you only need to review the added changes.

GitHub has two new features to help us here:

1) Commit squashing
-------------------

- All commits get squashed into one commit before the merge
- This is enabled for all DLang repos
- "auto-merge-squash" does squashing as auto-merge behavior

More infos: https://github.com/blog/2141-squash-your-commits

(Before you ask: Yes, CyberShadow had some initial concerns [1] about this feature, but we were able to address them and digger didn't break)

[1] http://forum.dlang.org/post/mciqgandxypjwblexqaf@forum.dlang.org

2) Write access to PRs
----------------------

- This is an _awesome_ feature that hasn't been used much so far
- It allows maintainer to do those nitpicks themselves (squashing all commits, fixing typos, ...) instead of going with the usual ping-pong cycle
- It's enabled by default for new PRs
- If someone turned it accidentally off, it's really okay to ask him/her as this is a massive time saver

I can only recommend to add the following alias to your `~/.gitconfig`:

```
[alias]
  pr  = "!f() { git fetch -fu ${2:-upstream} refs/pull/$1/head:pr/$1 && git checkout pr/$1; }; f"
```

With this you can checkout a PR like this:

> git pr 5150

And in case you are as lazy as I am and don't want to enter the branch to push manually, you can use my small snippet:

```
#!/bin/bash
tmpfile=$(mktemp)
repoSlug=$(git remote -v | grep '^upstream' | head -n1 | perl -lne 's/github.com:?\/?(.*)\/([^.]*)([.]git| )// or next; print $1,"/",$2')
prNumber=$(git rev-parse --abbrev-ref HEAD | cut -d/ -f 2)
curl -s https://api.github.com/repos/${repoSlug}/pulls/${prNumber} > $tmpfile
trap "{ rm -f $tmpfile; }" EXIT
headRef=$(cat $tmpfile | jq -r '.head.ref')
headSlug=$(cat $tmpfile | jq -r '.head.repo.full_name')
git push -f git@github.com:${headSlug} HEAD:${headRef}
```

For more details on pushing changes to a PR, please see this article:

More infos: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@all: I recently discovered the nice pattern to prepend sth. like "[SQUASH]" to appended commit messages. This helps maintainers as well ;-)

Auto-Merge
==========

In case you were wondering about the new "auto-merge" and "auto-merge-squash" labels.
This is the new auto-merge system that takes the status of all required CIs into account (the auto-tester tries the merge after its test passed, but doesn't look out for other CIs.)
You can toggle a PR for auto-merge by simply adding this label or for the keyboard-enthusiasts:
press "l", press "a" and hit enter.

Warning: this new auto-merge system is officially still WIP because:
- "auto-merge"-labelled PRs aren't yet prioritized on the auto-tester [1]
- We would like to set _all_ CIs to enforced

For more details, please see [2]:

[1] https://github.com/dlang-bots/dlang-bot/pull/50
[2] https://github.com/dlang-bots/dlang-bot#auto-merge-wip


CI status
=========

Martin and I have worked quite a bit on making the CIs more reliable. If you see a transient error, please let us know!
In any other case, the "red cross" on a CI has a meaning - surprise!
Here's a small summary of the tasks of the "new" CIs:

1) CircleCi

- Tests are run with code coverage enabled
- Phobos only: Over the last weeks, we have tried to unify Phobos to share a more common coding style. The CI is in place to ensure this status quo for the future
- Phobos only: All unittest blocks are separated from the main file and compiled separately. This helps to ensure that the examples on dlang.org that are runnable and don't miss any imports.

It should be fairly trivial to find out the regarding error here. Just click on the "CircleCi" link and open the red tab that is marked as failing and scroll down to the error message.

2) ProjectTester

- A couple of selected projects are run to ensure that no regressions are introduced
- Martin & Dicebot are currently working on a huge improvement of this system:

Preview: https://ci2.dawg.eu/blue/organizations/jenkins/dlang/detail/dlang/120/pipeline/
More infos: http://forum.dlang.org/post/aacadhgnkodaagwtwstc@forum.dlang.org

As we keep adding CIs it's also planned to move the CircleCi tasks to the new CI system once it's ready:

https://github.com/Dicebot/dlangci/issues/18

Code coverage
=============

If you are too lazy to click on the annotated coverage link, you can install the browser extension which will enrich the PR with code coverage information:

https://github.com/codecov/browser-extension

Unfortunately `codecov/project` is currently not exact as CodeCov doesn't handle the `parent_ref` correctly. For details:

https://github.com/dlang/phobos/pull/5197
https://github.com/dlang/phobos/pull/5198
https://github.com/codecov/support/issues/360

Milestones
==========

- They are intended to show which PRs are basically ready to be shipped OR should be shipped soon
- Please use them whenever you see nothing blocking a PR (except for a final merge decision)
- Ideally about one week before the close of the merge window (i.e. the end of the milestone), the focus on the remaining items of the current milestone should be increased

So far it worked well in the past and present:

2.072: https://github.com/dlang/phobos/milestone/9?closed=1
2.073: https://github.com/dlang/phobos/milestone/11?closed=1
2.074: https://github.com/dlang/phobos/milestone/12

Dlang-Bot
=========

- The bot [1] is getting smarter & smarter every week
- WebDrake and I are working on providing a default greeting message with some useful pointers [2]

What it does atm:
- Shows whether a PR will be part of the changelog ('X' means NO, '✔' means YES)
- Auto-merges a PR once all required CI pass
- Closes a Bugzilla issue if the respective PR has been merged
- Moves a Trello card if the respective PR has been merged
- Cancels stale Travis builds (this helps to free the Travis queue at dlang/dmd)

What is planned for the future:
- Automatically remove "needs work" / "needs rebase" on a push event [3]
- Recognize common labels in the title (e.g. "[WIP]") [4]
- Automatically tag inactive and unmergable PRs [5]
- Add a "needs review" label to unreviewed PRs with passing CIs [6]
- Show auto-detectable warnings (e.g. regression PR that isn't targeted at 'stable') [7]

Some of this features may be subjective. If there's anything you miss in particular or annoys you, please let us know -> [1].

[1] https://github.com/dlang-bots/dlang-bot
[2] https://github.com/dlang-bots/dlang-bot/pull/44
[3] https://github.com/dlang-bots/dlang-bot/pull/51
[4] https://github.com/dlang-bots/dlang-bot/pull/56
[5] https://github.com/dlang-bots/dlang-bot/pull/55
[6] https://github.com/dlang-bots/dlang-bot/pull/52
[7] https://github.com/dlang-bots/dlang-bot/pull/57

Changelog
=========

There's a new changelog format [1] for all three repos (DMD, Druntime, Phobos) in place. This format is a file per changelog entry, which has the advantage that a changelog can be added to a PR _without_ creating merge conflicts.
Hence, take advantage of this feature and don't merge a PR without a changelog entry ;-)
Moreover, DAutoTest will soon show a preview of the generated changelog [2].
Last but not least the separation in the DMD repo between "compiler changes" and "language changes" has been moved, s.t. the "changelog" repo at DMD just contains compiler changes and the changelog at dlang.org [3] is supposed to contain language changes of the upcoming release.

[1] https://github.com/dlang/phobos/tree/master/changelog
[2] https://github.com/dlang/dlang.org/pull/1549
[3] https://github.com/dlang/dlang.org/tree/master/language-changelog

On Sunday, 26 February 2017 at 14:38:03 UTC, Seb wrote:
> [snip]

Excellent, this info will be useful!

> - All commits get squashed into one commit before the merge
> - This is enabled for all DLang repos
> - "auto-merge-squash" does squashing as auto-merge behavior

I especially liked this one. No squashing just for squashing's sake.

On 02/26/2017 09:38 AM, Seb wrote:>
> Review workflow (squashed commits & write access to PRs)

Hoorayyyy for these!!!! *Please* lets make use of these.

Pretty much everything about the old way couldn't have been any better at dissuading regular contributors if it had be DESIGNED to discourage contributions. The back and forth on style or other nitpicks, getting the dreaded "looks good, plz rebase and squash" thus triggering "shit, ok, how the fuck do I 'rebase a pr' again without bugging the reviewer?" And as for squash, well that's just such a badly-documented convoluted mess in git, every single time I've attempted I gave up and found it vastly quicker and easier to just make a fresh branch in a new working directory and file-copy the final versions there. Used to even do a new PR until I learned enough about git to work out force-pushing the fresh new branch to the old PR's remote branch instead of just making a new PR (which contributors really shouldn't freaking have to figure out).

Contributors shouldn't have to know as much about git as a project's maintainers. So these features, if used, are AWESOME.

On Sunday, 26 February 2017 at 14:38:03 UTC, Seb wrote:
> As it's getting a bit exhaustive to repeat these bits on GitHub over and over again,
> I though I summarize a couple of notes that hopefully are interesting for the DLang maintainers.
>
> [...]

This is great, please put this somewhere (the wiki?) where it won't get lost.

On Sunday, 26 February 2017 at 14:38:03 UTC, Seb wrote:
> 1) Commit squashing

Reminder: please only do this only when it makes sense to (one commit with significant changes followed by fixup commits that have no significance on their own). If the PR already has multiple commits split into logical changes, don't squash as it makes bisecting and inspecting history more difficult. If in doubt, do a regular merge.

On 2017-02-28 00:42, Nick Sabalausky (Abscissa) wrote:

> Contributors shouldn't have to know as much about git as a project's
> maintainers. So these features, if used, are AWESOME.

Squashing and rebasing is part of the basic git, in my opinion.

-- 
/Jacob Carlborg

On 02/28/2017 01:07 AM, Vladimir Panteleev wrote:
> On Sunday, 26 February 2017 at 14:38:03 UTC, Seb wrote:
>> 1) Commit squashing
>
> Reminder: please only do this only when it makes sense to (one commit
> with significant changes followed by fixup commits that have no
> significance on their own).

This would be the overwhelmingly frequent case. Please reach to squash commits as the default, unless there is a special case. Thanks.

> If the PR already has multiple commits split
> into logical changes, don't squash as it makes bisecting and inspecting
> history more difficult. If in doubt, do a regular merge.

This indicates a problem with the PR more often than not. Good PRs focus on one issue and one issue only.

Of course there would be exceptions. Rules are not a substitute for judgment.


Thanks,

Andrei

On Tuesday, 28 February 2017 at 13:10:17 UTC, Andrei Alexandrescu wrote:
> This would be the overwhelmingly frequent case.
> ...
> This indicates a problem with the PR more often than not.

It is unfortunate that these two seem to be true right now, so given that, I'll agree with you. Currently a lot of contributed code seems to be placed in unnecessarily large commits with minimalist commit messages, which would rank lather low on the quality scale of large established projects. Ideally changes should be split into as many commits as is reasonable, which by itself brings a lot of benefits, and described in detail (50-char summary, long description of the change, and change rationale). For examples, see the commit message guidelines of Linux, git, or really any project that's large enough to have commit message guidelines.

On 02/28/2017 08:48 AM, Vladimir Panteleev wrote:
> On Tuesday, 28 February 2017 at 13:10:17 UTC, Andrei Alexandrescu wrote:
>> This would be the overwhelmingly frequent case.
>> ...
>> This indicates a problem with the PR more often than not.
>
> It is unfortunate that these two seem to be true right now, so given
> that, I'll agree with you. Currently a lot of contributed code seems to
> be placed in unnecessarily large commits with minimalist commit
> messages, which would rank lather low on the quality scale of large
> established projects. Ideally changes should be split into as many
> commits as is reasonable, which by itself brings a lot of benefits, and
> described in detail (50-char summary, long description of the change,
> and change rationale). For examples, see the commit message guidelines
> of Linux, git, or really any project that's large enough to have commit
> message guidelines.

Thanks. I'd replace "changes should be split into as many commits as is reasonable" with "changes should be split into as many pull requests as is reasonable", which is a natural consequence of "most pull requests should consist of one commit upon merging". (Of course there may be several commits during PR review.)

One vs. several commits per merged pull request is a matter in which reasonable people may disagree, and we can't do both ways. The Foundation fosters that github pull requests are squashed upon merging, with exceptions that need to be justified.

BTW we should put this in our guidelines, we have https://wiki.dlang.org/Contributing_to_Phobos but not an equivalent covering all of dlang properties. Is there one?


Thanks,

Andrei

On Tuesday, 28 February 2017 at 14:52:36 UTC, Andrei Alexandrescu wrote:
> Thanks. I'd replace "changes should be split into as many commits as is reasonable" with "changes should be split into as many pull requests as is reasonable", which is a natural consequence of "most pull requests should consist of one commit upon merging". (Of course there may be several commits during PR review.)

Well... not always. For example, introducing a private function that is not called from anywhere is something that doesn't really make sense as a pull request of its own, but does make sense as a separate commit.

> One vs. several commits per merged pull request is a matter in which reasonable people may disagree, and we can't do both ways. The Foundation fosters that github pull requests are squashed upon merging, with exceptions that need to be justified.

Sorry, but I don't think that's reasonable at all.

I have seen no arguments to support this way of doing things, only downsides. Established major projects seem to agree.

As far as I can see, this is not about a subjective point regarding which reasonable people may disagree. It seems to be a poorly justified mandate, that's all.

As I've mentioned previously, you will need to provide some arguments which would outweigh those supporting the opposite position.

> The Foundation fosters

IMHO, this phrase does not belong in technical discussions.