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: ``` #!/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
Feb 26
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
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
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
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
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 # Editor pops up, change "pick" to "squash" (or "s" for short) # for every commit you wish to squash. 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
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
 	# Editor pops up, change "pick" to "squash" (or "s" for short)
 	# for every commit you wish to squash.
 	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
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
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
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
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
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
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
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
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
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
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
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
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
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