Phab: conditional approval

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Phab: conditional approval

Richard Eisenberg-4
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?

Thanks,
Richard
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Phab: conditional approval

Ben Gamari-2
Richard Eisenberg <[hidden email]> writes:

> 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?
>
I have also wanted such an option in the past. In general I think the
accept/request changes responses are a bit coarse-grained: I sometimes
need to hold off on accepting patches merely to ensure that others have
a chance to review before merge.

We could open a ticket with Phacility asking whether a finer mechanism
is something they would entertain.

Cheers,

- Ben


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Phab: conditional approval

Simon Marlow-7
In reply to this post by Richard Eisenberg-4
On 19 August 2017 at 03:56, Richard Eisenberg <[hidden email]> 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?

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

Cheers
Simon 
 
Thanks,
Richard
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Phab: conditional approval

Ben Gamari-2
Simon Marlow <[hidden email]> writes:

> On 19 August 2017 at 03:56, Richard Eisenberg <[hidden email]> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Phab: conditional approval

Richard Eisenberg-4
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 <[hidden email]> wrote:
>
> Simon Marlow <[hidden email]> writes:
>
>> On 19 August 2017 at 03:56, Richard Eisenberg <[hidden email]> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Phab: conditional approval

Alan & Kim Zimmerman
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 <[hidden email]> 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 <[hidden email]> wrote:
>
> Simon Marlow <[hidden email]> writes:
>
>> On 19 August 2017 at 03:56, Richard Eisenberg <[hidden email]> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs