Bug 29744 – svn-unapply and svn-apply don't work when used outside multiple svn working directories

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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

[Eric Seidel (no email)]

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?

[David Kilzer (:ddkilzer)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Committed r48793: <http://trac.webkit.org/changeset/48793>