Bug 74681

Summary: [Refactoring] Remove all global variables from prepare-ChangeLog
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: Tools / TestsAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, ddkilzer, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73531    
Attachments:
Description Flags
Patch
none
rebased patch
rniwa: review+
patch for commit none

Kentaro Hara
Reported 2011-12-15 20:53:48 PST
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.
Attachments
Patch (11.36 KB, patch)
2011-12-15 21:01 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
rebased patch (11.84 KB, patch)
2011-12-15 23:15 PST, Kentaro Hara 		rniwa: review+
DetailsFormatted DiffDiff
patch for commit (11.83 KB, patch)
2011-12-16 00:07 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kentaro Hara
Comment 1 2011-12-15 21:01:24 PST
Created attachment 119553 [details] Patch
Kentaro Hara
Comment 2 2011-12-15 23:15:37 PST
Created attachment 119573 [details] rebased patch
Ryosuke Niwa
Comment 3 2011-12-15 23:47:11 PST
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
Comment 4 2011-12-16 00:07:27 PST
Created attachment 119576 [details] patch for commit
Kentaro Hara
Comment 5 2011-12-16 00:07:38 PST
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
Comment 6 2011-12-16 00:24:31 PST
(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 7 2011-12-16 01:21:19 PST
Comment on attachment 119576 [details] patch for commit Clearing flags on attachment: 119576 Committed r103046: <http://trac.webkit.org/changeset/103046>
Note You need to log in before you can comment on or make changes to this bug.