Bug 26755 – webkit-patch's commit messages are less readable than commit-log-editor's

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Bugzilla seems to have lost this description due to DB corruption last night.  The original description:

Compare http://trac.webkit.org/changeset/45257
to http://trac.webkit.org/changeset/45188

Two differences:
1. The lack of titles ("WebCore:", etc.)  to tell you what directory the
changelog is for.
2. The sections are sorted in a particular order.

[Eric Seidel (no email)]

This is a known bug.  There is a FIXME about it in the source.  I certainly welcome patches to fix it.  Although once we add this to bugzilla-tool (or really some changelogs.py) then we should consider re-writing commit-log-editor in python, as this I believe is the only missing feature from our python modules that commit-log-editor needs.

[Eric Seidel (no email)]

The source w/ FIXME:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/bugzilla-tool#L97

[David Levin]

Yep,  I know it is a known bug (esp. since I flagged it in review :) ), but thought it worth logging a bug since the tool is being used to land patches that expose this.

[Eric Seidel (no email)]

Totally!  Thanks for filing.

[Mark Rowe (bdash)]

I don't think that we should be exactly matching the format of commit message in <http://trac.webkit.org/changeset/45188>.  In particular, we should avoid having useless info in the first line of the commit.  Hoisting the bug title up to that first line, above the blocks for each directory, would be a good idea.  I also think it's preferable to omit the redundant date / author lines from the commit message when the author is landing their own patches.

[Eric Seidel (no email)]

Ideally what will happen is we'll convert commit-log-editor to a python module and re-use it from within bugzilla-tool.

[David Kilzer (:ddkilzer)]

In Bug 29206 Comment #8, I wrote:

BTW bugzilla-tool still differs from commit-log-editor (see <http://trac.webkit.org/changeset/48503>):

- bzt does't strip spaces to the left of the commit log.
- bzt doesn't change date/name/email lines to subdirectory names.
- bzt puts the LayoutTest/ChangeLog entry first (not last).
- bzt is not putting a blank line between ChangeLog entries.
- bzt doesn't consolidate common lines from each ChangeLog entry to the top of the commit log.

Having said that, I don't like how commit-log-editor squishes the "Directory:" headings into the first line of text after it (see <http://trac.webkit.org/changeset/48492> for example).

[Adam Barth]

Created attachment 44296 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44296 [details] without any errors.

[Adam Barth]

Comment on attachment 44296 [details]
Patch

In trying to write a unit test for this patch, we discovered that commit-log-editor is super fragile and has a number of error conditions that are very complex to recover from.  I no longer think this is the right approach.  We need a simpler way of creating commit messages that is testable.

[Adam Barth]

Created attachment 44297 [details]
Work in progress with unit test

[Adam Roben (:aroben)]

Can we just invoke commit-log-editor?

[Adam Barth]

> Can we just invoke commit-log-editor?

commit-log-editor is not designed in a way that it can be re-used outside of the SVN commit workflow.  One approach is to refactor commit-log-editor to work like a normal UNIX command and then use that core both in the SVN commit workflow and in the webkit-patch workflow.

[Adam Roben (:aroben)]

A lot of the comments in this bug apply to patches with only a single ChangeLog. Retitling to include that case.

[Adam Barth]

> Can we just invoke commit-log-editor?

By the way, we've got enough infrastructure now that we might be able to use this approach.  It might be worth investigating if folks like the format that commit-log-editor produces.

[Adam Roben (:aroben)]

(In reply to comment #15)
> > Can we just invoke commit-log-editor?
> 
> By the way, we've got enough infrastructure now that we might be able to use this approach.  It might be worth investigating if folks like the format that commit-log-editor produces.

Can you elaborate a bit? I'd like to take a stab at fixing this.

[Adam Barth]

> Can you elaborate a bit? I'd like to take a stab at fixing this.

aroben and I chatted a bit about this on IRC.  We talked about a couple different approaches.  Think the one that he seemed most interested in is to refactor commit-log-editor into two scripts: one the prints the commit message to stdout and another that interacts with SVN_EDITOR and svn.

[Adam Roben (:aroben)]

(In reply to comment #17)
> > Can you elaborate a bit? I'd like to take a stab at fixing this.
> 
> aroben and I chatted a bit about this on IRC.  We talked about a couple different approaches.  Think the one that he seemed most interested in is to refactor commit-log-editor into two scripts: one the prints the commit message to stdout and another that interacts with SVN_EDITOR and svn.

Or perhaps one script with two modes.

[Adam Roben (:aroben)]

Created attachment 99967 [details]
Extract commit-log-editor's commit-message-generation code into a separate function

[Adam Roben (:aroben)]

Created attachment 99968 [details]
Add a --print-log option to commit-log-editor

[Adam Roben (:aroben)]

Created attachment 99985 [details]
Add a --print-log option to commit-log-editor

[Adam Roben (:aroben)]

Created attachment 99987 [details]
Teach webkitpy's Checkout class to use commit-log-editor to create commit messages

[WebKit Review Bot]

Attachment 99987 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:87:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:94:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:123:  expected 1 blank line, found 0  [pep8/E301] [5]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 99987 [details]
Teach webkitpy's Checkout class to use commit-log-editor to create commit messages

View in context: https://bugs.webkit.org/attachment.cgi?id=99987&action=review

>> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:87
>> +Tools: 
> 
> trailing whitespace  [pep8/W291] [5]

This is required to match commit-log-editor's output. (We could change commit-log-editor not to output these extra spaces, but that should be done separately.)

>> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:94
>> +LayoutTests: 
> 
> trailing whitespace  [pep8/W291] [5]

Ditto.

>> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:123
>> +        def mock_script_path(script):
> 
> expected 1 blank line, found 0  [pep8/E301] [5]

I'll fix this locally.

[David Kilzer (:ddkilzer)]

Comment on attachment 99967 [details]
Extract commit-log-editor's commit-message-generation code into a separate function

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 99985 [details]
Add a --print-log option to commit-log-editor

View in context: https://bugs.webkit.org/attachment.cgi?id=99985&action=review

r=me, although consider fixing the exit issues.

> Tools/Scripts/commit-log-editor:50
>  sub usage

Consider renaming to printUsageAndExit() so it's more descriptive.

> Tools/Scripts/commit-log-editor:73
> +    exit !$getOptionsResult;

This exit doesn't do anything because usage() already exits.

> Tools/Scripts/commit-log-editor:81
> +        exit 1;

Ditto.

> Tools/Scripts/commit-log-editor:84
> +    exit;

"exit 0;" ?

[Adam Roben (:aroben)]

Comment on attachment 99985 [details]
Add a --print-log option to commit-log-editor

View in context: https://bugs.webkit.org/attachment.cgi?id=99985&action=review

>> Tools/Scripts/commit-log-editor:50
>>  sub usage
> 
> Consider renaming to printUsageAndExit() so it's more descriptive.

Done.

>> Tools/Scripts/commit-log-editor:73
>> +    exit !$getOptionsResult;
> 
> This exit doesn't do anything because usage() already exits.

Good point! Removed.

>> Tools/Scripts/commit-log-editor:81
>> +        exit 1;
> 
> Ditto.

Removed.

>> Tools/Scripts/commit-log-editor:84
>> +    exit;
> 
> "exit 0;" ?

Sure, changed.

[David Kilzer (:ddkilzer)]

Comment on attachment 99987 [details]
Teach webkitpy's Checkout class to use commit-log-editor to create commit messages

View in context: https://bugs.webkit.org/attachment.cgi?id=99987&action=review

r=me

> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:124
> +            return os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', '..', script))

There isn't a better way to get the path to the Tools directory (during a test)?

[Adam Roben (:aroben)]

Comment on attachment 99987 [details]
Teach webkitpy's Checkout class to use commit-log-editor to create commit messages

View in context: https://bugs.webkit.org/attachment.cgi?id=99987&action=review

>> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:124
>> +            return os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', '..', script))
> 
> There isn't a better way to get the path to the Tools directory (during a test)?

Not that I know of. The SCM class knows how to find Tools/Scripts, but it bases it on the repository root, and we're operating in a fake repository here.

[Adam Roben (:aroben)]

Committed r90564: <http://trac.webkit.org/changeset/90564>

[Adam Barth]

Comment on attachment 99987 [details]
Teach webkitpy's Checkout class to use commit-log-editor to create commit messages

View in context: https://bugs.webkit.org/attachment.cgi?id=99987&action=review

> Tools/Scripts/webkitpy/common/checkout/checkout.py:123
> +        message_text = Executive().run_command([self._scm.script_path('commit-log-editor'), '--print-log'] + changelog_paths, return_stderr=False)

This isn't correct.  You need to get the executive from the tool so that it is mocked out correctly in unit tests.

[Adam Barth]

@ddkilzer: This patch contains a couple architectural mistakes.  For example, grabbing the executive statically rather than using the dependency injection layer is a subtle but important violation of an invariant that we try to maintain across the entire codebase.

In the future, it might be better to give someone who works on webkitpy frequently a chance to look at a patch before landing it.

[Eric Seidel (no email)]

Comment on attachment 99987 [details]
Teach webkitpy's Checkout class to use commit-log-editor to create commit messages

View in context: https://bugs.webkit.org/attachment.cgi?id=99987&action=review

> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:85
> +    expected_commit_message = u"""Unreviewed build fix to un-break webkit-patch land.

Some of these intentionally contained unicode.  Do we still correctly support unicode commit messages?

[Adam Roben (:aroben)]

Comment on attachment 99987 [details]
Teach webkitpy's Checkout class to use commit-log-editor to create commit messages

View in context: https://bugs.webkit.org/attachment.cgi?id=99987&action=review

>> Tools/Scripts/webkitpy/common/checkout/checkout.py:123
>> +        message_text = Executive().run_command([self._scm.script_path('commit-log-editor'), '--print-log'] + changelog_paths, return_stderr=False)
> 
> This isn't correct.  You need to get the executive from the tool so that it is mocked out correctly in unit tests.

This code is below the level of the tool. Using it here would be a layering violation, I believe. How do you suggest we make this work?

[Adam Barth]

> This code is below the level of the tool. Using it here would be a layering violation, I believe. How do you suggest we make this work?

The tool has several layers.  For example, Checkout depends on SCM and SCM depends on Executive.  Checkout can either keep its own pointer to Executive or it can use the one that SCM has.  SCM initially gets that pointer from the tool when the tool is initialized.

[Eric Seidel (no email)]

Looks like this broke test-webkitpy on the Gtk bot:
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/24078/steps/webkitpy-test/logs/stdio

It suspect you're using a real executive when you mean to use MockExecutive.

[Adam Roben (:aroben)]

Comment on attachment 99987 [details]
Teach webkitpy's Checkout class to use commit-log-editor to create commit messages

View in context: https://bugs.webkit.org/attachment.cgi?id=99987&action=review

>>> Tools/Scripts/webkitpy/common/checkout/checkout.py:123
>>> +        message_text = Executive().run_command([self._scm.script_path('commit-log-editor'), '--print-log'] + changelog_paths, return_stderr=False)
>> 
>> This isn't correct.  You need to get the executive from the tool so that it is mocked out correctly in unit tests.
> 
> This code is below the level of the tool. Using it here would be a layering violation, I believe. How do you suggest we make this work?

Note also that other code in this module uses the standalone run_command() function. I used Executive().run_command() because run_command() says to do that.

>> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:85
>> +    expected_commit_message = u"""Unreviewed build fix to un-break webkit-patch land.
> 
> Some of these intentionally contained unicode.  Do we still correctly support unicode commit messages?

We do. I filed bug 64109 to prove it.

[Adam Roben (:aroben)]

(In reply to comment #35)
> > This code is below the level of the tool. Using it here would be a layering violation, I believe. How do you suggest we make this work?
> 
> The tool has several layers.  For example, Checkout depends on SCM and SCM depends on Executive.  Checkout can either keep its own pointer to Executive or it can use the one that SCM has.  SCM initially gets that pointer from the tool when the tool is initialized.

OK, I'll fix this in a separate bug. Sorry for the mess.

[Adam Roben (:aroben)]

(In reply to comment #36)
> Looks like this broke test-webkitpy on the Gtk bot:
> http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/24078/steps/webkitpy-test/logs/stdio
> 
> It suspect you're using a real executive when you mean to use MockExecutive.

MockExecutive seems to be part of the tool module. Is it really OK to use that in code inside the common module?

[Adam Barth]

> Note also that other code in this module uses the standalone run_command() function. I used Executive().run_command() because run_command() says to do that.

That code is also wrong.  :)

The code that calls the standalone run_command() function all dates from the beginning of webkitpy, before we had any testing and before we re-designed for testing.  Eventually we'll clean up this code to follow the same invariants as the rest of webkitpy, but that's not been a super high priority.

That said, we don't want to spread this bad pattern to more locations, and when touching any code with the bad pattern, we usually update it to be more modern.  I certainly understand why you wrote the patch the way you did.  This sort of thing is supposed to be flagged in the review process.

[Adam Barth]

> MockExecutive seems to be part of the tool module. Is it really OK to use that in code inside the common module?

MockExecutive should move into the common module.  If you don't want to do that in this patch, there's already MockExecutive2 in the common package, which is helpful for some kinds of tests.  (I'm not entirely sure why we have two kinds of MockExecutives.)

[Adam Roben (:aroben)]

(In reply to comment #41)
> > MockExecutive seems to be part of the tool module. Is it really OK to use that in code inside the common module?
> 
> MockExecutive should move into the common module.  If you don't want to do that in this patch, there's already MockExecutive2 in the common package, which is helpful for some kinds of tests.  (I'm not entirely sure why we have two kinds of MockExecutives.)

Both MockExecutive and MockExecutive2 just return canned output. Is that really what we want?

[Adam Barth]

> Both MockExecutive and MockExecutive2 just return canned output. Is that really what we want?

For most tests, yes.  If you specifically want to test integration with commit-log-editor, we can do that by explicitly passing a live Executive in the test.  Generally, we avoid unit tests that touch the real disk or spawn processes whenever possible.  We have a handful to make sure we continue to integrate correctly with external dependencies, but we don't need to test commit-log-editor exhaustively in test-webkitpy (we should just test that it works at all).

[Adam Roben (:aroben)]

(In reply to comment #36)
> Looks like this broke test-webkitpy on the Gtk bot:
> http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/24078/steps/webkitpy-test/logs/stdio

Bug 64113 will fix the GTK bot regardless of whether we end up using a real Executive or not in the test.

[Adam Roben (:aroben)]

(In reply to comment #43)
> > Both MockExecutive and MockExecutive2 just return canned output. Is that really what we want?
> 
> For most tests, yes.  If you specifically want to test integration with commit-log-editor, we can do that by explicitly passing a live Executive in the test.  Generally, we avoid unit tests that touch the real disk or spawn processes whenever possible.  We have a handful to make sure we continue to integrate correctly with external dependencies, but we don't need to test commit-log-editor exhaustively in test-webkitpy (we should just test that it works at all).

I think it makes sense to test the integration with commit-log-editor here. So here's my plan:

1) Make Checkout use SCM's Executive instance instead of conjuring one out of thin air
2) Make the mock SCM I create in the test use a real Executive instance

I'll file a new bug for this.

[Adam Barth]

Sounds like a good plan.  Thanks.

[Adam Roben (:aroben)]

(In reply to comment #45)
> (In reply to comment #43)
> > > Both MockExecutive and MockExecutive2 just return canned output. Is that really what we want?
> > 
> > For most tests, yes.  If you specifically want to test integration with commit-log-editor, we can do that by explicitly passing a live Executive in the test.  Generally, we avoid unit tests that touch the real disk or spawn processes whenever possible.  We have a handful to make sure we continue to integrate correctly with external dependencies, but we don't need to test commit-log-editor exhaustively in test-webkitpy (we should just test that it works at all).
> 
> I think it makes sense to test the integration with commit-log-editor here. So here's my plan:
> 
> 1) Make Checkout use SCM's Executive instance instead of conjuring one out of thin air
> 2) Make the mock SCM I create in the test use a real Executive instance
> 
> I'll file a new bug for this.

Bug 64115.

[David Kilzer (:ddkilzer)]

(In reply to comment #32)
> @ddkilzer: This patch contains a couple architectural mistakes.  For example, grabbing the executive statically rather than using the dependency injection layer is a subtle but important violation of an invariant that we try to maintain across the entire codebase.

Where is this documented?  How is it enforced (other than patch reviews)?  Where would one go to learn about this?  Does one need to read the entire webkitpy (test) codebase to soak up this knowledge?

> In the future, it might be better to give someone who works on webkitpy frequently a chance to look at a patch before landing it.

Will do.  But I think it should be a goal of WebKit to make it easy to write tests (whether they're layout tests, webkitpy tests or webkitperl tests) so that more people can contribute without investing the better part of a day studying existing code for "subtle invariants" or requiring that one of only a handful of people review the patches.

[Adam Barth]

> Where is this documented?

It's not document.

> How is it enforced (other than patch reviews)?

It's enforced by patch reviews.

> Where would one go to learn about this?

By writing patches for webkitpy and learning by interacting with reviewers.

> Does one need to read the entire webkitpy (test) codebase to soak up this knowledge?

How does one soak up knowledge about other parts of the codebase?  We generally ask that folks be active contributors to an area of code before reviewing patches in that area.

> Will do.  But I think it should be a goal of WebKit to make it easy to write tests (whether they're layout tests, webkitpy tests or webkitperl tests) so that more people can contribute without investing the better part of a day studying existing code for "subtle invariants" or requiring that one of only a handful of people review the patches.

That's what the review process is for.  The error is very easy to spot for folks who review and write code in this area often.  It's also easy to fix once spotted.  What's problematic is for folks to review patches in areas that they don't understand throughly.

[David Kilzer (:ddkilzer)]

(In reply to comment #49)
> > Will do.  But I think it should be a goal of WebKit to make it easy to write tests (whether they're layout tests, webkitpy tests or webkitperl tests) so that more people can contribute without investing the better part of a day studying existing code for "subtle invariants" or requiring that one of only a handful of people review the patches.
> 
> That's what the review process is for.  The error is very easy to spot for folks who review and write code in this area often.  It's also easy to fix once spotted.  What's problematic is for folks to review patches in areas that they don't understand throughly.

I think it would also be beneficial if these "subtle invariants" could be made more explicit through either assertions (assuming Python has such a mechanism; maybe throwing exceptions?) or changes in the structure of the code to make it much harder to write incorrect code (which is likely harder in an interpreted scripting language than C++, but maybe still possible?).

Such changes would also decrease the amount of time it takes to explain these concepts when someone wants to contribute to the tools, since new tests would break immediately in a working copy if they violated the invariants.

[Adam Barth]

I don't think that's possible in this case, but if it is, please let me know how!

[Stephen White]

I notice that the ChangeLog entries on trac.webkit.org after r90564 seem to no longer be pretty-printed (ie., no indentation, no * -> bullets, etc).  Is it possible this change introduced a regression in that functionality?

[Adam Roben (:aroben)]

(In reply to comment #52)
> I notice that the ChangeLog entries on trac.webkit.org after r90564 seem to no longer be pretty-printed (ie., no indentation, no * -> bullets, etc).  Is it possible this change introduced a regression in that functionality?

That is most definitely caused by this change. The lack of indentation is supposed to be a feature, not a bug, and is also what leads to * not being turned into a bullet. Maybe there's some better commit message format we can use, or maybe we can modify Trac to understand our commit messages better? I encourage you to file a new bug.

[Stephen White]

(In reply to comment #53)
> (In reply to comment #52)
> > I notice that the ChangeLog entries on trac.webkit.org after r90564 seem to no longer be pretty-printed (ie., no indentation, no * -> bullets, etc).  Is it possible this change introduced a regression in that functionality?
> 
> That is most definitely caused by this change. The lack of indentation is supposed to be a feature, not a bug, and is also what leads to * not being turned into a bullet. Maybe there's some better commit message format we can use, or maybe we can modify Trac to understand our commit messages better? I encourage you to file a new bug.

Done; filed as https://bugs.webkit.org/show_bug.cgi?id=64307.