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

[webkit-dev] Update commit log template to make GNU changelog optional

[Michael Catanzaro via webkit-dev]

Hi,

I'd like to remove the GNU changelog from the bottom of the commit 
messages by default. With "by default" I mean people who prefer to use 
the GNU changelog for formatting their commit messages would have to 
change git-webkit configuration to keep using it, and it would go away 
for everyone else's commit messages. I don't see any reason to prohibit 
the changelogs if really desired, but these were designed for an era 
before version control existed, to explain what is being changed rather 
than why. The freeform commit message that we add above the changelogs 
is usually a better way to explain commits.


(Another disadvantage is it is really easy for the changelog to become 
stale by mistake. When amending commits, you have to look closely at 
the bottom of the commit message template prepared by git-webkit to 
notice the updated changelog, then manually replace the original commit 
message's changelog. I'm sure we commit inaccurate changelogs all the 
time because we forget to do this.)


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

[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

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

[David Kilzer via webkit-dev]

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!).

* 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

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