www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Notes for DLang maintainers

reply Seb <seb wilzba.ch> writes:
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:

```

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
Feb 26 2017
next sibling parent Dukc <ajieskola gmail.com> writes:
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.
Feb 27 2017
prev sibling next sibling parent reply "Nick Sabalausky (Abscissa)" <SeeWebsiteToContactMe semitwist.com> writes:
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.
Feb 27 2017
parent reply Jacob Carlborg <doob me.com> writes:
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
Feb 27 2017
parent reply "Nick Sabalausky (Abscissa)" <SeeWebsiteToContactMe semitwist.com> writes:
On 02/28/2017 02:37 AM, Jacob Carlborg wrote:

 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.


Maybe they should be, but with the basic git interface, or any front-end 
I've seen, they're terribly convoluted. Particularly squashing. Well, 
either that, or the docs are just really, REALLY bad.

There's no reason either one of those operations couldn't/shouldn't be a 
(*simple*) one-line command, and yet, they just...aren't. But then, 
that's git :/ (And no, rebasing a PR is NOT a one-line git command, and 
no, add-on scripts don't count towards usability.)

Actually, about a week ago, I finally got around to staring a lib/cli 
front-end for git (github support planned, too) to make everything sane. 
Too early for anything public though, a lot still incomplete, a lot that 
may still be in flux.
Feb 28 2017
next sibling parent reply "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Tue, Feb 28, 2017 at 11:37:45AM -0500, Nick Sabalausky (Abscissa) via
Digitalmars-d wrote:

 On 02/28/2017 02:37 AM, Jacob Carlborg wrote:

 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.
 


 
 Maybe they should be, but with the basic git interface, or any
 front-end I've seen, they're terribly convoluted. Particularly
 squashing. Well, either that, or the docs are just really, REALLY bad.


[...]

Whut...?  What can be easier than:

	git checkout my_bug_fix
	git rebase -i master


	git push -f origin



 There's no reason either one of those operations couldn't/shouldn't be
 a (*simple*) one-line command, and yet, they just...aren't. But then,
 that's git :/ (And no, rebasing a PR is NOT a one-line git command,
 and no, add-on scripts don't count towards usability.)


"Add-on scripts" are just user-defined functions.  Is it really that bad
to expect programmers to be able to write their own functions as opposed
to expecting the standard lib to spoonfeed them for every little
conceivable task they might possibly need to accomplish?

(Having said that, though, I agree that your standard average *nix shell
interface is really dumb, and being forced to write scripts in that
broken language can be painful.  One of these days I should write a
shell with D-like syntax that doesn't require memorizing meaningless
30-year-old typographical infelicities like 'grep'.)



 Actually, about a week ago, I finally got around to staring a lib/cli
 front-end for git (github support planned, too) to make everything
 sane. Too early for anything public though, a lot still incomplete, a
 lot that may still be in flux.


Curious to see what you come up with.  AIUI, git is already providing
the 'porcelain' interface by default, so we see today is already far
better than what Linus used to use. (I dread to imagine how one might
use git back then!  I'll bet it's like piloting an airplane by coding in
machine language in real-time. :-P)  Let's see if you can come up with
something even better! ;-)   (Though I'll admit it may be easier than it
sounds. But still... the only sane way to use git is to understand its
innards -- y'know, all that DAG stuff -- 'cos it simply doesn't make any
sense otherwise.)


T

-- 
Don't modify spaghetti code unless you can eat the consequences.
Feb 28 2017
parent "Nick Sabalausky (Abscissa)" <SeeWebsiteToContactMe semitwist.com> writes:
On 02/28/2017 12:18 PM, H. S. Teoh via Digitalmars-d wrote:

 Whut...?  What can be easier than:

 	git checkout my_bug_fix
 	git rebase -i master


 	git push -f origin


1. wit squash my_bug_fix
(Or something along those lines. Not yet implemented.)

2. Not needing to intuit that process from the awful docs and Sheldon 
Cooper-esque web articles: 
<http://www.youtube.com/watch?v=AEIn3T6nDAo&t=1m12s>

Your instruction right up there, honestly, is the first time I've *ever* 
come across any instructions or explanation for "How do I squash my pr" 
that actually makes any straightforward sense.


 There's no reason either one of those operations couldn't/shouldn't be
 a (*simple*) one-line command, and yet, they just...aren't. But then,
 that's git :/ (And no, rebasing a PR is NOT a one-line git command,
 and no, add-on scripts don't count towards usability.)


 "Add-on scripts" are just user-defined functions.  Is it really that bad
 to expect programmers to be able to write their own functions as opposed
 to expecting the standard lib to spoonfeed them for every little
 conceivable task they might possibly need to accomplish?


When it's a basic common task, then yes.

When it's something that's expected of project contributors, then again, 
yes. (Paths of resistance are a great way to discourage contribution and 
work against the OSS ecosystem.)


 (Having said that, though, I agree that your standard average *nix shell
 interface is really dumb, and being forced to write scripts in that
 broken language can be painful.  One of these days I should write a
 shell with D-like syntax that doesn't require memorizing meaningless
 30-year-old typographical infelicities like 'grep'.)


Meh, I just use D anymore for any non-trivial scripts ;)


 Actually, about a week ago, I finally got around to staring a lib/cli
 front-end for git (github support planned, too) to make everything
 sane. Too early for anything public though, a lot still incomplete, a
 lot that may still be in flux.


 Curious to see what you come up with.  AIUI, git is already providing
 the 'porcelain' interface by default, so we see today is already far
 better than what Linus used to use. (I dread to imagine how one might
 use git back then!  I'll bet it's like piloting an airplane by coding in
 machine language in real-time. :-P)  Let's see if you can come up with
 something even better! ;-)   (Though I'll admit it may be easier than it
 sounds. But still... the only sane way to use git is to understand its
 innards -- y'know, all that DAG stuff -- 'cos it simply doesn't make any
 sense otherwise.)


I highly suspect how much people like my tool will have a strong inverse 
correlation to how well they understand git (and that's fine - if you're 
good with git, then git gets the job done, no problem.)

TBH though, the issues I've had understanding and using git have had far 
less to do with any DAG-iness of git[1], and more with:

A. The details of the CLI just plain being a mess (inconsistencies, 
questionable defaults, reinventing alternate meanings for established 
VCS terminology, etc)

B. Too much catering to "invent your own workflow!" when most users just 
need "Here's best-practice workflow" and tools appropriate for it.

C. Docs that assume more in-depth knowledge than is really necessary, 
and howto's that can't get straight to the point without throwing a 
bunch of blather at you first (again, the Sheldon Cooper teaching style.)

[1] For example, I've never really had any issue dealing with git's 
branches. No 11-tentacle monsters or anything. But then, I don't go 
branch-crazy either (does anyone?): I have "master" and then I have a 
branch for each PR (if I'm submitting PRs). And then once in a rare 
while, if I'm trying something big and experimental, I'll have a branch 
for that experiment.
Feb 28 2017
prev sibling next sibling parent Russel Winder via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Tue, 2017-02-28 at 09:18 -0800, H. S. Teoh via Digitalmars-d wrote:

 [=E2=80=A6]



 30-year-old typographical infelicities like 'grep'.)


Surely typing grep is far easier than get_regular_expression_print

=20


[=E2=80=A6]

 Curious to see what you come up with.=C2=A0=C2=A0AIUI, git is already pro=


viding

 the 'porcelain' interface by default, so we see today is already far
 better than what Linus used to use. (I dread to imagine how one might
 use git back then!=C2=A0=C2=A0I'll bet it's like piloting an airplane by =


coding

 in
 machine language in real-time. :-P)=C2=A0=C2=A0Let's see if you can come =


up

 with
 something even better! ;-)=C2=A0=C2=A0=C2=A0(Though I'll admit it may be =


easier than

 it
 sounds. But still... the only sane way to use git is to understand
 its
 innards -- y'know, all that DAG stuff -- 'cos it simply doesn't make
 any
 sense otherwise.)


Despite 10 years of trying to make things better the Git command line
is still dreadful and the perpetrators should apologies to the world.
Sadly Mercurial and Bazaar have lost (thought they are still used), and
Git is the winner. Programmers just have to use Git. Despite being crap
the Git command line can just about be mastered enough to be used, and
programmers should just do that.

Blogs showing cute tricks with Git, or anything involving ^^^^^^^HEAD
should be totally ignored by all right thinking people. The trick is to
use the simple stuff, use it well, and ignore all the tricks, and all
the people getting high on cute tricks.

But I rant.

--=20
Russel.
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D
Dr Russel Winder      t: +44 20 7585 2200   voip: sip:russel.winder ekiga.n=
et
41 Buckmaster Road    m: +44 7770 465 077   xmpp: russel winder.org.uk
London SW11 1EN, UK   w: www.russel.org.uk  skype: russel_winder
Feb 28 2017
prev sibling next sibling parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Tue, Feb 28, 2017 at 06:24:45PM +0000, Russel Winder via Digitalmars-d wrote:

 On Tue, 2017-02-28 at 09:18 -0800, H. S. Teoh via Digitalmars-d wrote:

 […]


 

 30-year-old typographical infelicities like 'grep'.)


 
 Surely typing grep is far easier than get_regular_expression_print


It only makes sense to those who already know what it means.  A typical
syndrome of UI's designed by programmers.

It should have been something more immediately descriptive, like 'find'
or 'match'.  But it's too late for that now, as 'find' now has a
different meaning thanks to yet another historical accident.

And don't get me started on `awk` or `sed`. Or `getty`.  As they say,
ASCII stupid question, getty stupid ANSI. :-P


[…]

 Curious to see what you come up with.  AIUI, git is already
 providing the 'porcelain' interface by default, so we see today is
 already far better than what Linus used to use. (I dread to imagine
 how one might use git back then!  I'll bet it's like piloting an
 airplane by coding in machine language in real-time. :-P)  Let's see
 if you can come up with something even better! ;-)   (Though I'll
 admit it may be easier than it sounds. But still... the only sane
 way to use git is to understand its innards -- y'know, all that DAG
 stuff -- 'cos it simply doesn't make any sense otherwise.)


 
 Despite 10 years of trying to make things better the Git command line
 is still dreadful and the perpetrators should apologies to the world.
 Sadly Mercurial and Bazaar have lost (thought they are still used), and
 Git is the winner. Programmers just have to use Git. Despite being crap
 the Git command line can just about be mastered enough to be used, and
 programmers should just do that.


I make no defense for the weirdness of git's current interface.  There's
a lot of asymmetry and unexpected exceptions, that only make sense in
retrospect (and sometimes not even that). Such as `git checkout -b` for
creating a new branch, but `git branch -d` for deleting a branch. Or
`git checkout` for *both* replacing a file with the last committed
version and for switching branches.

But that's what you get for an interface that's evolved rather than
designed.



 Blogs showing cute tricks with Git, or anything involving ^^^^^^^HEAD
 should be totally ignored by all right thinking people. The trick is
 to use the simple stuff, use it well, and ignore all the tricks, and
 all the people getting high on cute tricks.


[...]

Actually, all expectation of git being anything even remotely resembling
a traditional version control system is not thinking right. The only
sane way to make any sense of it whatsoever is to think on *its* terms,
i.e., think in terms of DAG manipulations.  Trying to impose (or think
in terms of) foreign concepts inherited from traditional VCS's
inevitably leads to frustration, annoyance, and hair loss.

For instance, the only way git "branches" even begin to make any sense
at all is when you think of them as mere pointers to certain DAG nodes,
rather than branches in the traditional sense of the word.  I.e., as
pure DAG manipulations and nothing more. Attaching any further meaning
to it which isn't there only leads to confusion and incomprehension.
(And usually also hair/code loss or your DAG turning into an
11-tentacled monster on you.)

In that sense, it's a *good* thing the git interface is so weird: it
forces you to confront the fact that you're dealing not with a VCS (in
the traditional sense of the word), but with a DAG manipulation system.
One that happens to have useful idioms that resembles a VCS of sorts,
but nonetheless at its heart still a mere DAG manipulation system.
Trying to use it as anything else or in any other way amounts to nothing
more than a cargo cult, with about the same level of effectiveness.


T

-- 
Turning your clock 15 minutes ahead won't cure lateness---you're just making
time go faster!
Feb 28 2017
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2017-02-28 17:37, Nick Sabalausky (Abscissa) wrote:


 Maybe they should be, but with the basic git interface, or any front-end
 I've seen, they're terribly convoluted. Particularly squashing. Well,
 either that, or the docs are just really, REALLY bad.

 There's no reason either one of those operations couldn't/shouldn't be a
 (*simple*) one-line command, and yet, they just...aren't.


When I google "git squash commits", the first hit is this page [1]. That 
site basically explains what H. S. Teoh said [2] with some more words 
and an example. Next hit is this one [3], which again have the basically 
the same command, just with a more fleshed out explanation of how it 
works. Not sure where you have been reading.

[1] https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/

[2] 
http://forum.dlang.org/post/mailman.782.1488302816.31550.digitalmars-d puremagic.com

[3] 
http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

-- 
/Jacob Carlborg
Feb 28 2017
prev sibling next sibling parent Nicholas Wilson <iamthewilsonator hotmail.com> writes:
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.
Feb 27 2017
prev sibling parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
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.
Feb 27 2017
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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
Feb 28 2017
parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
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.
Feb 28 2017
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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
Feb 28 2017
next sibling parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
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.
Feb 28 2017
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 02/28/2017 10:04 AM, Vladimir Panteleev wrote:

 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.


There can be any amount of discussion about this, to no conclusive 
results, and any argument may be responded with "I don't buy that". 
That's simply because, again, there's some subjective factor and there 
is no perfect approach that proves the others wrong. To wit, I have 
stated my argument several times in public and private but somehow it 
did not count.

Here we are simply applying a way of doing things that has worked at 
Facebook over six years and two orders of magnitude increase in team 
size. (They use phabricator there, which has a similar workflow.) It is 
obvious to me that other organizations use similar approaches; and also 
that other organizations use different approaches, also with good 
results. I should add that Facebook is one of the most successful 
software companies in history, and that may count for something.

Also, obviously the Internet can be used to find support for anything, 
and with that caveat allow me:

* "Why does every serious Github repo I do pull requests for want me to 
squash my commits into a single commit?" 
http://softwareengineering.stackexchange.com/questions/263164/why-squash-git-commits-for-pull-requests

* "I’ve been contributing to more projects in which it is expected that 
I should submit a pull request that contains a single commit." 
http://ndlib.github.io/practices/one-commit-per-pull-request/

* "Squashing your branch's changes into one commit is "good form" and 
helps the person merging your request to see everything that is going 
on." https://github.com/projecthydra/hydra-head/blob/master/CONTRIBUTING.md

* "But one thing occasionally bothers me, and that's pull requests that 
come loaded with several temporary commits. I often find myself asking 
contributors to squash those into a single descriptive commit." 
http://eli.thegreenplace.net/2014/02/19/squashing-github-pull-requests-into-a-single-commit

I'll stop here. Sure enough, there'd be other links to pages describing 
how many commits per PR work fine. Our organization happens do things 
this particular way, and it's not the only one. Moreover, we are are not 
rigidly enforcing it; within reason, definitely leeway exists.

Vladimir, I am gathering that this makes you unhappy and are close to 
getting ultimative about this. I have made all attempts I could to 
publicly and privately explain matters. I kindly ask you to consider the 
following:

* Any organization, be it for profit (company) or not (university, oss 
project) has certain rules in place. In all likelihood not all 
participants would choose the exact same rules if they could, yet they 
choose to walk (factually or virtually) in there every day and 
contribute to the organization. A simple way to look at things would be, 
would you quit a job or start a protracted argument with the CEO if the 
coding guidelines contained "please squash your github commits"?

* The leader of an organization (in this case me) cannot afford the time 
to justify and debate every single minor decision they make. No 
organization works that way. I am under enormous pressure to bring money 
in the Foundation, organize DConf, mentor students, write articles, 
contribute code, all while keeping things running.

* You seem to frame things as an arbitrary imposition made by an 
oppressive leadership. Think of it - ever since we have worked together 
(your first post is dated 2007-01-06!), is this the pattern by which 
matters have been conducted? Do you think it would be a fair 
characterization of Walter and my philosophy and values?

The short of it is we simply cannot work this way, as a simple matter of 
my own human limitations; I can't spend hours in the forum justifying 
every little thing, especially those that ten people do in eleven ways. 
I mean I _literally_ can't: I don't have the time because I am spending 
it making the D language more successful. Which I hope you'll help me 
with, too. To scale, we need to focus on the real problems and abandon 
debating the small things again and again.



Thanks,

Andrei
Feb 28 2017
parent Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Tuesday, 28 February 2017 at 15:49:56 UTC, Andrei Alexandrescu 
wrote:

 There can be any amount of discussion about this, to no 
 conclusive results, and any argument may be responded with "I 
 don't buy that". That's simply because, again, there's some 
 subjective factor and there is no perfect approach that proves 
 the others wrong. To wit, I have stated my argument several 
 times in public and private but somehow it did not count.


Apologies if I have forgotten about any important arguments from 
past discussions; however, I can't recall any substantial ones 
that would settle this argument.


 Also, obviously the Internet can be used to find support for 
 anything, and with that caveat allow me:

 * "Why does every serious Github repo I do pull requests for 
 want me to squash my commits into a single commit?" 
 http://softwareengineering.stackexchange.com/questions/263164/why-squash-git-commits-for-pull-requests


The top answer advocates not squashing the commits when it 
doesn't make sense to.


 * "I’ve been contributing to more projects in which it is 
 expected that I should submit a pull request that contains a 
 single commit." 
 http://ndlib.github.io/practices/one-commit-per-pull-request/


This article was brought up here before, with a similar debate.


 * "Squashing your branch's changes into one commit is "good 
 form" and helps the person merging your request to see 
 everything that is going on." 
 https://github.com/projecthydra/hydra-head/blob/master/CONTRIBUTING.md


Only suggests considering whether squashing is appropriate. Links 
to the previous article.


 * "But one thing occasionally bothers me, and that's pull 
 requests that come loaded with several temporary commits. I 
 often find myself asking contributors to squash those into a 
 single descriptive commit." 
 http://eli.thegreenplace.net/2014/02/19/squashing-github-pull-requests-into-a-single-commit


Mentions that the steps are for dealing with "temporary commits". 
Does not advocate one style over the other.

Perhaps there is some misunderstanding on what the point of 
disagreement is?

- Splitting changes across multiple pull requests is a point 
orthogonal to this one. I do wish we would adopt a workflow where 
larger pull requests are acceptable as long as the changes are 
properly divided into commits, as I believe it would be overall 
more productive; however, this is not what this thread is about.

- Squashing temporary commits is always good. We are both in 
agreement here.

- What we seem to disagree on is whether commits should be 
squashed in other cases. You argue that if a pull request 
contains enough changes that they could be split across several 
commits, they should be split across several pull requests. I am 
not contesting this argument (at least in this discussion). 
However, you also seem to say that pull requests containing 
enough changes that they could be split across several commits 
should be squashed into a single commit before merging, which is 
what I can't comprehend.

To reiterate, squashing commits that do not need to be squashed 
destroys information (commit messages and separation of changes) 
and makes bisecting, blaming and generally examining git history 
more difficult. Squashing discourages splitting changes into 
granular commits (even smaller than would make sense for a PR), 
and documenting each in part. Writing detailed commit messages is 
really important - not only does it provide additional context to 
people not familiar with the code base in question, it also 
forces the author to review their justification for the changes 
and consider alternative solutions.

What does squashing achieve, on the practical front?


 Vladimir, I am gathering that this makes you unhappy and are 
 close to getting ultimative about this.


What I'm unhappy about is that waste so much time arguing about 
things that seem to have an obvious correct answer, and when I 
ask for rationales for your decisions, I get huge replies about 
politics and what Facebook does and how these discussions don't 
scale and zero practical arguments to support your point. This is 
a systematic problem and has happened several times before.

Where are the practical arguments?

Closest thing is link number 2. Quoting:


 For pull requests, a single commit is easier to inspect, 
 critique, and discuss. By creating a single commit, I am saying 
 “This is a logical unit of work” for the project. I can explain 
 what and why these changes are made - developer documentation 
 if you will. At a future date, for other contributors, it is 
 easier to get a context for a small change in one file if that 
 change tracks to a larger unit of work.


Exactly the same arguments apply to each commit in a commit 
series which should not be squashed.

Where is the rationale for squashing self-contained commits with 
well-written commit messages over not doing so?


 * Any organization, be it for profit (company) or not 
 (university, oss project) has certain rules in place. In all 
 likelihood not all participants would choose the exact same 
 rules if they could, yet they choose to walk (factually or 
 virtually) in there every day and contribute to the 
 organization. A simple way to look at things would be, would 
 you quit a job or start a protracted argument with the CEO if 
 the coding guidelines contained "please squash your github 
 commits"?


If a company's internal guidelines contain rules written by the 
CEO, that most workers disagree with, and which seem to lead to 
the opposite of productivity, it would certainly be a big red 
flag.


 * The leader of an organization (in this case me) cannot afford 
 the time to justify and debate every single minor decision they 
 make. No organization works that way. I am under enormous 
 pressure to bring money in the Foundation, organize DConf, 
 mentor students, write articles, contribute code, all while 
 keeping things running.


This is why people delegate, right? As you're aware, both Martin 
and I are in favor of squashing conservatively.


 * You seem to frame things as an arbitrary imposition made by 
 an oppressive leadership. Think of it - ever since we have 
 worked together (your first post is dated 2007-01-06!), is this 
 the pattern by which matters have been conducted? Do you think 
 it would be a fair characterization of Walter and my philosophy 
 and values?


Let's not pretend that there have been zero decisions made 
seemingly in opposite to the community's consensus before. And, 
reminder: I did start the page 
http://wiki.dlang.org/Language_issues :)


 The short of it is we simply cannot work this way, as a simple 
 matter of my own human limitations; I can't spend hours in the 
 forum justifying every little thing, especially those that ten 
 people do in eleven ways. I mean I _literally_ can't: I don't 
 have the time because I am spending it making the D language 
 more successful. Which I hope you'll help me with, too. To 
 scale, we need to focus on the real problems and abandon 
 debating the small things again and again.


I understand, but it's not either-or. I think delegation and 
trust in the community are both viable alternatives... but, more 
importantly, for the past few instances the discussions would 
have been a lot shorter if we'd stick to the practical arguments 
that matter.

To try to cut a long thread short, you wrote:


 Please reach to squash commits as the default, unless there is 
 a special case.



 with exceptions that need to be justified.


I just think that "multiple self-contained commits which are too 
granular to split into multiple PRs" should be an implied special 
case that should not be justified. This is all.
Feb 28 2017
prev sibling parent reply Seb <seb wilzba.ch> writes:
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.)

 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?


Not that I would know of.
I started an initial wiki entry (basically a cleaned copy/paste 
version of my post):

https://wiki.dlang.org/Guidelines_for_maintainers

 other maintainers: As this seems to be quite useful for 
newcomers and semi-regular contributors, please help to extend 
this wiki entry (or whenever a question pops up on GH).
Feb 28 2017
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 02/28/2017 11:03 AM, Seb wrote:

 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.)

 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?


 Not that I would know of.
 I started an initial wiki entry (basically a cleaned copy/paste version
 of my post):

 https://wiki.dlang.org/Guidelines_for_maintainers

  other maintainers: As this seems to be quite useful for newcomers and
 semi-regular contributors, please help to extend this wiki entry (or
 whenever a question pops up on GH).


That's awesome. Thanks! -- Andrei
Feb 28 2017