Phab: conditional approval

[Ben Gamari]

Simon Marlow <marlowsd at gmail.com> writes:

> On 19 August 2017 at 03:56, Richard Eisenberg <rae at cs.brynmawr.edu> 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 487 bytes
Desc: not available
URL: <http://mail.haskell.org/pipermail/ghc-devs/attachments/20170913/20f99d91/attachment.sig>