<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c8">Comment # 8</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:jmarcell@apple.com" title="Jason Marcell <jmarcell@apple.com>"> <span class="fn">Jason Marcell</span></a>
</span></b>
<pre>Comment on <span class="bz_obsolete"><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>
<span class="quote">>> Tools/ChangeLog:4
>> + that are marked as requiring corresponding tests.
>
> The second line seems like duplicate text that can be removed.</span >
I addressed this.
<span class="quote">>> Tools/ChangeLog:11
>> + simulate Git's attribute behevaior.
>
> 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 >
I ran the tests before and after my patch and diffed the results. I didn't see any change.
<span class="quote">>> Tools/Scripts/prepare-ChangeLog:1815
>> + my (@dirparts) = File::Spec->splitdir($file);
>
> @dirparts should be named @dirParts or @directoryParts.</span >
I addressed this.
<span class="quote">>> Tools/Scripts/prepare-ChangeLog:1827
>> + my $command = SVN . " propget $attr $subPath | tr -d '\\n'";
>
> Any reason you use 'tr' here instead of chomp()? (Not necessary to fix to land the patch.)</span >
When I was developing on the command line I was using standard Unix tools. I meant to convert to native PERL upon incorporating this code into the code base. As such I've changed this to use chomp in a later patch.
<span class="quote">>> Tools/Scripts/prepare-ChangeLog:2032
>> + push @requiresTests, $file
>
> 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.)</span >
I decided to work with strings internally but have attributeCommand return an integer (1 for true, 0 for false). Users should store a "1" for a given SVN property or Git attribute to indicate true.</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>