Update commit log template to add placeholder for explanation of why a patch fixes a bug
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 <https://bugs.webkit.org/show_bug.cgi?id=248012 <https://bugs.webkit.org/show_bug.cgi?id=248012>> <https://github.com/WebKit/WebKit/pull/6579 <https://github.com/WebKit/WebKit/pull/6579>> 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? Dave
On Nov 17, 2022, at 12:23 PM, David Kilzer via webkit-dev <webkit-dev@lists.webkit.org> 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 <https://bugs.webkit.org/show_bug.cgi?id=248012 <https://bugs.webkit.org/show_bug.cgi?id=248012>> <https://github.com/WebKit/WebKit/pull/6579 <https://github.com/WebKit/WebKit/pull/6579>>
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
On Nov 17, 2022, at 12:23 PM, David Kilzer via webkit-dev <webkit-dev@lists.webkit.org> 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 <https://bugs.webkit.org/show_bug.cgi?id=248012> <https://github.com/WebKit/WebKit/pull/6579>
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
On Nov 17, 2022, at 12:43 PM, Simon Fraser via webkit-dev <webkit-dev@lists.webkit.org> wrote:
On Nov 17, 2022, at 12:23 PM, David Kilzer via webkit-dev <webkit-dev@lists.webkit.org> 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 <https://bugs.webkit.org/show_bug.cgi?id=248012> <https://github.com/WebKit/WebKit/pull/6579>
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 <mailto:webkit-dev@lists.webkit.org> https://lists.webkit.org/mailman/listinfo/webkit-dev
On Nov 17, 2022, at 2:37 PM, Michael Catanzaro via webkit-dev <webkit-dev@lists.webkit.org> wrote:
On Thu, Nov 17 2022 at 12:23:54 PM -0800, David Kilzer via webkit-dev <webkit-dev@lists.webkit.org> 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
On Thu, Nov 17 2022 at 02:41:35 PM -0800, Ryosuke Niwa <rniwa@apple.com> 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. :)
On Nov 17, 2022, at 2:46 PM, Michael Catanzaro <mcatanzaro@gnome.org> wrote:
On Thu, Nov 17 2022 at 02:41:35 PM -0800, Ryosuke Niwa <rniwa@apple.com> 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
On Thu, Nov 17 2022 at 02:48:04 PM -0800, Ryosuke Niwa <rniwa@apple.com> 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
On Nov 17, 2022, at 3:17 PM, Michael Catanzaro <mcatanzaro@gnome.org> wrote:
On Thu, Nov 17 2022 at 02:48:04 PM -0800, Ryosuke Niwa <rniwa@apple.com> 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
participants (5)
-
David Kilzer
-
John Wilander
-
Michael Catanzaro
-
Ryosuke Niwa
-
Simon Fraser