Bug 220822

Summary: The generated commit message have a directory label at the first line rather than the bug's title if ChangeLogs have different bug titles
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: Tools / TestsAssignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED WONTFIX    
Severity: Normal CC: ap, darin, ddkilzer, ews-watchlist, glenn, jbedard, mcatanzaro, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=220950
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Fujii Hironori 2021-01-21 13:30:25 PST
If ChangeLogs have different bug titles, the generated commit
message have a directory label at the first line rather than the
bug's title. This is not Git friendly.
For example, r271701, r271695 and r271690.

Tools/Scripts/webkitpy/common/checkout/checkout.py spawns
'commit-log-editor' Perl script to generate a commit message.
https://github.com/WebKit/WebKit/blob/37b6acb392110b1f3e9db830d4207dadef64dbe0/Tools/Scripts/webkitpy/common/checkout/checkout.py#L140

removeLongestCommonPrefixEndingInNewline detects the longest
common header lines of all ChangeLogs and remove the common part
from them and return the prefix and modified ChangeLogs.
https://github.com/WebKit/WebKit/blob/37b6acb392110b1f3e9db830d4207dadef64dbe0/Tools/Scripts/commit-log-editor#L384
Bug 29190 – commit-log-editor should move common prefixes to the top of the commit log
Comment 1 Alexey Proskuryakov 2021-01-21 17:44:38 PST
This is not Subversion friendly either.

This has to be a regression I presume?
Comment 2 Fujii Hironori 2021-01-24 16:57:06 PST
Created attachment 418245 [details]
Patch
Comment 3 Darin Adler 2021-01-24 16:58:59 PST
(In reply to Alexey Proskuryakov from comment #1)
> This has to be a regression I presume?

I think not. Rather, a case we did not optimize for.
Comment 4 Fujii Hironori 2021-01-24 17:53:18 PST
Created attachment 418248 [details]
Patch
Comment 5 Darin Adler 2021-01-25 09:52:25 PST
Comment on attachment 418248 [details]
Patch

Really need regression tests for this script. review+ on "faith" since I didn’t really study it carefully enough to spot mistakes.
Comment 6 Fujii Hironori 2021-01-25 11:59:17 PST
Comment on attachment 418248 [details]
Patch

Clearing flags on attachment: 418248

Committed r271805: <https://trac.webkit.org/changeset/271805>
Comment 7 Fujii Hironori 2021-01-25 11:59:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2021-01-25 12:01:32 PST
<rdar://problem/73581935>
Comment 9 Fujii Hironori 2021-01-25 12:19:19 PST
A Git pull request has a commit message.
I hope WebKit will deprecate ChangeLog after the Git migration.
Comment 10 Darin Adler 2021-01-25 13:30:06 PST
(In reply to Fujii Hironori from comment #9)
> A Git pull request has a commit message.
> I hope WebKit will deprecate ChangeLog after the Git migration.

The social contract that goes with our decision of including change logs vs. relying only on commit messages (and note that Subversion has commit messages just like Git does and always has, as did CVS before it) goes far beyond the considerations in making this script work well.
Comment 11 Ryan Haddad 2021-01-25 15:14:17 PST
Reverted r271805 for reason:

Appears to have broken generation of changed files/functions in commit logs

Committed r271865: <https://trac.webkit.org/changeset/271865>
Comment 12 Ryan Haddad 2021-01-25 15:15:17 PST
(In reply to Ryan Haddad from comment #11)
> Reverted r271805 for reason:
> 
> Appears to have broken generation of changed files/functions in commit logs
It also caused some webkitpy test failures, documented in https://bugs.webkit.org/show_bug.cgi?id=220950
Comment 13 Darin Adler 2021-01-25 15:39:48 PST
Besides the test failures, this caused problems for some programmers at Apple who found the script wasn’t including files at all any more in their configuration. There’s a chance that’s specific to Apple’s internal configuration.
Comment 14 Fujii Hironori 2021-01-25 18:49:30 PST
Created attachment 418363 [details]
Patch
Comment 15 Fujii Hironori 2021-01-25 18:57:57 PST
(In reply to Darin Adler from comment #13)
> Besides the test failures, this caused problems for some programmers at
> Apple who found the script wasn’t including files at all any more in their
> configuration. There’s a chance that’s specific to Apple’s internal
> configuration.

I fixed the test failure. And webkitpy EWS passed.
However, I don't know about the Apple internal issue. It seems that I should give up. I hope someone in Apple would take over this task.
Comment 16 Jonathan Bedard 2021-01-26 08:50:09 PST
(In reply to Fujii Hironori from comment #15)
> (In reply to Darin Adler from comment #13)
> > Besides the test failures, this caused problems for some programmers at
> > Apple who found the script wasn’t including files at all any more in their
> > configuration. There’s a chance that’s specific to Apple’s internal
> > configuration.
> 
> I fixed the test failure. And webkitpy EWS passed.
> However, I don't know about the Apple internal issue. It seems that I should
> give up. I hope someone in Apple would take over this task.

It seems unlikely that the Apple Internal issue is actually special, I suspect that the webkitpy test failure caught the mistake, we just didn't have EWS configured properly to catch it (Aakash Jain fixed that yesterday).

Will double-check today what the specifics of the Apple Internal issue was.
Comment 17 Fujii Hironori 2022-05-22 13:48:23 PDT
ChangeLog has been deprecated. Closed as WONTFIX.