[asterisk-dev] [Code Review] 3554: repotools: Get rid of duplicate JIRA-1234 #comments

Tue May 27 09:43:32 CDT 2014

> On May 22, 2014, 11:41 a.m., rmudgett wrote:
> > Only the #close should be used to auto-close issues.  Granted, it only needs to be done once because JIRA complains that it cannot close the issue because it is already closed.  (More stupid JIRA behavior :) )
> > 
> > The #comment should *not* be used for the reported by information for several reasons.  One, it is not the format used by the existing release scripts.  Two, the JIRA issue alaready has that information.  Three, you get the comment repeated for every merge.
> 
> wdoekes wrote:
>     Fine with me. However, when reading this post, I got a different impression:
>     http://lists.digium.com/pipermail/asterisk-dev/2014-March/066185.html
>     
>     > The biggest implication of changing the commit message format is
>     > making sure that patch submitters are properly attributed, such that
>     > the release summary scripts and other parts of the Asterisk project
>     > pick up who actually wrote the patch. Right now, I'm proposing
>     > something like the following:
>     > 
>     > ASTERISK-12345 #close
>     > ASTERISK-12345 #comment patch my_awesome_fix.diff submitted by jdoe (license 12345)
>     
>     And I believe I'm not the only one. Considering the other other commits I've seen recently. E.g.:
>     
>     > ------------------------------------------------------------------------
>     > r411375 | mjordan | 2014-03-28 04:55:26 +0100 (vr, 28 mrt 2014) | 21 lines
>     > 
>     > chan_sip: Add MESSAGE request to allowed methods
>     > 
>     > The allowed methods advertised by chan_sip did not previously note the MESSAGE
>     > request. Even in Asterisk 1.8, we do accept in-dialog MESSAGE requests; we
>     > should advertise that we support MESSAGE requests.
>     > 
>     > ASTERISK-23504 #close
>     > ASTERISK-23504 #comment Reported by: Martin Kontsek
>     > ASTERISK-23504 #comment Patch sip.h_patch.diff uploaded by Martin Kontsek (license 6587)
>     > 
>     > Review: https://reviewboard.asterisk.org/r/3396/
>     
>     So, perhaps a followup on that March post is in order..
>     
>     Matt? :)
>     
>

Mea culpa.

So, the plan was definitely to use the #comment to note the reporter, tester, and patches. Unfortunately, it ran into the problem you're solving here, where the comment gets detected multiple times. At the time, it seemed liked we would need to move all of the smart commit tags over to using a different nomenclature. Smart commits will recognize any reference to a JIRA issue as a 'link' - so this:

ipsum factor ASTERISK-1234 blah yackity schmackity

Will result in ASTERISK-1234 getting updated with the commit, in the same fashion as an actual "explicit" linkage. At the time, it felt like that could cause problems unless there was some other piece of data that the release scripts could search on, such as a #comment.

As it turns out, I'm not sure changing the metadata - other than "#close" - was actually necessary. It ended up being easier to update the release scripts than I anticipated, and most of the attribution is only necessary when there is an explicit "#close", which makes it easy to key on.

All of that aside, the patch here is still good. The #comment feature is still useful, and this prevents duplicate comments from being left when we make use of it. I'll respond to the asterisk-dev list with an update on the commit message, and make sure the repotools/wiki is updated correctly as well.

Sorry for the confusion here - this should have gotten caught by me sooner.

- Matt

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3554/#review11960
-----------------------------------------------------------

On May 23, 2014, 5:23 a.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3554/
> -----------------------------------------------------------
> 
> (Updated May 23, 2014, 5:23 a.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Repository: Repotools
> 
> 
> Description
> -------
> 
> When merging across branches, the commit message is duplicated using the mergeXY
> command.
> 
> Unfortunately, when using the new JIRA-style ASTERISK-1234 #comments, this comment
> is now reported as many times as the merges are done.
> 
> See for example the last 4 comments of:
> https://issues.asterisk.org/jira/browse/ASTERISK-23650
> 
> 
> This patch prepends a pound (#) to the ISSUE_KEY so that it is (hopefully) not picked
> up a second time by JIRA and we only get a single comment from the primary commit.
> 
> (Also began to run the flake8 checker on svnmerge, but that turned out to be a bad
> idea.)
> 
> 
> Diffs
> -----
> 
>   /svnmerge 877 
> 
> Diff: https://reviewboard.asterisk.org/r/3554/diff/
> 
> 
> Testing
> -------
> 
> asterisk-11.x-WRITE$ svn log -r 414214 ../asterisk-1.8.x-WRITE/
> ------------------------------------------------------------------------
> r414214 | sgriepentrog | 2014-05-21 20:58:47 +0200 (wo, 21 mei 2014) | 13 lines
> 
> pbx.c: prevent potential crash from recursive replace()
> 
> Recurisve [sic] usage of replace() resulted in corruption of the
> temporary string storage and potential crash.  By changing
> the string to be allocated separtely per instance, this is
> eliminated.
> 
> ASTERISK-23650 #comment Reported by: Roel van Meer
> ASTERISK-23650 #close
> 
> Review: https://reviewboard.asterisk.org/r/3539/
> 
> 
> ------------------------------------------------------------------------
> 
> asterisk-11.x-WRITE$ svn up -r 414214
> ...
> 
> asterisk-11.x-WRITE$ merge811 414214
> ...
> 
> asterisk-11.x-WRITE$ cat ../merge.msg 
> pbx.c: prevent potential crash from recursive replace()
> 
> Recurisve [sic] usage of replace() resulted in corruption of the
> temporary string storage and potential crash.  By changing
> the string to be allocated separtely per instance, this is
> eliminated.
> 
> #ASTERISK-23650 #comment Reported by: Roel van Meer
> #ASTERISK-23650 #close
> 
> Review: https://reviewboard.asterisk.org/r/3539/
> ........
> 
> Merged revisions 414214 from http://svn.asterisk.org/svn/asterisk/branches/1.8
> 
> 
> Thanks,
> 
> wdoekes
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140527/debdfb77/attachment.html>