Bug 29206 – post-diff and post-commits should be able to find bug urls in ChangeLogs.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

post-diff and post-commits should be able to find bug urls in ChangeLogs.

I also cleaned up the code a little.

[Eric Seidel (no email)]

Created attachment 39470 [details]
Patch v1

[Eric Seidel (no email)]

I'm going to test this locally for a bit longer before marking for review.

[Eric Seidel (no email)]

Created attachment 39662 [details]
post-diff cleanups

[Eric Seidel (no email)]

Dave or Dave might be interested in reviewing this.  Eventually we should make land-diff also have this magical "find the bug number" behavior.

[David Kilzer (:ddkilzer)]

Comment on attachment 39662 [details]
post-diff cleanups

>+         - Fallback to bug urls in commit diffs, instead of just in commit messages,
>+           meaning post-commits will now find bug urls in ChangeLogs.

Why do this?  By providing fallback behavior, we don't encourage developers to put the bug number in the commit message.  (Or is this because svn doesn't have the concept of a commit log within its patches?)

[David Kilzer (:ddkilzer)]

Comment on attachment 39662 [details]
post-diff cleanups

> +         - Fallback to bug urls in commit diffs, instead of just in commit messages,
> +           meaning post-commits will now find bug urls in ChangeLogs.

If this is for svn support, then it's fine.  Otherwise, I'd rather enforce using the ChangeLog entries for commit logs.

> @@ -459,11 +459,17 @@ class ObsoleteAttachmentsOnBug(Command):
>  class PostDiffAsPatchToBug(Command):
>      def __init__(self):
>          options = [
> +            make_option("-m", "--description", action="store", type="string", dest="description", help="Description string for the attachment (default: 'patch')"),
> +        ]
> +        options += self.posting_options()
> +        Command.__init__(self, 'Attaches the current working directory diff to a bug as a patch file.', '[BUGID]', options=options)
> +
> +    @staticmethod
> +    def posting_options():
> +        return [
>              make_option("--no-obsolete", action="store_false", dest="obsolete_patches", default=True, help="Do not obsolete old patches before posting this one."),
>              make_option("--no-review", action="store_false", dest="review", default=True, help="Do not mark the patch for review."),
> -            make_option("-m", "--description", action="store", type="string", dest="description", help="Description string for the attachment (default: 'patch')"),
>          ]
> -        Command.__init__(self, 'Attaches the current working directory diff to a bug as a patch file.', 'BUGID', options=options)

Why is the -m|--description option listed separately?  It's also included separately in PostCommitsAsPatchesToBug, so it should be included with posting_options().

> +            # Perfer --bug-id=, then a bug url in the commit message, then a bug url in the entire commit diff (i.e. ChangeLogs).

Typo: "Perfer" should be "Prefer".

r=me

[Eric Seidel (no email)]

(In reply to comment #5)
> (From update of attachment 39662 [details])
> >+         - Fallback to bug urls in commit diffs, instead of just in commit messages,
> >+           meaning post-commits will now find bug urls in ChangeLogs.
> 
> Why do this?  By providing fallback behavior, we don't encourage developers to
> put the bug number in the commit message.  (Or is this because svn doesn't have
> the concept of a commit log within its patches?)

When I'm writing a change in Git to post to bugzilla I don't put the ChangeLog in teh commit log.  Only when I'm landing do I do that.  I don't use commit-log-editor, and perhaps that explains this habit of mine.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 39662 [details] [details])
> > >+         - Fallback to bug urls in commit diffs, instead of just in commit messages,
> > >+           meaning post-commits will now find bug urls in ChangeLogs.
> > 
> > Why do this?  By providing fallback behavior, we don't encourage developers to
> > put the bug number in the commit message.  (Or is this because svn doesn't have
> > the concept of a commit log within its patches?)
> 
> When I'm writing a change in Git to post to bugzilla I don't put the ChangeLog
> in teh commit log.  Only when I'm landing do I do that.  I don't use
> commit-log-editor, and perhaps that explains this habit of mine.

I realized after posting this that if people use bzt to commit (or the commit-queue), it won't really matter, so this is really a moot point.

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 "WebCore:" headings into the first line of text after it (see <http://trac.webkit.org/changeset/48492> for example).

I guess I should file a bug about these differences?

[Eric Seidel (no email)]

(In reply to comment #8)
> 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 "WebCore:"
> headings into the first line of text after it (see
> <http://trac.webkit.org/changeset/48492> for example).
> 
> I guess I should file a bug about these differences?

Yes, this is awful.  No it's not intentional.  bzt's current behavior is simply the easiest thing I could hack together.  I would much rather have them be identical in behavior and share commit-log-editor code between the script and bugzilla-tool.

Eventually we should either find a way to make bugzilla-tool call commit-log-editor to produce the ChangeLog entry, or re-write commit-log-editor as a python module which can be shared by both bugzilla-tool and the script/editor-launcher-thingy.

bug 26755 already covers fixing this, but you should feel encouraged to add more comments there and/or post patches to improve the situation. :)

[Eric Seidel (no email)]

(In reply to comment #6)
> (From update of attachment 39662 [details])
> Why is the -m|--description option listed separately?  It's also included
> separately in PostCommitsAsPatchesToBug, so it should be included with
> posting_options().

They have different default behaviors, which are documented in separate messages.  I could have made a slightly different abstraction, but that's what I came up with for now.

> > +            # Perfer --bug-id=, then a bug url in the commit message, then a bug url in the entire commit diff (i.e. ChangeLogs).
> 
> Typo: "Perfer" should be "Prefer".

Will fix and land.

Thanks!

[Eric Seidel (no email)]

Committed r48614: <http://trac.webkit.org/changeset/48614>