<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3554/">https://reviewboard.asterisk.org/r/3554/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 22nd, 2014, 11:41 a.m. CDT, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On May 22nd, 2014, 3:47 p.m. CDT, <b>wdoekes</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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? :)
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Matt</p>
<br />
<p>On May 23rd, 2014, 5:23 a.m. CDT, wdoekes wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and Matt Jordan.</div>
<div>By wdoekes.</div>
<p style="color: grey;"><i>Updated May 23, 2014, 5:23 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Repotools
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/svnmerge <span style="color: grey">(877)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3554/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>