Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[David Kilzer via webkit-dev]

On Nov 17, 2022, at 3:17 PM, Michael Catanzaro  wrote:

> On Thu, Nov 17 2022 at 02:48:04 PM -0800, Ryosuke Niwa  
> wrote:
>> But every change in WebKit comes with a Bugzilla bug.
> 
> Certainly most do, but some counterexamples:
> 
> * Unreviewed build fixes sometimes do not reference a bug
> * When fixing a new compiler warning or build failure, I often reference the 
> bug that introduced the problem, rather than reporting a new one just to have 
> a new hyperlink for the commit message
> * We probably don't need a bug report for stuff like "update my email address 
> in contributors.json" or "fix typo in comment”

Yes, this is why I stated this in the original message:

>>> Note that the line may be simply be deleted when it doesn’t apply (such as 
>>> gardening, a build fix, etc.).


I don’t think it’s too burdensome to delete a couple lines of text for the 
above kinds of fixes if the result is better commit messages overall.

Dave

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[Michael Catanzaro via webkit-dev]

On Thu, Nov 17 2022 at 02:48:04 PM -0800, Ryosuke Niwa 
 wrote:

But every change in WebKit comes with a Bugzilla bug.


Certainly most do, but some counterexamples:

* Unreviewed build fixes sometimes do not reference a bug
* When fixing a new compiler warning or build failure, I often 
reference the bug that introduced the problem, rather than reporting a 
new one just to have a new hyperlink for the commit message
* We probably don't need a bug report for stuff like "update my email 
address in contributors.json" or "fix typo in comment"


Anyway, it doesn't matter terribly much what wording we use.

Michael


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[Ryosuke Niwa via webkit-dev]

> On Nov 17, 2022, at 2:46 PM, Michael Catanzaro  wrote:
> 
> On Thu, Nov 17 2022 at 02:41:35 PM -0800, Ryosuke Niwa  
> wrote:
>> We don´t want a description of what PR is; that´s obvious from diff. We want 
>> a description of why that PR fixes the bug.
> 
> Problem is, that is not very useful when the change is anything other than a 
> change that fixes a bug. :)

But every change in WebKit comes with a Bugzilla bug. If a bug is adding new 
feature, for example, one could still explain why implementing that feature is 
desirable and how it’s implemented.

- R. Niwa

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[Michael Catanzaro via webkit-dev]

On Thu, Nov 17 2022 at 02:41:35 PM -0800, Ryosuke Niwa 
 wrote:
We don’t want a description of what PR is; that’s obvious from 
diff. We want a description of why that PR fixes the bug.


Problem is, that is not very useful when the change is anything other 
than a change that fixes a bug. :)



___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[Ryosuke Niwa via webkit-dev]

> On Nov 17, 2022, at 2:37 PM, Michael Catanzaro via webkit-dev 
>  wrote:
> 
> On Thu, Nov 17 2022 at 12:23:54 PM -0800, David Kilzer via webkit-dev 
>  wrote:
>> Any feedback on this change?
> 
> We could alternatively say "Explanation of this change (OOPS!)" or 
> "Explanation of this commit (OOPS!)" to be a little more general.

That’s strictly worse than “explain why this fixes the bug”.

We don’t want a description of what PR is; that’s obvious from diff. We want a 
description of why that PR fixes the bug.

- R. Niwa

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[Michael Catanzaro via webkit-dev]

On Thu, Nov 17 2022 at 12:23:54 PM -0800, David Kilzer via webkit-dev 
 wrote:

Any feedback on this change?


We could alternatively say "Explanation of this change (OOPS!)" or 
"Explanation of this commit (OOPS!)" to be a little more general.


Michael


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[John Wilander via webkit-dev]

> On Nov 17, 2022, at 12:43 PM, Simon Fraser via webkit-dev 
>  wrote:
> 
>> 
>> On Nov 17, 2022, at 12:23 PM, David Kilzer via webkit-dev 
>>  wrote:
>> 
>> Hi,
>> 
>> The following PR adds placeholder text in the commit log template to remind 
>> authors to explain why a change fixes a bug:
>> 
>> Bug 248012: Update commit message template to request a brief explanation of 
>> why a PR fixes the bug
>> 
>> 
>> 
>> It looks like this:
>> 
>> Need a short description (OOPS!).
>> Need the bug URL (OOPS!).
>> Include a Radar link (OOPS!).
>> 
>> Reviewed by NOBODY (OOPS!).
>> 
>> Short explanation why this fixes the bug (OOPS!).
> 
> I would remove the word “Short”. Sometimes a longer explanation is needed. 
> More that one paragraph is often a good thing (para 1 explains the bug, para 
> 2 explains how the change fixes it).

Good idea to have the placeholder there.  I also thing the word “short” can 
be dropped.

   Regards, John

> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org 
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[Simon Fraser via webkit-dev]

> On Nov 17, 2022, at 12:23 PM, David Kilzer via webkit-dev 
>  wrote:
> 
> Hi,
> 
> The following PR adds placeholder text in the commit log template to remind 
> authors to explain why a change fixes a bug:
> 
> Bug 248012: Update commit message template to request a brief explanation of 
> why a PR fixes the bug
> 
> 
> 
> It looks like this:
> 
> Need a short description (OOPS!).
> Need the bug URL (OOPS!).
> Include a Radar link (OOPS!).
> 
> Reviewed by NOBODY (OOPS!).
> 
> Short explanation why this fixes the bug (OOPS!).

I would remove the word “Short”. Sometimes a longer explanation is needed. More 
that one paragraph is often a good thing (para 1 explains the bug, para 2 
explains how the change fixes it).

Simon



___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[Ryosuke Niwa via webkit-dev]

> On Nov 17, 2022, at 12:23 PM, David Kilzer via webkit-dev 
>  wrote:
> 
> The following PR adds placeholder text in the commit log template to remind 
> authors to explain why a change fixes a bug:
> 
> Bug 248012: Update commit message template to request a brief explanation of 
> why a PR fixes the bug
>  >
>  >
> 
> It looks like this:
> 
> Need a short description (OOPS!).
> Need the bug URL (OOPS!).
> Include a Radar link (OOPS!).
> 
> Reviewed by NOBODY (OOPS!).
> 
> Short explanation why this fixes the bug (OOPS!).
> 
> * path/to/File.cpp:
> (Method::Name):
> 
> Since this is effectively a policy change, Jonathan suggested I post it here 
> first before the change is landed.
> 
> I believe many WebKit contributors already do this anyway, and it’s always 
> been a best-practice to explain why a change fixes a bug (vs. what a patch 
> does, which is usually obvious by reading the code), so this text just serves 
> as a reminder to write a short explanation for every relevant patch.
> 
> Note that the line may be simply be deleted when it doesn’t apply (such as 
> gardening, a build fix, etc.).
> 
> Any feedback on this change?

Seems like a good idea.

- R. Niwa

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev