Simon Marlow writes:

On 19 August 2017 at 03:56, Richard Eisenberg wrote:

...

Hi devs,

When reviewing a diff on Phab, I can "accept" or "request changes". Sometimes, though, I want to do both: I suggest very minor (e.g., typo) changes, but then when these changes are made, I accept. I'm leery of making the suggestions and saying "accept", because then someone working quickly may merge without noticing the typos. Does Phab have such an option?

"Accept with nits" is standard practice, but you're right it can go wrong when someone else is merging accepted diffs. We could adopt a standard comment keyword, e.g. "NITS" that indicates you'd like the nits to be fixed before committing, perhaps?

Sounds reasonable to me.

Also, I don't think it's a good idea to merge commits when the author is a committer, they can land themselves.

I would be quite happy to not have to merge such patches; I merely merge them currently since I thought it was generally expected. On the other hand, I generally do integration builds on the batches of patches that I merge which can sometimes catch validation issues. However, I expect this will be less of an issue with the test-before-merge support in the pipeline. Cheers, - Ben

William Casarin recently tweeted a link to the bitcoincore devs ACK system[1], which are Concept ACK - Agree with the idea and overall direction, but haven't reviewed the code changes or tested them. utACK (untested ACK) - Reviewed and agree with the code changes but haven't actually tested them. Tested ACK - Reviewed the code changes and have verified the functionality or bug fix. ACK - A loose ACK can be confusing. It's best to avoid them unless it's a documentation/comment only change in which case there is nothing to test/verify; therefore the tested/untested distinction is not there. NACK - Disagree with the code changes/concept. Should be accompanied by an explanation. [1] https://github.com/bitcoin/bitcoin/issues/6100 Alan On 14 September 2017 at 18:37, Richard Eisenberg wrote:

Yes, this works for me.

As for merging, I'm always very grateful when Ben does it -- though I agree that it would make more sense for me to do it when I can test-then-merge.

Thanks, Richard

...

On Sep 13, 2017, at 10:29 AM, Ben Gamari wrote:

Simon Marlow writes:

...

On 19 August 2017 at 03:56, Richard Eisenberg wrote:

...

Hi devs,

When reviewing a diff on Phab, I can "accept" or "request changes". Sometimes, though, I want to do both: I suggest very minor (e.g., typo) changes, but then when these changes are made, I accept. I'm leery of making the suggestions and saying "accept", because then someone working quickly may merge without noticing the typos. Does Phab have such an option?

"Accept with nits" is standard practice, but you're right it can go wrong when someone else is merging accepted diffs. We could adopt a standard comment keyword, e.g. "NITS" that indicates you'd like the nits to be fixed before committing, perhaps?

Sounds reasonable to me.

...

Also, I don't think it's a good idea to merge commits when the author is a committer, they can land themselves.

I would be quite happy to not have to merge such patches; I merely merge them currently since I thought it was generally expected.

On the other hand, I generally do integration builds on the batches of patches that I merge which can sometimes catch validation issues. However, I expect this will be less of an issue with the test-before-merge support in the pipeline.

Cheers,

- Ben

_______________________________________________ ghc-devs mailing list ghc-devs@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs