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. Global variables in prepare-ChangeLog can be categorized as follows: [A: Constant variables] $changeLogTimeZone $SVN $GIT %supportedTestExtensions [B: Arguments] $bugDescription $bugNumber $name $emailAddress $mergeBase $gitCommit $gitIndex $gitReviewer $openChangeLogs $writeChangeLogs $showHelp $spewDiff $updateChangeLogs $parseOptionsResult [C: Global variables that are used many times here and there] $isSVN $isGit [D: Global variables that are used only a few times] %paths $changedFiles $conflictFiles $functionLists $addedRegressionTests $bugURL $filesInChangeLog $prefixes $changeLogs In this bug, we remove all global variables in [C]. More accurately, we make $isGit and $isSVN be used only through parameter passing.
Created attachment 119161 [details] Patch
Comment on attachment 119161 [details] Patch Clearing flags on attachment: 119161 Committed r102761: <http://trac.webkit.org/changeset/102761>
All reviewed patches have been landed. Closing bug.
(In reply to comment #1) > Created an attachment (id=119161) [details] > Patch Instead of plumbing $isGit and $isSVN through a bunch of methods, why not use the isGit() and isSVN() methods where they're needed from the VCSUtils module (which is already imported)?
(In reply to comment #4) > (In reply to comment #1) > > Created an attachment (id=119161) [details] [details] > > Patch > > Instead of plumbing $isGit and $isSVN through a bunch of methods, why not use the isGit() and isSVN() methods where they're needed from the VCSUtils module (which is already imported)? Do they cache the return value? If not, calling those two functions every time might be quite expensive.
(In reply to comment #5) > (In reply to comment #4) > > Instead of plumbing $isGit and $isSVN through a bunch of methods, why not use the isGit() and isSVN() methods where they're needed from the VCSUtils module (which is already imported)? > > Do they cache the return value? If not, calling those two functions every time might be quite expensive. Yes.
(In reply to comment #6) > (In reply to comment #5) > > Do they cache the return value? If not, calling those two functions every time might be quite expensive. > > Yes. I see. That sounds like a good change then.
Hara-san, can we revert this change and do what David suggested? (i.e. just call isGit / isSVN).
(In reply to comment #8) > Hara-san, can we revert this change and do what David suggested? (i.e. just call isGit / isSVN). Of course!
Reverted r102761 for reason: we came up with a better fix than this (see comments in bug 74485) Committed r102841: <http://trac.webkit.org/changeset/102841>
Created attachment 119334 [details] Patch
Comment on attachment 119334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119334&action=review > Tools/ChangeLog:16 > + (isGit): Judges whether the first argument is a Git-managed directory, if the first argument is defined. Otherwise, judges whether the current working directory is a Git-managed directory. > + (isSVN): Judges whether the first argument is a SVN-managed directory, if the first argument is defined. Otherwise, judges whether the current working directory is a SVN-managed directory. Please line-break somewhere. > Tools/Scripts/prepare-ChangeLog:173 > +# First, cache isGit() and isSVN() results for future use. > my %paths = processPaths(@ARGV); > +isGit(firstDirectoryOrCwd(%paths)); > +isSVN(firstDirectoryOrCwd(%paths)); Why do we need to do that? Are there cases where just calling isGit/isSVN isn't sufficient? I mean people don't mix git/svn checkouts, right?
Created attachment 119348 [details] Patch
(In reply to comment #12) > (From update of attachment 119334 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119334&action=review > > Tools/Scripts/prepare-ChangeLog:173 > > +# First, cache isGit() and isSVN() results for future use. > > my %paths = processPaths(@ARGV); > > +isGit(firstDirectoryOrCwd(%paths)); > > +isSVN(firstDirectoryOrCwd(%paths)); > > Why do we need to do that? Are there cases where just calling isGit/isSVN isn't sufficient? I mean people don't mix git/svn checkouts, right? I agree. I removed firstDirectoryOrCwd() and just used isGit()/isSVN().
Comment on attachment 119348 [details] Patch r=me
Comment on attachment 119348 [details] Patch Clearing flags on attachment: 119348 Committed r102890: <http://trac.webkit.org/changeset/102890>