On 8 March 2013 03:27, Walter Bright <newshound2@digitalmars.com> wrote:

> On 3/7/2013 3:55 PM, Leandro Lucarella wrote:
>
>> Is nice to see the 2 main contributors of the 2 other compiler implementations with some official status!
>>
>
> It's hard to think of someone having better credentials!
>
>
With a status of "His Honor the GNU D Compiler Guru"  :o)


-- 
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';

On 3/8/13 3:33 AM, Dmitry Olshansky wrote:
> There is got to be more effective process to merge stuff. Current
> situation involves ping-pong between commiter/reviewer and contributor
> on every minor nit there is. That basically involves reviewing the same
> exact code few times over as cleanup arrives some days later. And even
> when contributor think he did cleanup something, he/she may as well miss
> what's the deal and the cycle repeats.
>
> Instead it's definitely possible for committer to checkout the pull, do
> an extra cleanup commit (with automatic tool possibly, like detab/toln
> and I'd love to see official "indent" for D) and push it to the main
> repo. (Or squash the commits. This doesn't cancel out reviewing anything
> non-trivial by at least 2 persons.)

We could experiment with this, but I'm skeptical. At Facebook we have that option (called "commandeering" a revision) but it's very rarely used.

One issue that I do think is holding us back is the diff viewer. Phabricator has a great one (side by side, not interleaved), and it would be great if we could have the same. One possibility would be to integrate smoothly with meld.


Andrei

On Friday, 8 March 2013 at 12:58:56 UTC, Andrei Alexandrescu wrote:
> On 3/8/13 3:33 AM, Dmitry Olshansky wrote:
>> There is got to be more effective process to merge stuff. Current
>> situation involves ping-pong between commiter/reviewer and contributor
>> on every minor nit there is. That basically involves reviewing the same
>> exact code few times over as cleanup arrives some days later. And even
>> when contributor think he did cleanup something, he/she may as well miss
>> what's the deal and the cycle repeats.
>>
>> Instead it's definitely possible for committer to checkout the pull, do
>> an extra cleanup commit (with automatic tool possibly, like detab/toln
>> and I'd love to see official "indent" for D) and push it to the main
>> repo. (Or squash the commits. This doesn't cancel out reviewing anything
>> non-trivial by at least 2 persons.)
>
> We could experiment with this, but I'm skeptical. At Facebook we have that option (called "commandeering" a revision) but it's very rarely used.
>
> One issue that I do think is holding us back is the diff viewer. Phabricator has a great one (side by side, not interleaved), and it would be great if we could have the same. One possibility would be to integrate smoothly with meld.
>
>
> Andrei

git difftool?

Dmitry Olshansky, el  8 de March a las 12:33 me escribiste:
> There is got to be more effective process to merge stuff. Current situation involves ping-pong between commiter/reviewer and contributor on every minor nit there is. That basically involves reviewing the same exact code few times over as cleanup arrives some days later. And even when contributor think he did cleanup something, he/she may as well miss what's the deal and the cycle repeats.
> 
> Instead it's definitely possible for committer to checkout the pull, do an extra cleanup commit (with automatic tool possibly, like detab/toln and I'd love to see official "indent" for D) and push it to the main repo. (Or squash the commits. This doesn't cancel out reviewing anything non-trivial by at least 2 persons.)

I think this is the wrong approach. People need to learn how to do
a proper pull request, you can't get the committers doing cleaning work
after contributors. Is a learning process, once you get it right, your
pull requests shouldn't too many cycles to get accepted.

-- 
Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
Algún día los libros desterrarán a la radio y el hombre descubrirá el
oculto poder del Amargo Serrano.
	-- Ricardo Vaporeso. El Bolsón, 1909.

08-Mar-2013 18:32, Leandro Lucarella пишет:
> Dmitry Olshansky, el  8 de March a las 12:33 me escribiste:
>> There is got to be more effective process to merge stuff. Current
>> situation involves ping-pong between commiter/reviewer and
>> contributor on every minor nit there is. That basically involves
>> reviewing the same exact code few times over as cleanup arrives some
>> days later. And even when contributor think he did cleanup
>> something, he/she may as well miss what's the deal and the cycle
>> repeats.
>>
>> Instead it's definitely possible for committer to checkout the pull,
>> do an extra cleanup commit (with automatic tool possibly, like
>> detab/toln and I'd love to see official "indent" for D) and push it
>> to the main repo. (Or squash the commits. This doesn't cancel out
>> reviewing anything non-trivial by at least 2 persons.)
>
> I think this is the wrong approach. People need to learn how to do
> a proper pull request, you can't get the committers doing cleaning work
> after contributors. Is a learning process, once you get it right, your
> pull requests shouldn't too many cycles to get accepted.
>

I would have agreed if it were not for these things:

a) The learning process is an evolution of knowledge, thus you've got to be nice to newbies. This means trying to not overwhelm them with minor details, fascist style requirements and whatnot on the first pulls. (+ learn by example - show the commit that cleans things up and insist on following conventions etc. next time around)

b) The time the pull takes to be accepted is no less then one month unless it gets pulled in during the first few days. Typical RTT between committer-contributor of up to one-two week(s).

c) Committer already spent as much (if not more) time by looking through the pull. Doing a trivial cleanup after that should be a semi-automated five minute job.

-- 
Dmitry Olshansky

08-Mar-2013 16:58, Andrei Alexandrescu пишет:
> On 3/8/13 3:33 AM, Dmitry Olshansky wrote:
>> There is got to be more effective process to merge stuff. Current
>> situation involves ping-pong between commiter/reviewer and contributor
>> on every minor nit there is. That basically involves reviewing the same
>> exact code few times over as cleanup arrives some days later. And even
>> when contributor think he did cleanup something, he/she may as well miss
>> what's the deal and the cycle repeats.
>>
>> Instead it's definitely possible for committer to checkout the pull, do
>> an extra cleanup commit (with automatic tool possibly, like detab/toln
>> and I'd love to see official "indent" for D) and push it to the main
>> repo. (Or squash the commits. This doesn't cancel out reviewing anything
>> non-trivial by at least 2 persons.)
>
> We could experiment with this, but I'm skeptical. At Facebook we have
> that option (called "commandeering" a revision) but it's very rarely used.

I think the skepticism is misplaced. Of course, inside of any large organization there is generally some coding culture (style guide, strict rules etc.) that one may expect to be followed. Recalling your comment on e.g. 80-colum as a hard limit enforced by the commit hook and such, I understand that at Facebook you don't even have to see these minor flaws at all.

The above is not the case in the highly scattered D community. Above all the person you nag about a misplaced whitespace of something like that most definitely could have completely different timezone. Even worse he/she may have a couple of deadlines to meet, be traveling over sea, has a sick dog to treat or whatever other urgent things to attend to. Even under normal conditions I expect no less then 8 hours for a random comment to penetrate.

> One issue that I do think is holding us back is the diff viewer.
> Phabricator has a great one (side by side, not interleaved), and it
> would be great if we could have the same. One possibility would be to
> integrate smoothly with meld.

Having a better tool then Github's online diff would be nice if that said tool could be integrated somewhere close to github's pull view. Just like say pull tester is.

Otherwise setting up a script on a local machine that grabs few latest pulls and then cycles through them with git difftool would work. And meld works with git.


-- 
Dmitry Olshansky

On 3/8/13 10:53 AM, Dmitry Olshansky wrote:
> I think the skepticism is misplaced.

Feel free to experiment with it. The problem with this approach is people will post increasingly sloppy code on the account that reviewers must fix it. I'm glad we're well past the point where people shove essentially unfinished work with the subtext "well here's as far as I got, feel free to take it over from here".

At the level of professionalism and commitment necessary for contributing to D, I don't think asking for crisp diffs is much of a drag.

I won't reply to this any further. As long as it's just a hypothesis, arguments pro and con may go on forever. Again, experiment with it; if successful, then we can adopt it.

BTW ironically only today I've done such a thing: https://github.com/D-Programming-Language/d-programming-language.org/pull/297. The change was minor; but by doing that systematically we create precedent for less parallelization of work and more work for bottleneck contributors.


Andrei

08-Mar-2013 20:19, Andrei Alexandrescu пишет:
> On 3/8/13 10:53 AM, Dmitry Olshansky wrote:
>> I think the skepticism is misplaced.
>
> Feel free to experiment with it.

AFAIK I can't if we are speaking of dmd/druntime/phobos/etc ...

> The problem with this approach is
> people will post increasingly sloppy code on the account that reviewers
> must fix it. I'm glad we're well past the point where people shove
> essentially unfinished work with the subtext "well here's as far as I
> got, feel free to take it over from here".
>
> At the level of professionalism and commitment necessary for
> contributing to D, I don't think asking for crisp diffs is much of a drag.
>
> I won't reply to this any further. As long as it's just a hypothesis,
> arguments pro and con may go on forever. Again, experiment with it; if
> successful, then we can adopt it.

At the moment I could just suggest for others to try which I did. I guess time will tell if it's viable or not.

[snip]
-- 
Dmitry Olshansky

On 3/8/2013 1:49 AM, Iain Buclaw wrote:
> With a status of "His Honor the GNU D Compiler Guru"  :o)

Yes, your Worship!

On 3/8/2013 7:31 AM, Dmitry Olshansky wrote:
> c) Committer already spent as much (if not more) time by looking through the
> pull. Doing a trivial cleanup after that should be a semi-automated five minute
> job.

It doesn't scale to have the committers be expected to fix up the pull requests.