Bug 38870 – svn.webkit.org needs a post-commit hook to fix svn:author from commit-queue patches

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

svn.webkit.org needs a post-commit hook to fix svn:author from commit-queue patches

Proposal is detailed here:
https://lists.webkit.org/pipermail/webkit-dev/2010-April/012350.html

Should be simple to write.  It needs to be tested locally (to make sure it can't cause the commit to fail in any way, even if it fails), and then handed off to Bill Siegrist for integration with svn.webkit.org

[Eric Seidel (no email)]

Bug 29274 should also be fixed before this is turned on to make it possible to distinguish patches which were landed via the commit-queue from those which weren't.

[Eric Seidel (no email)]

Tests that this post-commit hook should pass:

- correctly do nothing patches where the author is not a committer (committers.py or the svn auth file)
- correctly not block the commit in case of some sort of lookup error (the fix-the-author script should be separate)

[William Siegrist]

This part of the hook will have to be run synchronously and finish before anything else in the hook runs, so that everything else will see the correct author (emails to -changes, buildbot, etc).  So we'll need a way of making this timeout if it hangs for any reason.

[Adam Barth]

Are our existing commit hooks in SVN somewhere?  If you show me an example, I might be able to do this.

[William Siegrist]

The hook script is not in svn. The hook script is just a shell script, with two variables already defined for you (path to the repo and the revision number).  I can make webkitpy available on that server as well if that helps.

[Adam Barth]

Created attachment 55619 [details]
Patch

[Adam Barth]

This is more of a first draft for discussion.  Open questions:

1) Should we only change the svn:author for the commit-queue?
2) How does this code handle the case where we have a users bugzilla email in committers.py but not their svn email?
3) Can post-commit call this script directly or do we need a shell script in between?
4) What's our testing plan here?

[Adam Barth]

Created attachment 55621 [details]
Patch

[Adam Barth]

I should also say that I don't have any clue if this script works because I don't know how to test it.

[Eric Seidel (no email)]

I think in the end we're going to want to use the svn auth file (whatever that looks like).

We should be able to test this using various ChangeLog entries as test cases.

[Eric Seidel (no email)]

(In reply to comment #7)
> This is more of a first draft for discussion.  Open questions:

Thank you!

> 1) Should we only change the svn:author for the commit-queue?

Yes.  Otherwise you could "commit as" darin just by putting his name in the ChangeLog.  Eeek.
Ideally we should create a separate commit account for the commit-queue. :)

> 2) How does this code handle the case where we have a users bugzilla email in committers.py but not their svn email?

I don't think we should use the committers.py list.  It's nice, but it's not as accurate as the theoretical svn auth file on the server (which I know nothing about).

> 3) Can post-commit call this script directly or do we need a shell script in between?

I don't see why not.

> 4) What's our testing plan here?

If we're using webkitpy, we should be able to write python unit tests for testing what happens with various ChangeLog messages.  scm_unittests or checkout_unittests would be appropriate places.

[Eric Seidel (no email)]

(In reply to comment #9)
> I should also say that I don't have any clue if this script works because I don't know how to test it.

We would have to set up a post-commit hook on a local test repository.  possibly as part of scm_unittest, or more likely as part of some new module which was this command.

[William Siegrist]

This seems like a lot of abstraction for an svn hook. And we don't have a working copy on the server, so this should end up using svnlook instead. I'll work on an alternate approach.

[Eric Seidel (no email)]

Really all one needs is:

1.  The location and format of the svn auth file (probably grep is all one really needs)
2.  A regexp with which to parse date lines.

The trouble is, if the ChangeLog ever doens't start at the top of the file, then you coudl pull the wrong author for the commit!

We may need a pre-commit hook for SVN to prevent such bad changelog edits before we could really use something like this dependably.   See bug 28291.

[Eric Seidel (no email)]

Comment on attachment 55621 [details]
Patch

wms is going to work up an alternate approach he says.

[Eric Seidel (no email)]

wms is currently testing a solution.

[William Siegrist]

I'm pretty sure this has been fixed for a while.

Eric, was there any other changes you wanted under this bug?

[Eric Seidel (no email)]

Nope.  Thanks.