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.

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.

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

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.

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.

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

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.

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

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

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.