Bug 74681 – [Refactoring] Remove all global variables from prepare-ChangeLog

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kentaro Hara]

We are planning to write unit-tests for prepare-ChangeLog in a run-leaks_unittest manner. This bug is one of the incremental refactorings to remove all top-level code and global variables from prepare-ChangeLog.

In this bug, we remove all the remaining global variables. Accurately, we make $mergeBase, $gitCommit and $gitIndex be used only through parameter passing.

[Kentaro Hara]

Created attachment 119553 [details]
Patch

[Kentaro Hara]

Created attachment 119573 [details]
rebased patch

[Ryosuke Niwa]

Comment on attachment 119573 [details]
rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119573&action=review

> Tools/Scripts/prepare-ChangeLog:527
> +    my $changedFilesString = "'" . join ("' '", @$changedFiles) . "'";

Nit: There is a space between join and (.

> Tools/Scripts/prepare-ChangeLog:1468
> -    my @files = @_;
> +    my ($paths, $gitCommit, $gitIndex, $mergeBase) = @_;

Why are we taking $paths instead of @files here?

> Tools/Scripts/prepare-ChangeLog:1476
> +        my $filesString = "\"" . join ("\" \"", keys %$paths) . "\"";

Ditto about the space between join and (. By the way, can you replace all these "\" \"" by '" "' to improve the readability?

[Kentaro Hara]

Created attachment 119576 [details]
patch for commit

[Kentaro Hara]

Comment on attachment 119573 [details]
rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119573&action=review

>> Tools/Scripts/prepare-ChangeLog:527
>> +    my $changedFilesString = "'" . join ("' '", @$changedFiles) . "'";
> 
> Nit: There is a space between join and (.

Fixed it.

>> Tools/Scripts/prepare-ChangeLog:1468
>> +    my ($paths, $gitCommit, $gitIndex, $mergeBase) = @_;
> 
> Why are we taking $paths instead of @files here?

If we write

    my (@files, $gitCommit, $gitIndex, $mergeBase) = @_;

then all @_ are assigned to @files, unfortunately. So possible options are 

    statusCommand(keys %$paths, ...)
    sub statusCommand(\@$$$$) {
        my ($files, ...) = @_;
        ... = @$files;
    }

or

    statusCommand($paths, ...)    
    sub statusCommand($$$$$) {
        my ($paths, $gitCommit, $gitIndex, $mergeBase) = @_;
        ... = keys %$paths;
    }

I think both are fine (and chose the latter without a strong reason).

>> Tools/Scripts/prepare-ChangeLog:1476
>> +        my $filesString = "\"" . join ("\" \"", keys %$paths) . "\"";
> 
> Ditto about the space between join and (. By the way, can you replace all these "\" \"" by '" "' to improve the readability?

Fixed it.

[Ryosuke Niwa]

(In reply to comment #5)
> I think both are fine (and chose the latter without a strong reason).

Yeah, either solution is fine. I just wanted to avoid us duplicating "keys %$paths".

[WebKit Review Bot]

Comment on attachment 119576 [details]
patch for commit

Clearing flags on attachment: 119576

Committed r103046: <http://trac.webkit.org/changeset/103046>