[asterisk-dev] Tagging commits with issue numbers

Fri Aug 19 18:14:32 CDT 2011

On Tue, Aug 16, 2011 at 11:31:58AM -0400, Russell Bryant wrote:
> On Tue, Aug 16, 2011 at 11:28 AM, Kevin P. Fleming <kpfleming at digium.com> wrote:
> > In the past (when we were using Mantis), the policy was that only commits
> > which were intended to "close" an issue got tagged with the issue number; a
> > bot then noticed the commit, copied the commit message into a note on the
> > issue, and closed it (if it was open).
> 
> We actually did both.  "issue #foo" would leave a comment on a mantis
> issue.  "closes issue #foo" would leave and comment and close it.
> 
> > Now that we've switched to JIRA, and we are using the 'Subversion' plugin
> > for JIRA, *all* commits associated with an issue can appear on the issue (in
> > a separate tab, so they don't clutter the normal comments tab). As a result,
> > it would be beneficial if *all* commits associated with an issue included
> > the issue number. If a commit is made, and then subsequently other commits
> > are made to correct it, improve it, etc. (but still to address the same
> > issue), *ALL* of those commits should include the issue number(s) in the
> > commit message.
> >
> > In fact, we can even go a step further: if a commit is made in order to
> > correct a problem caused by work related to a previously resolved issue,
> > including that previous issue number in the commit message will cause that
> > commit to show up on both the new and old issues... so if anyone is
> > reviewing the old issue and looking at commits related to it, they'll not
> > only see the initial work that was done, but the subsequent corrections to
> > that work. At some point we could even modify the plugin to automatically
> > create a 'related to' Issue Link if there isn't one in place already... but
> > let's start with the simpler stuff first :-)
> >
> > Please try to get into the habit of doing this; we'll all benefit. Thanks.
> 
> Related wiki page which may need some updating:
> 
> https://wiki.asterisk.org/wiki/display/AST/Commit+Messages

There are three types of comment formats there:

1. (issue ASTERISK-1234)

2.  Tested by: SomeOtherGuy, SomeOtherGuy2

3. Patches:
     fix_bug_1234.diff uploaded by SomeDeveloper (license 5678)

I wonder how exactly are all of those supposed to be parsed. Comments of
type 1 seem to be originally designed for comments included inline in
the commit message text. In that case the special tag should be allowed
to span multiple lines ("(issue\nASTERISK-1234)"). 'Closes:' comments in
Debian changelog entries are allowed to. However I believe that allowing
the 'issue' tags to wrap makes parsing more complicated.

Another issue is what happens to the commit messages once they are
quoted by svnmerge. Take this recent commit:

http://svnview.digium.com/svn/asterisk?view=revision&revision=332655

Merged revisions 332654 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/10

........
  r332654 | kmoore | 2011-08-19 14:59:34 -0500 (Fri, 19 Aug 2011) | 8 lines

  Make CONFBRIDGE_INFO behave more nicely

  CONFBRIDGE_INFO doesn't behave as well in edge cases as MEETME_INFO.  With this
  patch, CONFBRIDGE_INFO should behave in a much more reasonable manner when
  presented with invalid conferences and keywords.

  Review: https://reviewboard.asterisk.org/r/1359/
........

Note that the commit message does not match /^Review:/ (that is: in the
beginning of the line). If we want to allow that style of quoting, we
should allow the special tags to begin after some white spaces.

This is a simple rule if you just want to parse headers of type (1) and
(2). But what about headers of type (3)? What is the end of a list?

Another issue I'd like to raise is that while the original message
complied with the guidelines, the svnmerge-generated one doesn't.
Its first line is a single and leanthy line that is not a good
description of the patch.

For those of you who don't understand why a summary Here is how a typical
part of the log of trunk looks at my git-svn log browser:

* 82b4726 Merged revisions 332119 via svnmerge from https://origsvn.digium.com/s
* 60ba3c9 Merged revisions 332101 via svnmerge from https://origsvn.digium.com/s
* 6c2184d Merged revisions 332042 via svnmerge from https://origsvn.digium.com/s
* be9650e Merged revisions 332029 via svnmerge from https://origsvn.digium.com/s
* 8418d21 Merged revisions 332027 via svnmerge from https://origsvn.digium.com/s
* 5ab650a Formatting changes while working with DTMF...
* 1bc81db Merged revisions 332022 via svnmerge from https://origsvn.digium.com/s
* 63f66f3 Merged revisions 331956 via svnmerge from https://origsvn.digium.com/s
* 369f4ce Merged revisions 331894 via svnmerge from https://origsvn.digium.com/s
* b4a40bb Merged revisions 331868 via svnmerge from https://origsvn.digium.com/s
* c6d5457 Formatting guideline fixes
* 1b92d5a Merged revisions 331775 via svnmerge from https://origsvn.digium.com/s
* d2298f4 Merged revisions 331772 via svnmerge from https://origsvn.digium.com/s
* d139046 Merged revisions 331644 via svnmerge from https://origsvn.digium.com/s

Compare that to 1.4, that has almost no merges:

* bf510a7 Addresses AST-2011-010, remote crash in IAX2 driver
* 5f3d51b Fix DYNAMIC_FEATURES
* fae908d Resolve a segfault/bus error when we try to map memory that falls on a
* f301af7 unlock pvt when we drop voice frames received in early media when in t
* 5d04fb2 whitespace
* 3bf7d25 don't drop any voice frames when checking for T.38 during early media
* 4f6a908 Solaris compatibility fixes
* f624a73 The meetme CLI command completion leaves conferences mutex locked.
* 11c09a1 chan_sip: Destroy variables on a sip_pvt before copying vars from the
* c37a93a Make sure everyone gets an unhold when a transfer succeeds
* a0c8f28 Fix app_dial ring groups
* a4d0152 Merged revisions 318671 via svnmerge from https://origsvn.digium.com/s
* c8fdf47 Regression after r297603 (Improve handling of REGISTER requests with m
* 46699f4 Re-fix queue round-robin
* 91a8264 chan_sip: fix broken realtime peer count, fix memory leak
* 246f77b Disable console colourization inside safe_asterisk checks.

-- 
               Tzafrir Cohen
icq#16849755              jabber:tzafrir.cohen at xorcom.com
+972-50-7952406           mailto:tzafrir.cohen at xorcom.com
http://www.xorcom.com  iax:guest at local.xorcom.com/tzafrir