www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - The delang is using merge instead of rebase/squash

reply deadalnix <deadalnix gmail.com> writes:
This is making the history very spaghettified. Is that possible 
to have the bot rebase/squash commits and then pushing ?
Mar 15
next sibling parent reply deadalnix <deadalnix gmail.com> writes:
On Wednesday, 15 March 2017 at 13:14:31 UTC, deadalnix wrote:
 This is making the history very spaghettified. Is that possible 
 to have the bot rebase/squash commits and then pushing ?
Arf I fat fingered the title, i meant the dlang bot.
Mar 15
parent Seb <seb wilzba.ch> writes:
On Wednesday, 15 March 2017 at 13:23:00 UTC, deadalnix wrote:
 On Wednesday, 15 March 2017 at 13:14:31 UTC, deadalnix wrote:
 This is making the history very spaghettified. Is that 
 possible to have the bot rebase/squash commits and then 
 pushing ?
Arf I fat fingered the title, i meant the dlang bot.
I absolutely agree with you and in fact Andrei and I pushed for auto-merge-squash, which was enabled for the last three months. However, recently Martin disabled it to prevent "accidental squashes". Discussion: https://github.com/dlang-bots/dlang-bot/issues/64
Mar 15
prev sibling parent reply Martin Nowak <code dawg.eu> writes:
On Wednesday, 15 March 2017 at 13:14:31 UTC, deadalnix wrote:
 This is making the history very spaghettified. Is that possible 
 to have the bot rebase/squash commits and then pushing ?
I don't really agree with the argument. A merge commit is a clear way to integrate changes from a PR/branch. Just rebasing a PR on top of master removes a lot of information from git, only leaving references to github. Can you be more specific, what you mean w/ spaghetti? The fact that reciew fixes are added to PRs. Also github's commit view misleadingly shows commits from merged PRs/branches, which aren't actually in master.
Mar 19
next sibling parent reply deadalnix <deadalnix gmail.com> writes:
On Monday, 20 March 2017 at 05:10:04 UTC, Martin Nowak wrote:
 On Wednesday, 15 March 2017 at 13:14:31 UTC, deadalnix wrote:
 This is making the history very spaghettified. Is that 
 possible to have the bot rebase/squash commits and then 
 pushing ?
I don't really agree with the argument. A merge commit is a clear way to integrate changes from a PR/branch. Just rebasing a PR on top of master removes a lot of information from git, only leaving references to github. Can you be more specific, what you mean w/ spaghetti? The fact that reciew fixes are added to PRs. Also github's commit view misleadingly shows commits from merged PRs/branches, which aren't actually in master.
Because a picture is clearer than a thousand words: | | | | | | | | * | | | | | | | 08ae52d8 The Dlang Bot |\ \ \ \ \ \ \ \ Merge pull request #5231 from RazvanN7/Update_generated | |_|_|_|_|/ / / |/| | | | | | | | | | | | | | | | * | | | | | | c6480976 RazvanN7 |/ / / / / / / Updated posix.mak makefile to use ../tools/checkwhitespace.d | | | | | | | * | | | | | | 1181fcf7 The Dlang Bot |\ \ \ \ \ \ \ Merge pull request #5239 from sprinkle131313/ignore-vscode-lib | | | | | | | | | * | | | | | | f1b8d0d4 sprinkle131313 | | | | | | | | Add temp/tmp folder to gitignore. | | | | | | | | | * | | | | | | b67bf9d1 sprinkle131313 | | |_|/ / / / Add vscode folder and lib files to gitignore. | |/| | | | | | | | | | | | * | | | | | | 0b41c996 The Dlang Bot |\ \ \ \ \ \ \ Merge pull request #5242 from wilzbach/fix-lref-links | | | | | | | | | * | | | | | | 090d5164 Sebastian Wilzbach |/ / / / / / / Fix links from $(LREF $(D ...)) -> $(LREF ...) | | | | | | | * | | | | | | f2a019df The Dlang Bot |\ \ \ \ \ \ \ Merge pull request #5241 from MartinNowak/merge_stable | | | | | | | | | * | | | | | | a6cb85b8 Sebastian Wilzbach | | | | | | | | Add safe to std.regex unittest | | | | | | | | | * | | | | | | ad70b082 Martin Nowak | |\ \ \ \ \ \ \ Merge remote-tracking branch 'upstream/stable' into merge_stable |/ / / / / / / / | | | | | | | | | * | | | | | | 694dd174 Stefan Koch | |\ \ \ \ \ \ \ Merge pull request #5167 from DmitryOlshansky/fix-freeform-regex | | | | | | | | | | | * | | | | | | 62cf615d Dmitry Olshansky | |/ / / / / / / Fix issue 17212 std.regex doesn't ignore whitespace after character classes | | | | | | | | * | | | | | | | 5b07bd59 Sebastian Wilzbach | |_|_|_|/ / / [BOOKTABLES]: Add BOOKTABLE to stdx.checkedint (#5238) |/| | | | | | | | | | | | | * | | | | | | 75059373 Jack Stouffer |\ \ \ \ \ \ \ Merge pull request #5225 from wilzbach/booktable-std-utf | |_|_|_|_|_|/ |/| | | | | | What the hell is going on in there ? In addition there are a bunch of practical issues with this way of doing things. First there is no given that any intermediate state is sound, or even builds at all. That makes it very hard to bissect anything. There are also a lot of errands and correction that are made during review that are not that interesting to keep in the project history. Knowing that someone did thing the A way and then changed it the B way after review is more noise than useful infos in the general case, and in the rare case when someone actually wants to know, the github PR is still out there (on that note, yes GH PR kind fo sucks, but that's another topic).
Mar 20
next sibling parent Martin Nowak <code dawg.eu> writes:
On Monday, 20 March 2017 at 12:25:22 UTC, deadalnix wrote:
 In addition there are a bunch of practical issues with this way 
 of doing things. First there is no given that any intermediate 
 state is sound, or even builds at all. That makes it very hard 
 to bissect anything.
You bissect on master and there is one merge commit per PR, no intermediate states involved.
 There are also a lot of errands and correction that are made 
 during review that are not that interesting to keep in the 
 project history. Knowing that someone did thing the A way and 
 then changed it the B way after review is more noise than 
 useful infos in the general case, and in the rare case when 
 someone actually wants to know, the github PR is still out 
 there (on that note, yes GH PR kind fo sucks, but that's 
 another topic).
That's a conflict of interest. As said, GH's interface is targetted toward pushing review fixes as new commits, rather than ammending changes. And yes those commits mostly provide little information, but they're also on a separate branch. Using auto-squash before merging https://github.com/dlang-bots/dlang-bot/issues/64#issuecomment-284155249 made sense but isn't offered by GH's API and thus requires quite some work. Just squashing everything to a single commit and putting that on master (as done by GH's squash+rebase) doesn't preserve all information in git. Also smells like it might cause automation troubles at some point.
Mar 20
prev sibling parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Monday, 20 March 2017 at 12:25:22 UTC, deadalnix wrote:
 Because a picture is clearer than a thousand words:
What this tells me is that the default way git-log presents history is not very useful. Consider this presentation of the same information: 08ae52d8 The Dlang Bot: Merge pull request #5231 from RazvanN7/Update_generated - c6480976 RazvanN7: Updated posix.mak makefile to use ../tools/checkwhitespace.d 1181fcf7 The Dlang Bot: Merge pull request #5239 from sprinkle131313/ignore-vscode-lib - f1b8d0d4 sprinkle131313: Add temp/tmp folder to gitignore. - b67bf9d1 sprinkle131313: Add vscode folder and lib files to gitignore. 0b41c996 The Dlang Bot: Merge pull request #5242 from wilzbach/fix-lref-links - 090d5164 Sebastian Wilzbach: Fix links from $(LREF $(D ...)) -> $(LREF ...) f2a019df The Dlang Bot: Merge pull request #5241 from MartinNowak/merge_stable - a6cb85b8 Sebastian Wilzbach: Add safe to std.regex unittest - ad70b082 Martin Nowak: Merge remote-tracking branch 'upstream/stable' into merge_st… - 694dd174 Stefan Koch: Merge pull request #5167 from DmitryOlshansky/fix-freeform-… - 62cf615d Dmitry Olshansky: Fix issue 17212 std.regex doesn't ignore whitespace … 5b07bd59 Sebastian Wilzbach: [BOOKTABLES]: Add BOOKTABLE to stdx.checkedint (#5238) 75059373 Jack Stouffer: Merge pull request #5225 from wilzbach/booktable-std-utf In particular, the origin commit of a branch is often not interesting; only the list of commits that are on one branch and aren't on another are. Anyway, personally I don't think there is a severe problem in dire need of fixing with the git log excerpt you pasted. If you're interested in looking at changes to the master branch, look for asterisks in the first column.
 In addition there are a bunch of practical issues with this way 
 of doing things.
There seem to be more practical issues with the opposite approach.
 First there is no given that any intermediate state is sound, 
 or even builds at all. That makes it very hard to bissect 
 anything.
Bisecting D is not something that can be reasonably done by looking at just one repository's history anyway; this is why we have D-dot-git and Digger. Either way, for pull requests that make non-trivial changes or additions, you will need to descend into the pull request itself.
 There are also a lot of errands and correction that are made 
 during review that are not that interesting to keep in the 
 project history. Knowing that someone did thing the A way and 
 then changed it the B way after review is more noise than 
 useful infos in the general case,
Agreed, this is one case where squashing is appropriate. However, consider the worst-case scenarios where either merge strategy is abused: - If a pull request that should have been squashed has been merged without squashing, the result is: - Some clutter in the git history; - Possible (but avoidable) complications when doing git-bisect on a single repository, which you shouldn't be doing anyway. - If a pull request that should not have been squashed has been squashed while merging, the result is: - Commit messages are lost and remain available only on GitHub. - Any logical separation of changes that might have been represented through separate commits is lost and remains available only on GitHub. - "git blame" becomes less useful because it can only lead to the big blob of the squashed changes. - "git blame" becomes less useful because in some situations it loses its ability to track moved code, which should and often is done in separate commits. - Bisection becomes more difficult because it is no longer easily possible to dive into a PR, as has been occasionally necessary. In general, I am not opposed to giving reviewers the option to merge pull requests with squashing, assuming we can all agree to not abuse it and only use it for PRs where there nothing useful can be gained by preserving the multiple commits as they are; however, their words and actions have shown that this doesn't seem to be an attainable point of agreement.
Mar 20
parent reply deadalnix <deadalnix gmail.com> writes:
On Tuesday, 21 March 2017 at 01:39:39 UTC, Vladimir Panteleev 
wrote:
 On Monday, 20 March 2017 at 12:25:22 UTC, deadalnix wrote:
 Because a picture is clearer than a thousand words:
What this tells me is that the default way git-log presents history is not very useful. Consider this presentation of the same information:
It's not good either. Why would I want to look at a DAG when the serie of event is strictly linear to begin with ?
 In particular, the origin commit of a branch is often not 
 interesting; only the list of commits that are on one branch 
 and aren't on another are.
Yes, that's why rebasing makes thing clearer. Nobody care what the master commit was when the work was started.
 First there is no given that any intermediate state is sound, 
 or even builds at all. That makes it very hard to bissect 
 anything.
Bisecting D is not something that can be reasonably done by looking at just one repository's history anyway; this is why we have D-dot-git and Digger. Either way, for pull requests that make non-trivial changes or additions, you will need to descend into the pull request itself.
"Our source control is completely broken, but that's not a problem because we developed 3rd party tools to work around the brokenness" While I agree with you that things like bisecting are broken in D, I don't see it as a reason to screw things up even more. I'm not a big fan of "it's already broken, so we can break it even more". This should, and can, be fixed. https://danluu.com/monorepo/ Incidentally, I got a company contacting me last week willing to pay me good money to help them transition toward these kind of workflow.
 - If a pull request that should not have been squashed has been 
 squashed while merging, the result is:
   - Commit messages are lost and remain available only on 
 GitHub.
   - Any logical separation of changes that might have been 
 represented through separate commits is lost and remains 
 available only on GitHub.
   - "git blame" becomes less useful because it can only lead to 
 the big blob of the squashed changes.
   - "git blame" becomes less useful because in some situations 
 it loses its ability to track moved code, which should and 
 often is done in separate commits.
   - Bisection becomes more difficult because it is no longer 
 easily possible to dive into a PR, as has been occasionally 
 necessary.
Then it should have been 2 PR or more to begin with. Splitting PR in smaller ones is a good practice in general, there are ample proof that is increase the quality of the code review, reduce conflicts surface with other PR, makes reverting easier and more targeted when something happens, etc... Keeping this PR's commits is just a way to mitigate one of the negative consequences of kitchen sink PRs. It does so by impacting negatively others aspects of source control, and does nothing to mitigate other negatives aspects of kitchen sink PRs, such as review fatigue (see a specific example below).
 In general, I am not opposed to giving reviewers the option to 
 merge pull requests with squashing, assuming we can all agree 
 to not abuse it and only use it for PRs where there nothing 
 useful can be gained by preserving the multiple commits as they 
 are; however, their words and actions have shown that this 
 doesn't seem to be an attainable point of agreement.
If multiple commits are important for the PR, then the PR should have been several PR to begin with. Asking people to split s the way to go. Consider this PR: https://github.com/BitcoinUnlimited/BitcoinUnlimited/pull/164 You can see in the comments that I asked the original author to split it up because it was a kitchen sink and very hard to review in its current form. This was ignored. The PR ended up containing a bug that would cost about $12 500 to one of the users of the software, plus a fair amount of reputational damage. The change containing the bug did not need to be bundled with the rest of the PR, and would have almost certainly be noticed if it had been made on a PR of its own. Bundling several changes in the same PR has real world consequences that go beyond screwing up source control.
Mar 21
next sibling parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Tuesday, 21 March 2017 at 11:59:42 UTC, deadalnix wrote:
 It's not good either. Why would I want to look at a DAG when 
 the serie of event is strictly linear to begin with ?
Not sure what you mean here. The way it's presented is not a DAG.
 Yes, that's why rebasing makes thing clearer. Nobody care what 
 the master commit was when the work was started.
Sure, I'm not against rebasing. It's the squashing that's problematic.
 "Our source control is completely broken, but that's not a 
 problem because we developed 3rd party tools to work around the 
 brokenness"
That's fallacious.
 While I agree with you that things like bisecting are broken in 
 D, I don't see it as a reason to screw things up even more. I'm 
 not a big fan of "it's already broken, so we can break it even 
 more". This should, and can, be fixed.

 https://danluu.com/monorepo/

 Incidentally, I got a company contacting me last week willing 
 to pay me good money to help them transition toward these kind 
 of workflow.
I don't disagree with you, but this is a different discussion that's orthogonal to this one.
 Then it should have been 2 PR or more to begin with. Splitting 
 PR in smaller ones is a good practice in general,
You are changing the subject. I'll reply in another post with a different subject.
Mar 21
parent reply deadalnix <deadalnix gmail.com> writes:
On Tuesday, 21 March 2017 at 12:45:45 UTC, Vladimir Panteleev 
wrote:
 On Tuesday, 21 March 2017 at 11:59:42 UTC, deadalnix wrote:
 It's not good either. Why would I want to look at a DAG when 
 the serie of event is strictly linear to begin with ?
Not sure what you mean here. The way it's presented is not a DAG.
Blue is red, up is down, and the commit graph is not a DAG.
 "Our source control is completely broken, but that's not a 
 problem because we developed 3rd party tools to work around 
 the brokenness"
That's fallacious.
If you can't bissect, it's broken. Listen, you know it's broken because you wrote tools to work around the brokenness. If it wasn't broken you wouldn't have written these tools as there would be no need to do so. So let's not play pretend.
Mar 21
parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Tuesday, 21 March 2017 at 17:58:06 UTC, deadalnix wrote:
 On Tuesday, 21 March 2017 at 12:45:45 UTC, Vladimir Panteleev 
 wrote:
 On Tuesday, 21 March 2017 at 11:59:42 UTC, deadalnix wrote:
 It's not good either. Why would I want to look at a DAG when 
 the serie of event is strictly linear to begin with ?
Not sure what you mean here. The way it's presented is not a DAG.
Blue is red, up is down, and the commit graph is not a DAG.
Not sure what you mean. The commit graph is a DAG. The way you quoted my post made your remark seem to refer to my attempt to reformat it, which is not presented as a DAG.
 "Our source control is completely broken, but that's not a 
 problem because we developed 3rd party tools to work around 
 the brokenness"
That's fallacious.
If you can't bissect, it's broken.
By that definition of "broken", all git repositories which use branch merging are "broken". That includes some of the biggest open-source projects. Frankly, if you want to stick to that definition, I have nothing against it.
 Listen, you know it's broken because you wrote tools to work 
 around the brokenness. If it wasn't broken you wouldn't have 
 written these tools as there would be no need to do so. So 
 let's not play pretend.
Digger would probably have existed even if D were a monorepo and squashed PRs' commits from the start, because it also knows how to satisfy each prior version's build dependencies and how to invoke the build scripts. Regardless, D is perfectly suitable for automatic bisection, which is unreasonably awkward with git itself - Digger makes it much easier. I think there's no shame in writing domain-specific tools to enhance some functionality of standard ones.
Mar 21
next sibling parent Sebastien Alaiwan <ace17 free.fr> writes:
It's common practice for "merge" commits to have the form:
"merge work from some/branch, fix PR #somenumber".

This basically tells me nothing about what this commit does.

We already know it's a merge commit, we don't care so much what 
branch it's from, and we don't want to dig into the bug tracker 
to translate the issue number into english.

We care more about how this merge modifies the code behaviour.

What if "merge" commits had better messages, not containing the 
word "merge" at all?
This way, the depth-0 history, which is always linear, would be 
human-readable and bisectable.
Mar 21
prev sibling parent Daniel N <no public.email> writes:
On Wednesday, 22 March 2017 at 01:25:37 UTC, Vladimir Panteleev 
wrote:
 On Tuesday, 21 March 2017 at 17:58:06 UTC, deadalnix wrote:
 On Tuesday, 21 March 2017 at 12:45:45 UTC, Vladimir Panteleev 
 wrote:
 On Tuesday, 21 March 2017 at 11:59:42 UTC, deadalnix wrote:
 It's not good either. Why would I want to look at a DAG when 
 the serie of event is strictly linear to begin with ?
This is almost human readable... git log --first-parent --no-merges --decorate ... except if a merge commit is tagged, I haven't found any solution for that, can you? It's very important to be able to see tags, yet filter away merge commits. Fortunately I managed to convert my team to rebase, so I no longer suffer this problem at work, only with D. Even this simplest git commands break down: git show WTF? there was no difference? Ahh... I was supposed to type: git show --first-parent well at least this case can be solved by a simple alias, but log cannot.
Mar 22
prev sibling parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Tuesday, 21 March 2017 at 11:59:42 UTC, deadalnix wrote:
 Then it should have been 2 PR or more to begin with. Splitting 
 PR in smaller ones is a good practice in general,
This is probably true for many cases, but I don't think it's a general truth. First, there are extreme cases like these: https://github.com/dlang/druntime/pull/1402 https://github.com/dlang/phobos/pull/260 I think we can agree that it would be better to have 1 pull request with 70 commits than 70 pull requests with 1 commit. Second, there are many cases that fall in the middle: some ancillary change (such as a minor refactoring, or a build file change) is needed by a bigger change, but it is too minor to be a PR of its own. Putting both in one commit is also unwarranted. I think that the philosophy to prefer squashing is not suited for projects such as D, where we care about history. Pushing for one change per PR also pushes people to put too many things in one commit, and write less descriptive commit messages. Finally, some of the biggest open source projects merge pull requests consisting of multiple commits, and encourage submitters to divide their changes into as many commits as is logical, which seems to be the workflow they consider optimal.
 there are ample proof that is increase the quality of the code 
 review,
OK, where is the proof? It is worth pointing out that GitHub's UI is heavily biased towards reviewing PRs in entirety, however it does allow reviewing PRs commit by commit, which is how I think non-trivial submissions and reviews should occur anyway.
 reduce conflicts surface with other PR, makes reverting easier 
 and more targeted when something happens, etc...
Sure, I'm not advocating that all submissions happen as one PR. The way I understand it, it is you who is advocating the extreme position that all PRs should always contain a single commit.
 Keeping this PR's commits is just a way to mitigate one of the 
 negative consequences of kitchen sink PRs. It does so by 
 impacting negatively others aspects of source control, and does 
 nothing to mitigate other negatives aspects of kitchen sink PRs,
Frankly I don't think this makes any sense at all.
 such as review fatigue (see a specific example below).
The other side of the coin is submitter fatigue. I've seen this happen: 1. Submitter submits a PR, containing two commits: a change, and a refactoring required for the change. 2. Reviewers ask the submitter to split it into two PRs. 3. Submitter resubmits the refactoring as a separate PR. 4. The refactoring PR sits in the review queue forever because at best, it does nothing, at worst it introduces a regression. Reviewers who did not see or bother to read the first PR ask what this refactoring is for and why it's needed. 5. Submitter is fed up and leaves.
 Consider this PR: 
 https://github.com/BitcoinUnlimited/BitcoinUnlimited/pull/164

 You can see in the comments that I asked the original author to 
 split it up because it was a kitchen sink and very hard to 
 review in its current form. This was ignored. The PR ended up 
 containing a bug that would cost about $12 500 to one of the 
 users of the software, plus a fair amount of reputational 
 damage. The change containing the bug did not need to be 
 bundled with the rest of the PR, and would have almost 
 certainly be noticed if it had been made on a PR of its own.

 Bundling several changes in the same PR has real world 
 consequences that go beyond screwing up source control.
I don't think that's a fair example at all. What exactly prevents reviewing a PR consisting of multiple commits differently from multiple PRs consisting of one commit? In both cases, you can: - look at each change individually - add review comments on the change, either on the change in entirety or on individual lines - selectively pick and merge a subset of the submitted changes (though, GitHub makes this more difficult for multi-commit PRs). I can only guess that by "difficult to review" you mean from only looking at the "diff" tab; however, I think it's disingenuous to say that if you are not using the tools properly. Anyway, to reiterate, this is a distinct argument from which merge strategy to use. However, generally, I think this approach is more bad than good because it pushes towards destroying information (commit messages and separation) and clumping too many minor changes into single commits.
Mar 21
next sibling parent reply deadalnix <deadalnix gmail.com> writes:
On Tuesday, 21 March 2017 at 12:49:22 UTC, Vladimir Panteleev 
wrote:
 there are ample proof that is increase the quality of the code 
 review,
OK, where is the proof?
Large companies such as Google or Facebook measure these things. You have presented 0 arguments so far, and dismissed both facts and argument that were presented to you (one of them as unfair, because fairness and correctness surely are correlated). But cool guys you are right, don't change anything. This is great. I have other things to do to convince you guys when other are paying me to do so.
Mar 21
next sibling parent Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Tuesday, 21 March 2017 at 18:07:57 UTC, deadalnix wrote:
 You have presented 0 arguments so far, and dismissed both facts 
 and argument that were presented to you (one of them as unfair, 
 because fairness and correctness surely are correlated).
This is factually wrong, as is obvious from reading the thread. If you are not interested in constructive discussion, then I'm sorry that both of our times have been wasted.
Mar 21
prev sibling parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Tuesday, 21 March 2017 at 18:07:57 UTC, deadalnix wrote:
 Large companies such as Google or Facebook
A blind appeal to authority is fallacious, but it's still worthwhile to see what others are doing. I think it's important to look at projects that are similar to our own, so I looked at what other programming language implementations do. - Go is developed using Google's source code infrastructure, and code reviews happen using Gerrit. On Gerrit, every commit is reviewed separately (as I've been advocating). Furthermore, if you push multiple commits to Gerrit, this automatically creates one review page per commit, and marks them as inter-dependent in the commit order. This is an awesome approach, and I wish GitHub made this workflow more practical. Importantly, Gerrit does not squash commits - you are expected to squash fixup commits yourself. - Rust uses GitHub, and all merges seem to be done by a bot. We are heading in that direction too. The bot uses regular merges and does not squash commits or rebase them onto master. - Python: I looked at the CPython repository on GitHub. They seem to be using squashing exclusively, and only using branches for version maintenance. However, when I tried to find how they would deal with a contribution that would be desirable to be split into several PRs/commits, I couldn't find one on the first 5 pages of merged PRs. I guess the project is in the stage of mostly minor bugfixes only - we're certainly not there yet. Curiously, submitters are expected to resubmit the same PR themselves against every maintenance branch, e.g. here is the same PR submitted 4 times, to different branches: - https://github.com/python/cpython/pull/629 - https://github.com/python/cpython/pull/633 - https://github.com/python/cpython/pull/634 - https://github.com/python/cpython/pull/635 - Ruby uses Subversion, a GitHub mirror, and a bot which synchronizes between the two. I don't think there's anything we can learn from here. - OCaml uses GitHub PRs and regular git merges. - Clang and GHC use Phabricator. I'm not too familiar with it, but I understand it's not too different from Gerrit: it creates one review per commit, and you can push multiple commits at once which will do the right thing. To sum it up, I don't think we're doing anything too weird. Though it would be nice if GitHub's UI were to improve to better handle this workflow, I don't think it makes sense to force submitters to go through the busywork of creating one PR per commit for many cases.
Mar 22
next sibling parent reply Daniel N <no public.email> writes:
On Wednesday, 22 March 2017 at 09:02:24 UTC, Vladimir Panteleev 
wrote:
 Importantly, Gerrit does not squash commits - you are expected 
 to squash fixup commits yourself.
You can configure Gerrit to do virtually anything, including squashing, even cherry-pick if you fancy.
Mar 22
parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Wednesday, 22 March 2017 at 10:43:46 UTC, Daniel N wrote:
 On Wednesday, 22 March 2017 at 09:02:24 UTC, Vladimir Panteleev 
 wrote:
 Importantly, Gerrit does not squash commits - you are expected 
 to squash fixup commits yourself.
You can configure Gerrit to do virtually anything, including squashing, even cherry-pick if you fancy.
Ah, thanks. Could you link me to the relevant documentation? Looking at https://bugs.chromium.org/p/gerrit/issues/detail?id=1254, it seems Gerrit can't handle multiple commits in any scenarios right now. Either way, I guess it doesn't squash a series of inter-dependent commits/reviews into one :)
Mar 22
parent reply Daniel N <no public.email> writes:
On Wednesday, 22 March 2017 at 10:49:37 UTC, Vladimir Panteleev 
wrote:
 On Wednesday, 22 March 2017 at 10:43:46 UTC, Daniel N wrote:
 On Wednesday, 22 March 2017 at 09:02:24 UTC, Vladimir 
 Panteleev wrote:
 Importantly, Gerrit does not squash commits - you are 
 expected to squash fixup commits yourself.
You can configure Gerrit to do virtually anything, including squashing, even cherry-pick if you fancy.
Ah, thanks. Could you link me to the relevant documentation? Looking at https://bugs.chromium.org/p/gerrit/issues/detail?id=1254, it seems Gerrit can't handle multiple commits in any scenarios right now. Either way, I guess it doesn't squash a series of inter-dependent commits/reviews into one :)
https://gerrit-review.googlesource.com/Documentation/project-configuration.html#submit_type It's also possible to extend the basic functionality with plugins.
Mar 22
parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Wednesday, 22 March 2017 at 11:26:49 UTC, Daniel N wrote:
 On Wednesday, 22 March 2017 at 10:49:37 UTC, Vladimir Panteleev 
 wrote:
 On Wednesday, 22 March 2017 at 10:43:46 UTC, Daniel N wrote:
 You can configure Gerrit to do virtually anything, including 
 squashing, even cherry-pick if you fancy.
Ah, thanks. Could you link me to the relevant documentation?
https://gerrit-review.googlesource.com/Documentation/project-configuration.html#submit_type
Thanks. I don't see anything about squashing, though.
Mar 22
parent Daniel N <no public.email> writes:
On Wednesday, 22 March 2017 at 11:35:11 UTC, Vladimir Panteleev 
wrote:
 On Wednesday, 22 March 2017 at 11:26:49 UTC, Daniel N wrote:
 On Wednesday, 22 March 2017 at 10:49:37 UTC, Vladimir 
 Panteleev wrote:
 On Wednesday, 22 March 2017 at 10:43:46 UTC, Daniel N wrote:
 You can configure Gerrit to do virtually anything, including 
 squashing, even cherry-pick if you fancy.
Ah, thanks. Could you link me to the relevant documentation?
https://gerrit-review.googlesource.com/Documentation/project-configuration.html#submit_type
Thanks. I don't see anything about squashing, though.
The documentation is not very obvious. IIRC it was this option... "Create a new change for every commit not in the target branch: false" ... but it also requires a specific submit type in combination with that option which submits what is in the change-review instead of what's in the branch. (this is useful because reviewers can simply opt to fix a spelling error directly in the browser rather than just commenting on it, totally avoiding the normal ping-pong).
Mar 22
prev sibling parent reply deadalnix <deadalnix gmail.com> writes:
On Wednesday, 22 March 2017 at 09:02:24 UTC, Vladimir Panteleev 
wrote:
 On Tuesday, 21 March 2017 at 18:07:57 UTC, deadalnix wrote:
 Large companies such as Google or Facebook
A blind appeal to authority is fallacious, but it's still worthwhile to see what others are doing. I think it's important to look at projects that are similar to our own, so I looked at what other programming language implementations do.
The good new is, you provided much more authorities to confirm my claim, so is it so blind ?
 - Go is developed using Google's source code infrastructure, 
 and code reviews happen using Gerrit. On Gerrit, every commit 
 is reviewed separately (as I've been advocating). Furthermore, 
 if you push multiple commits to Gerrit, this automatically 
 creates one review page per commit, and marks them as 
 inter-dependent in the commit order. This is an awesome 
 approach, and I wish GitHub made this workflow more practical. 
 Importantly, Gerrit does not squash commits - you are expected 
 to squash fixup commits yourself.
So Go use squash.
 - Rust uses GitHub, and all merges seem to be done by a bot. We 
 are heading in that direction too. The bot uses regular merges 
 and does not squash commits or rebase them onto master.
So that's 1.
 - Python: I looked at the CPython repository on GitHub. They 
 seem to be using squashing exclusively, and only using branches 
 for version maintenance. However, when I tried to find how they 
 would deal with a contribution that would be desirable to be 
 split into several PRs/commits, I couldn't find one on the 
 first 5 pages of merged PRs. I guess the project is in the 
 stage of mostly minor bugfixes only - we're certainly not there 
 yet.

   Curiously, submitters are expected to resubmit the same PR 
 themselves against every maintenance branch, e.g. here is the 
 same PR submitted 4 times, to different branches:

   - https://github.com/python/cpython/pull/629
   - https://github.com/python/cpython/pull/633
   - https://github.com/python/cpython/pull/634
   - https://github.com/python/cpython/pull/635
So they use squash.
 - Ruby uses Subversion, a GitHub mirror, and a bot which 
 synchronizes between the two. I don't think there's anything we 
 can learn from here.
So they use squash (that's the only thing svn knows how to do).
 - OCaml uses GitHub PRs and regular git merges.
That's 2.
 - Clang and GHC use Phabricator. I'm not too familiar with it, 
 but I understand it's not too different from Gerrit: it creates 
 one review per commit, and you can push multiple commits at 
 once which will do the right thing.
Phabricator can be configured to do many things, pretty much like gerrit, but in the case of clang and LLVM, they use squash.
 To sum it up, I don't think we're doing anything too weird. 
 Though it would be nice if GitHub's UI were to improve to 
 better handle this workflow, I don't think it makes sense to 
 force submitters to go through the busywork of creating one PR 
 per commit for many cases.
4 out of your 6 examples use squash.
Mar 22
parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Wednesday, 22 March 2017 at 15:59:29 UTC, deadalnix wrote:
 4 out of your 6 examples use squash.
No, and at this point I don't know if you're being willfully ignorant or plainly malicious. The Gerrit/Phabricator equivalent of squashing GitHub PRs would be to squash multiple inter-dependent reviewed changesets after they've all been reviewed. Suffice to say that this is not a thing that happens. There are two ways to attempt to use the Gerrit workflow on GitHub: 1. Use one PR per commit. This is pretty far from Gerrit, because there is a lot of overhead in creating and maintaining the PRs, and no way to indicate inter-dependent PRs in the system itself. Although possible in theory, it is too inconvenient to be practical (see earlier arguments in this thread). Gerrit has a lot of tooling and workflow conventions that are geared towards making this workflow practical, things which are completely absent in the GitHub world. 2. Use one PR per patch series, and review commit by commit. GitHub does allow reviewing a PR commit by commit, so even considering that it's more difficult to merge just some parts of a PR, and the results from the various merge strategies, I believe this to overall be the better solution. Without a doubt, if GitHub provided better tooling to submit a patch series in which each commit gets its own first-class review page, and allow easily updating each part of the patch series without causing severe maintenance overhead for the rest, it would be the way to go. As things are, consider, how would you submit a change set consisting of 5 commits, where each one depends on the previous one? You would need to either waste a lot of time updating dependent PRs as you update earlier commits, waste time waiting until each commit is reviewed before submitting the rest, or eschew git best practices and lump things into as few commits as you can get away with. Whereas, when all the commits are on a single branch, updating changes in an early commit is as easy as an interactive rebase and force push.
Mar 22
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
I'm a bit confused. This got settled a while ago, in part to avoid silly 
debates over the inconsequential. Our organization prefers squash before 
commit in the majority of cases. For a minority of pull requests (that 
touch many files, are semi-mechanical etc) multiple commits in one PR 
are fine within reason. These would be about one order of magnitude less 
frequent. -- Andrei
Mar 22
parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Wednesday, 22 March 2017 at 17:16:09 UTC, Andrei Alexandrescu 
wrote:
 I'm a bit confused. This got settled a while ago, in part to 
 avoid silly debates over the inconsequential. Our organization 
 prefers squash before commit in the majority of cases. For a 
 minority of pull requests (that touch many files, are 
 semi-mechanical etc) multiple commits in one PR are fine within 
 reason. These would be about one order of magnitude less 
 frequent. -- Andrei
Well, I don't think we shouldn't keep researching for ways to improve wolkflow. I certainly don't think it's inconsequential, and anyone who has time and thinks they can bring fresh arguments to the table is welcome to do so. There are some very solid arguments in favor of moving to an exclusively one-commit-per-PR model, with no exceptions (with more involved contributions occurring in feature branches), the main obstacle for which is that the tooling isn't there. I also think we can do better for the current model - the diff tab is often misused when reviewing per-commit is more appropriate.
Mar 22
next sibling parent Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Thursday, March 23, 2017 02:57:04 Vladimir Panteleev via Digitalmars-d 
wrote:
 On Wednesday, 22 March 2017 at 17:16:09 UTC, Andrei Alexandrescu

 wrote:
 I'm a bit confused. This got settled a while ago, in part to
 avoid silly debates over the inconsequential. Our organization
 prefers squash before commit in the majority of cases. For a
 minority of pull requests (that touch many files, are
 semi-mechanical etc) multiple commits in one PR are fine within
 reason. These would be about one order of magnitude less
 frequent. -- Andrei
Well, I don't think we shouldn't keep researching for ways to improve wolkflow. I certainly don't think it's inconsequential, and anyone who has time and thinks they can bring fresh arguments to the table is welcome to do so. There are some very solid arguments in favor of moving to an exclusively one-commit-per-PR model, with no exceptions (with more involved contributions occurring in feature branches), the main obstacle for which is that the tooling isn't there. I also think we can do better for the current model - the diff tab is often misused when reviewing per-commit is more appropriate.
Honestly, I think that having only one commit per PR would encourage overly large commits. Being able to have a series of small commits merged together is a strength of git, whereas something like svn usually results in patches that are single, larger commits. I also don't like the idea that commits get squashed when merged. In theory, they were separate for a reason, and in the cases that squashing them all makes sense, the commits were probably too small to begin with. But there is a bit of an art to creating commits that are small enough to be sensible while not having too many of them, and I've definitely seen PRs for Phobos that had way too many small commits, because the person who created the PR didn't bother to squash stuff where it made sense. - Jonathan M Davis
Mar 22
prev sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/23/17 4:57 AM, Vladimir Panteleev wrote:
 On Wednesday, 22 March 2017 at 17:16:09 UTC, Andrei Alexandrescu wrote:
 I'm a bit confused. This got settled a while ago, in part to avoid
 silly debates over the inconsequential. Our organization prefers
 squash before commit in the majority of cases. For a minority of pull
 requests (that touch many files, are semi-mechanical etc) multiple
 commits in one PR are fine within reason. These would be about one
 order of magnitude less frequent. -- Andrei
Well, I don't think we shouldn't keep researching for ways to improve wolkflow. I certainly don't think it's inconsequential, and anyone who has time and thinks they can bring fresh arguments to the table is welcome to do so.
Of course, new research is always welcome! The more, the better. Bring it over! There's a spectrum at work; at one extreme there's be close-mindedness that keeps a rigid status quo and refuses to accept new evidence. At the other end of the spectrum there's frequent reopening of the same debate with the same arguments, then repeatedly agreeing to close it just to repeat the cycle at the next opportunity. Walter and I think the better course of action for the community is to favor small pull requests that are squashed upon committing. There are reasons and a body of evidence that has been hashed over several times. Clearly there are extreme cases that don't do well with this flow, which confirm our understanding that no rule is a replacement of good judgment. Such rare exceptions are fine with us. But we can't afford this incessant challenge of the slightest authority and this reopening of old decisions with no new arguments. Far as I understand (and please do correct me if I'm wrong) what's being discussed now does not qualify as new research and is a reopening of a previous discussion with no new evidence, in which one side of the dialog accuses the other of appeal to authority, while simultaneously invoking appeal to its own authority. I have spent a long time this day thinking how to reply to this so as to close the argument once and for all, after I had already spent more time than is reasonable thinking and discussing this in public and in private. There really is no time for this kind of stuff if we want to scale. We should discuss how to make exceptions nogc and reference counted strings and a bunch of other important and urgent things. What steps can we take on this particular matter so we save everybody involved time and cognitive load? Thanks, Andrei
Mar 23
parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Thursday, 23 March 2017 at 22:35:13 UTC, Andrei Alexandrescu 
wrote:
 Far as I understand (and please do correct me if I'm wrong) 
 what's being discussed now does not qualify as new research and 
 is a reopening of a previous discussion with no new evidence,
Actually I think there were some interesting new arguments presented in this thread, as well as useful ancillary information on Gerrit et al and other projects' practices.
 There really is no time for this kind of stuff if we want to 
 scale. We should discuss how to make exceptions  nogc and 
 reference counted strings and a bunch of other important and 
 urgent things.
I think that if you do not think that discussing this subject any further is worth your time, then you shouldn't allocate any of your time time towards it. As previously mentioned, I don't think the arguments presented here warrant changing the status quo, so it is all theorizing.
Mar 23
parent reply Seb <seb wilzba.ch> writes:
On Friday, 24 March 2017 at 05:10:54 UTC, Vladimir Panteleev 
wrote:
 I think that if you do not think that discussing this subject 
 any further is worth your time, then you shouldn't allocate any 
 of your time time towards it. As previously mentioned, I don't 
 think the arguments presented here warrant changing the status 
 quo, so it is all theorizing.
FWIW (as mentioned before) the status quo is different to what's intended as auto-merge-squash has been disabled by Martin: https://github.com/dlang-bots/dlang-bot/issues/64
Mar 23
parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Friday, 24 March 2017 at 05:56:57 UTC, Seb wrote:
 On Friday, 24 March 2017 at 05:10:54 UTC, Vladimir Panteleev 
 wrote:
 I think that if you do not think that discussing this subject 
 any further is worth your time, then you shouldn't allocate 
 any of your time time towards it. As previously mentioned, I 
 don't think the arguments presented here warrant changing the 
 status quo, so it is all theorizing.
FWIW (as mentioned before) the status quo is different to what's intended as auto-merge-squash has been disabled by Martin: https://github.com/dlang-bots/dlang-bot/issues/64
Yep, because of the misuse-worst-case arguments. Simple solutions that guard against such mistakes are welcome. E.g. we could allow squashing if all commits' commit messages except the first one's start with "[SQUASH] " or "fixup! ".
Mar 24
parent deadalnix <deadalnix gmail.com> writes:
On Friday, 24 March 2017 at 09:27:54 UTC, Vladimir Panteleev 
wrote:
 Yep, because of the misuse-worst-case arguments. Simple 
 solutions that guard against such mistakes are welcome. E.g. we 
 could allow squashing if all commits' commit messages except 
 the first one's start with "[SQUASH] " or "fixup! ".
Because it is meant to be the default, doing only when some specific message exist is not going to fly. Using !donotsquash or alike in the commit message is, however, a good way to proceed.
Mar 24
prev sibling parent Martin Nowak <code dawg.eu> writes:
On Tuesday, 21 March 2017 at 12:49:22 UTC, Vladimir Panteleev 
wrote:
 On Tuesday, 21 March 2017 at 11:59:42 UTC, deadalnix wrote:
 Then it should have been 2 PR or more to begin with. Splitting 
 PR in smaller ones is a good practice in general,
This is probably true for many cases, but I don't think it's a general truth.
Let's please not conflate a small technical discussion about how to preserve information in git (e.g. squash vs. merge vs. rebase) with a workflow debate about proper PR size.
Mar 24
prev sibling parent reply Atila Neves <atila.neves gmail.com> writes:
On Monday, 20 March 2017 at 05:10:04 UTC, Martin Nowak wrote:
 On Wednesday, 15 March 2017 at 13:14:31 UTC, deadalnix wrote:
 This is making the history very spaghettified. Is that 
 possible to have the bot rebase/squash commits and then 
 pushing ?
I don't really agree with the argument. A merge commit is a clear way to integrate changes from a PR/branch. Just rebasing a PR on top of master removes a lot of information from git, only leaving references to github. Can you be more specific, what you mean w/ spaghetti? The fact that reciew fixes are added to PRs. Also github's commit view misleadingly shows commits from merged PRs/branches, which aren't actually in master.
There's a compromise, which I'm using right now. Always rebase and always merge. You can see that it branched off for a purpose (the argument for merging) and the history is much cleaner (the argument for rebasing). i.e.: git rebase master my_branch git checkout master git merge --no-ff my_branch gitlab supports doing this via the web interface, I don't know about github. Atila
Mar 21
parent Martin Nowak <code dawg.eu> writes:
On Tuesday, 21 March 2017 at 20:16:00 UTC, Atila Neves wrote:
 git rebase master my_branch
 git checkout master
 git merge --no-ff my_branch
Yes, that's about what we aim for, rebase w/ --autosquash though, so that people can `git commit --fixup` new fixup commits to open PRs w/o leaving noise behind. https://github.com/dlang-bots/dlang-bot/issues/64 Requires a local checkout of the repo which the bot doesn't have atm.
Mar 24