<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:ddkilzer@webkit.org" title="David Kilzer (:ddkilzer) <ddkilzer@webkit.org>"> <span class="fn">David Kilzer (:ddkilzer)</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Modify prepare-Changelog to be aware of files that are marked as tests as well as files that are marked as requiring corresponding tests."
href="https://bugs.webkit.org/show_bug.cgi?id=148498">bug 148498</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #259995 Flags</td>
<td>review?, commit-queue?
</td>
<td>review+, commit-queue-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Modify prepare-Changelog to be aware of files that are marked as tests as well as files that are marked as requiring corresponding tests."
href="https://bugs.webkit.org/show_bug.cgi?id=148498#c2">Comment # 2</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Modify prepare-Changelog to be aware of files that are marked as tests as well as files that are marked as requiring corresponding tests."
href="https://bugs.webkit.org/show_bug.cgi?id=148498">bug 148498</a>
from <span class="vcard"><a class="email" href="mailto:ddkilzer@webkit.org" title="David Kilzer (:ddkilzer) <ddkilzer@webkit.org>"> <span class="fn">David Kilzer (:ddkilzer)</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=259995&action=diff" name="attach_259995" title="Patch">attachment 259995</a> <a href="attachment.cgi?id=259995&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=259995&action=review">https://bugs.webkit.org/attachment.cgi?id=259995&action=review</a>
r=me if you fix the duplicate text in ChangeLog, run test-webkitperl (and the prepare-ChangeLog tests still pass), and rename @dirparts.
<span class="quote">> Tools/ChangeLog:4
> + Modify prepare-Changelog to be aware of files that are marked as tests as well as files that are marked as requiring corresponding tests.
> + that are marked as requiring corresponding tests.</span >
The second line seems like duplicate text that can be removed.
<span class="quote">> Tools/ChangeLog:11
> + * Scripts/prepare-ChangeLog: Added "attributeCache" to cache Subversion properties in order to
> + simulate Git's attribute behevaior.</span >
Note that there are unit tests for prepare-ChangeLog in Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/.
This patch didn't modify any methods that are already tested, but we want to make sure the tests still run with this change (because each unit test imports prepare-ChangeLog as a custom Perl module, so things like global variables can break that technique).
The tests are run thusly: ./Tools/Scripts/test-webkitperl
Looks like there are a couple of tests that are broken now, but the prepare-ChangeLog tests look fine.
<span class="quote">> Tools/Scripts/prepare-ChangeLog:85
> -sub generateNewChangeLogs($$$$$$$$$$$$);
> +sub generateNewChangeLogs($$$$$$$$$$$$$);</span >
Some day, maybe we should change generateNewChangeLogs() to take a single argument that's a hash so we can get pseudo-named arguments in Perl.
You do not have to fix this to land the patch.
<span class="quote">> Tools/Scripts/prepare-ChangeLog:1815
> + my (@dirparts) = File::Spec->splitdir($file);</span >
@dirparts should be named @dirParts or @directoryParts.
<span class="quote">> Tools/Scripts/prepare-ChangeLog:1827
> + my $command = SVN . " propget $attr $subPath | tr -d '\\n'";</span >
Any reason you use 'tr' here instead of chomp()? (Not necessary to fix to land the patch.)
<span class="quote">> Tools/Scripts/prepare-ChangeLog:2032
> + } elsif (attributeCommand($file, "test") eq "true") {
> + push @addedRegressionTests, $file;
> + } elsif (attributeCommand($file, "requiresTests") eq "true") {
> + push @requiresTests, $file</span >
I find returning string results of 'true' or 'unspecified' kind of unusual. Is there a reason you couldn't use 1 for 'true' and '0' for unspecified?
Oh, is this to make the svn and git attributes more readable in that you're literally storing 'true' instead of just '1'?
(This doesn't need to be addressed before landing the change, but I'd like to understand the design decision here.)</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>