Re: Phab: conditional approval

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