Bug 74485 – [Refactoring] In prepare-ChangeLog, make $isGit and $isSVN be used only through parameter passing

• 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.

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.

[Kentaro Hara]

Created attachment 119161 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119161 [details]
Patch

Clearing flags on attachment: 119161

Committed r102761: <http://trac.webkit.org/changeset/102761>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(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)?

[Ryosuke Niwa]

(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.

[David Kilzer (:ddkilzer)]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Hara-san, can we revert this change and do what David suggested? (i.e. just call isGit / isSVN).

[Kentaro Hara]

(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!

[Kentaro Hara]

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>

[Kentaro Hara]

Created attachment 119334 [details]
Patch

[Ryosuke Niwa]

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?

[Kentaro Hara]

Created attachment 119348 [details]
Patch

[Kentaro Hara]

(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().

[David Kilzer (:ddkilzer)]

Comment on attachment 119348 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 119348 [details]
Patch

Clearing flags on attachment: 119348

Committed r102890: <http://trac.webkit.org/changeset/102890>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.