Bug 74485 - [Refactoring] In prepare-ChangeLog, make $isGit and $isSVN be used only through parameter passing
Summary: [Refactoring] In prepare-ChangeLog, make $isGit and $isSVN be used only throu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 73531
  Show dependency treegraph
 
Reported: 2011-12-13 23:08 PST by Kentaro Hara
Modified: 2011-12-14 22:32 PST (History)
4 users (show)

See Also:


Attachments
Patch (12.02 KB, patch)
2011-12-13 23:13 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (8.00 KB, patch)
2011-12-14 16:57 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (7.34 KB, patch)
2011-12-14 17:29 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2011-12-13 23:08:24 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.

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.
Comment 1 Kentaro Hara 2011-12-13 23:13:30 PST
Created attachment 119161 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-14 02:31:49 PST
Comment on attachment 119161 [details]
Patch

Clearing flags on attachment: 119161

Committed r102761: <http://trac.webkit.org/changeset/102761>
Comment 3 WebKit Review Bot 2011-12-14 02:31:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 David Kilzer (:ddkilzer) 2011-12-14 10:53:24 PST
(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)?
Comment 5 Ryosuke Niwa 2011-12-14 11:21:40 PST
(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.
Comment 6 David Kilzer (:ddkilzer) 2011-12-14 11:33:50 PST
(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.
Comment 7 Ryosuke Niwa 2011-12-14 15:52:43 PST
(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.
Comment 8 Ryosuke Niwa 2011-12-14 15:53:19 PST
Hara-san, can we revert this change and do what David suggested? (i.e. just call isGit / isSVN).
Comment 9 Kentaro Hara 2011-12-14 15:54:25 PST
(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!
Comment 10 Kentaro Hara 2011-12-14 16:12:41 PST
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>
Comment 11 Kentaro Hara 2011-12-14 16:57:25 PST
Created attachment 119334 [details]
Patch
Comment 12 Ryosuke Niwa 2011-12-14 17:05:10 PST
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?
Comment 13 Kentaro Hara 2011-12-14 17:29:06 PST
Created attachment 119348 [details]
Patch
Comment 14 Kentaro Hara 2011-12-14 17:29:34 PST
(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 15 David Kilzer (:ddkilzer) 2011-12-14 21:15:38 PST
Comment on attachment 119348 [details]
Patch

r=me
Comment 16 WebKit Review Bot 2011-12-14 22:32:44 PST
Comment on attachment 119348 [details]
Patch

Clearing flags on attachment: 119348

Committed r102890: <http://trac.webkit.org/changeset/102890>
Comment 17 WebKit Review Bot 2011-12-14 22:32:49 PST
All reviewed patches have been landed.  Closing bug.