[webkit-dev] Comment on the bug & email author/reviewer before reverting a patch

Ryosuke Niwa rniwa at webkit.org
Wed Jul 9 13:43:58 PDT 2014


On Wed, Jul 9, 2014 at 1:35 PM, Brady Eidson <beidson at apple.com> wrote:

>
> On Jul 9, 2014, at 1:15 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>
> On Wed, Jul 9, 2014 at 1:08 PM, Brady Eidson <beidson at apple.com> wrote:
>
>>
>> On Jul 9, 2014, at 12:39 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>>
>> On Wed, Jul 9, 2014 at 12:35 PM, Tim Horton <timothy_horton at apple.com>
>> wrote:
>>
>>>
>>> On Jul 9, 2014, at 12:10 PM, Maciej Stachowiak <mjs at apple.com> wrote:
>>>
>>>
>>> Could we teach webkitbot to do an appropriate notification with a
>>> waiting period? Either as part of rollout or add a new command to do it.
>>>
>>>
>>> It already does. The "waiting period" is defined by when the person who
>>> asked for the rollout sets the cq+ bit on the rollout patch.
>>>
>>
>> I don't think creating a rollout patch should be the standard method of
>> notifying the author/reviewer.  We should be informing the author/reviewer
>> ahead of the time.
>>
>>
>> We already have an automated tool that quickly and easily notifies the
>> author/reviewer, and that tool also happens to create the rollout patch.
>>
>> As Tim points out, the rollout patch is never landed unless a reviewer
>> (usually the person who created the rollout patch) sets the cq+ bit on it.
>>
>> I don't see what negative effect the mere existence of the rollout patch
>> has, or why we should codify into the process that a rollout patch is *not*
>> created when notifying the author/reviewer.
>>
>
> When the bug for a rollout is created, the original bug is automatically
> reopened.
>
>
> Which makes sense when a patch breaks something, whether the resolution is
> the author following up with a fix *or* the rollout committing.'
>
> This is not a reason to avoid creating a rollout patch.
>
> Also, the bot doesn't provide enough information as to what's breaking
> because it only takes a single line of description on IRC.
>
>
> This seems like a complaint you have with the tool that can be fixed. This
> is not a reason to avoid creating a rollout patch.
>

This is not a complaint about the tool.  In practice, the bot can't figure
out why a given patch needs to be rolled out.  It's the responsibility of
the person who is rolling out the patch to give necessary details.

> It's crucial that whoever reverting a patch provide a detailed explanation
> on what build or test failed and provide a hyper link to build.webkit.org.
>  Otherwise the original author and the reviewer may have no idea what went
> wrong.
>
> This statement seems at odds with how webkitbot (or an earlier form
> thereof) has been used countless times, since it has been reverting patches
> with only 1-line explanations for years without an uproar.
>

Not at all.  The point is that the person who requested to rollout a patch
should provide the detailed explanation as to why the patch has to be
rolled out, or exactly what got broken by the patch.

If the premise of this email thread is "please provide a detailed
> description of why a patch is a candidate to be rolled out, including a
> link to the build/test failures", then I wholeheartedly agree that
> webkitbot should be enhanced to allow and encourage this.
>

Giving a detailed description has already been a prerequisite to revert a
patch.  I don't see why we need to enhance the tool to continue doing what
we have always done.  If you want to enhance the tool to help this process,
please go ahead but I'm not singing up to do that work.

- R. Niwa
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-dev/attachments/20140709/056ad9af/attachment.html>


More information about the webkit-dev mailing list