| Summary: | svn-unapply and svn-apply don't work when used outside multiple svn working directories | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
| Component: | Tools / Tests | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | eric, mitz | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Other | ||||||
| OS: | OS X 10.5 | ||||||
| Attachments: |
|
||||||
Comment on attachment 40130 [details]
Patch v1
Why would we want to change the behavior of all scripts using vcsutils? I would think we would just want to fix these too. Or maybe we should pass an extra bool to determineVCSRoot if we're to assume svn?
(In reply to comment #1) > (From update of attachment 40130 [details]) > Why would we want to change the behavior of all scripts using vcsutils? I > would think we would just want to fix these too. Or maybe we should pass an > extra bool to determineVCSRoot if we're to assume svn? I thought about passing a boolean value to VCSUtils::determineVCSRoot() to change the behavior as well, but reading the code didn't make it obvious what the boolean did. I am operating under the assumption that users know what they're doing when they run each command, and will see the warning if the tool doesn't do what they want it to do. If you think the boolean should be added when calling determineVCSRoot() from svn-appply and svn-unapply, I'll go ahead and do that. It's only used in these places: Scripts/commit-log-editor:my $topLevel = determineVCSRoot(); Scripts/resolve-ChangeLogs:my $relativePath = isInGitFilterBranch() ? '.' : chdirReturningRelativePath(determineVCSRoot()); Scripts/svn-apply:my $pathForRepositoryRoot = determineVCSRoot(); Scripts/svn-unapply:my $pathForRepositoryRoot = determineVCSRoot(); So I think the behavior change is not going to cause trouble. It still feels strange for the common library function to behave this way. (Then again, it's stranger still to use svn-apply outside of an svn directory... It would be nice if you could explain that use case in the ChangeLog). Comment on attachment 40130 [details]
Patch v1
I'm OK with this. Another way to solve this (since perl doesn't support named args) is to have a separate function "determineVCSRootOrWarn()", but I'm not sure that really helps us any. This is OK as is. Would be nice to explain the use case better in the ChangeLog. I assume folks make patches rotted across both OpenSource and Internal directories and roll them in/out of their trees.
(In reply to comment #4) > (From update of attachment 40130 [details]) > I'm OK with this. Another way to solve this (since perl doesn't support named > args) is to have a separate function "determineVCSRootOrWarn()", but I'm not > sure that really helps us any. This is OK as is. Would be nice to explain the > use case better in the ChangeLog. I assume folks make patches rotted across > both OpenSource and Internal directories and roll them in/out of their trees. Yes, exactly. Doesn't the ChangeLog entry capture that? + Some users have a workflow where svn-create-patch, svn-apply and + svn-unapply are used outside of multiple svn working + directories. Instead of aborting the scripts in these cases, + print a warning and assume that Subversion is being used. (In reply to comment #3) > So I think the behavior change is not going to cause trouble. It still feels > strange for the common library function to behave this way. (Then again, it's > stranger still to use svn-apply outside of an svn directory... It would be > nice if you could explain that use case in the ChangeLog). It's no less strange than making svn-apply work on git repositories. :) Committed r48793: <http://trac.webkit.org/changeset/48793> |
Created attachment 40130 [details] Patch v1 Reviewed by NOBODY (OOPS!). Some users have a workflow where svn-create-patch, svn-apply and svn-unapply are used outside of multiple svn working directories. Instead of aborting the scripts in these cases, print a warning and assume that Subversion is being used. * Scripts/VCSUtils.pm: (determineVCSRoot): Call warn() instead of die() if both isGit() and isSVN() initially return false. Set $VCSUtils::isSVN to 1 to enforce the assumption about Subversion, then return determineSVNRoot(). * Scripts/svn-apply: Switch to using isGit() and isSVN() from VCSUtils.pm. They both already cache their values and checking here is redundant since determineVCSRoot() is called later. --- 3 files changed, 41 insertions(+), 19 deletions(-)