Bug 27172 – bugzilla-tool: create CommitMessage class

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

Created attachment 32604 [details]
Patch v1

Reviewed by NOBODY (OOPS!).

Create a CommitMessage class to encapsulate related code.

* Scripts/bugzilla-tool:
(bug_id_from_commit_message): Moved to
CommitMessage.parse_bug_id().
(commit_message_for_this_commit): Return a CommitMessage.
(ApplyPatchesFromBug.apply_patches): Use CommitMessage.message().
(LandPatchesFromBugs.build_and_commit): Ditto.
(CommitMessageForCurrentDiff.execute): Ditto.
(PostCommitsAsPatchesToBug.execute): Switched from
Git.commit_message_for_commit() to
Git.commit_message_for_local_commit().  Switched from
bug_id_from_commit_message() to CommitMessage.parse_bug_id().

* Scripts/modules/scm.py:
(first_non_empty_line_after_index): Added.
(CommitMessage.__init__): Added.
(CommitMessage.body): Added.
(CommitMessage.description): Added.
(CommitMessage.message): Added.
(CommitMessage.parse_bug_id): Added.  Moved from
bug_id_from_commit_message() in bugzilla-tool.
(Git.commit_message_for_local_commit): Renamed from
commit_message_for_commit().  Return a CommitMessage.
---
 3 files changed, 87 insertions(+), 21 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 32604 [details]
Patch v1

It's difficult for me to tell where the divide between a CommitMessage class and ChangeLog* classes should be.

 114     return CommitMessage(''.join(changelog_messages).splitlines())
Seems a little odd to take an array of lines, but OK.

Maybe CommitMessage should take an array of ChangeLog paths instead? (eventually probably an array of ChangeLog entries?)

Should we name this bug_id and have it lazily call parse_bug_id?
 426             bug_id = options.bug_id or commit_message.parse_bug_id()

I don't understand this code:
 72         if strip_url:
 73             line = re.sub("^(\s*)<.+> ", "\1", line)

Seems it at least needs a comment.  I assume this is some new prepare-ChangeLog thing which I'm not used to yet?

Some of this code really seems to belong in a ChangeLogEntry class.

Seems odd that these lines don't match:
 81             match = re.search("http\://webkit\.org/b/(?P<bug_id>\d+)", line)
 84             match = re.search(Bugzilla.bug_server_regex + "show_bug\.cgi\?id=(?P<bug_id>\d+)", line)

I don't think this is needed:
 87         return None

I think this is OK.  But you might want to try some of the comments above.  I think much of this will end up in a ChangeLogEntry class instead of in CommitMessage.  CommitMessage seems useful for doing all the commit-log-editor stuff dealing with ChangeLogEntry (or similar) classes.

[Eric Seidel (no email)]

Thanks again for all your hard work on bugzilla-tool.  I'm very glad to see someone else using it.  I'm trying to take a step back for a moment, so I'm glad to see you running with things.

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/45940.

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> (From update of attachment 32604 [details])
> It's difficult for me to tell where the divide between a CommitMessage class
> and ChangeLog* classes should be.
> 
>  114     return CommitMessage(''.join(changelog_messages).splitlines())
> Seems a little odd to take an array of lines, but OK.
> 
> Maybe CommitMessage should take an array of ChangeLog paths instead?
> (eventually probably an array of ChangeLog entries?)

After thinking about this over the weekend, I do think a CommitMessage object should contain a list of ChangeLogEntry objects.  It could then use these objects to create a commit message string.

> Should we name this bug_id and have it lazily call parse_bug_id?
>  426             bug_id = options.bug_id or commit_message.parse_bug_id()

Yes.

> I don't understand this code:
>  72         if strip_url:
>  73             line = re.sub("^(\s*)<.+> ", "\1", line)
> 
> Seems it at least needs a comment.  I assume this is some new prepare-ChangeLog
> thing which I'm not used to yet?

This would strip the URL" off the beginning of a bug description, e.g.:

<rdar://problem/1234567> This is a serious bug
<http://webkit.org/b/12345> This is an even more serious bug

The new "webkit.org/b/NNNNN" URL was discussed on webkit-dev recently.

> Some of this code really seems to belong in a ChangeLogEntry class.

Yes, per above discussion.

> Seems odd that these lines don't match:
>  81             match = re.search("http\://webkit\.org/b/(?P<bug_id>\d+)",
> line)
>  84             match = re.search(Bugzilla.bug_server_regex +
> "show_bug\.cgi\?id=(?P<bug_id>\d+)", line)

The first one parses out the shortened WebKit bug URL format ("webkit.org/b/NNNNN").  The second parses out the bugs.webkit.org URL format.

> I don't think this is needed:
>  87         return None

None is returned implicitly if there is no return statement?

> I think this is OK.  But you might want to try some of the comments above.  I
> think much of this will end up in a ChangeLogEntry class instead of in
> CommitMessage.  CommitMessage seems useful for doing all the commit-log-editor
> stuff dealing with ChangeLogEntry (or similar) classes.

Brent committed this before I had a chance to clear the review flag.  (It doesn't hurt that it landed, though.  I can continue refactoring CommitMessage to use ChangeLogEntry objects.)