Bug 32896 - [bzt] Bugzilla-tool should not try to get the username and password from Git when we're using SVN
Summary: [bzt] Bugzilla-tool should not try to get the username and password from Git ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-23 01:36 PST by Daniel Bates
Modified: 2010-01-19 16:59 PST (History)
3 users (show)

See Also:


Attachments
Trace (3.08 KB, text/plain)
2009-12-23 01:37 PST, Daniel Bates
no flags Details
Patch (Work in progress) (1.75 KB, patch)
2009-12-23 01:39 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (Work in progress) (2.68 KB, patch)
2009-12-24 22:37 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (Work in progress) (2.69 KB, patch)
2009-12-24 22:39 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with unit tests (8.77 KB, patch)
2009-12-27 17:32 PST, Daniel Bates
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2009-12-23 01:36:03 PST
Bugzilla-tool tries to read the username and password from Git to use on bugs.webkit.org. However, if neither Git is installed nor we are using Git as our SCM for WebKit then bugzilla-tool will die (with a trace). For instance, we may be using SVN.

We should make bugzilla-tool handle the case when the external command (i.e. git) cannot be found.
Comment 1 Daniel Bates 2009-12-23 01:37:36 PST
Created attachment 45425 [details]
Trace

Python trace that occurred when I tried to land-patches 32799.
Comment 2 Daniel Bates 2009-12-23 01:39:43 PST
Created attachment 45426 [details]
Patch (Work in progress)

Work in progress.
Comment 3 Adam Barth 2009-12-23 11:56:15 PST
A couple comments:

1) We already have code that detects whether we're in a Git working copy: Git.in_working_directory in SCM.py.

2) We'll need a unittest.  You can look in credentials_unittest to see some examples.
Comment 4 Daniel Bates 2009-12-24 22:37:10 PST
Created attachment 45483 [details]
Patch (Work in progress)

Revised patch to use Git.in_working_directory.

Added argument cwd (current working directory) to Credentials constructor. Added unit test that uses SVNTestRepository to create a temporary SVN repo. before calling credentials.read_credentials.
Comment 5 Daniel Bates 2009-12-24 22:39:55 PST
Created attachment 45484 [details]
Patch (Work in progress)
Comment 6 Daniel Bates 2009-12-27 17:32:09 PST
Created attachment 45538 [details]
Patch with unit tests

Notice, calling Git.in_working_directory can throw an OSError errrno 2 exception if the git command is not found because we call Executive.run_command which does not catch the exception. Hence, this patch modifies Executive.run_command to catch these exceptions and passes them to the specified error handler/returns the error code of the exception/returns the error message of the exception.

For the credentials unit test, I assume SVN is installed as per <http://webkit.org/building/tools.html> at the time of this writing, since SVNTestRepository (imported from modules.scm_unittest) assumes it.

Also, I added a new unit test file, executive_unittest.py, to test the change made to Executive.run_command.
Comment 7 WebKit Review Bot 2009-12-27 17:37:31 PST
style-queue ran check-webkit-style on attachment 45538 [details] without any errors.
Comment 8 Adam Barth 2009-12-27 17:46:35 PST
Comment on attachment 45538 [details]
Patch with unit tests

This looks good.  Thanks Dan!
Comment 9 Daniel Bates 2009-12-27 18:02:55 PST
Committed in <http://trac.webkit.org/changeset/52587>.
Comment 10 Eric Seidel (no email) 2010-01-19 16:18:33 PST
This broke a couple scm.py unit tests:
ERROR: test_error_handlers (webkitpy.scm_unittest.SCMClassTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/scm_unittest.py", line 150, in test_error_handlers
    self.assertRaises(OSError, run_command, command_does_not_exist)
  File "/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/unittest.py", line 320, in failUnlessRaises
    callableObj(*args, **kwargs)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/executive.py", line 70, in run_command
    return Executive().run_command(*args, **kwargs)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/executive.py", line 142, in run_command
    (error_handler or self.default_error_handler)(script_error)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/executive.py", line 113, in default_error_handler
    raise error
ScriptError: Failed to run "['does_not_exist', 'invalid_option']" exit_code: 2

I'm not sure catching OSError here is a good idea.  It turns missing scripts into "exit code 2" which happens to be the same as QueueEngine.handled_error_code = 2

We obviously could change handled_error_code....
Comment 11 Daniel Bates 2010-01-19 16:59:43 PST
Will look into this issue.

(In reply to comment #10)
> This broke a couple scm.py unit tests:
> ERROR: test_error_handlers (webkitpy.scm_unittest.SCMClassTests)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File
> "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/scm_unittest.py",
> line 150, in test_error_handlers
>     self.assertRaises(OSError, run_command, command_does_not_exist)
>   File
> [...]
> I'm not sure catching OSError here is a good idea.  It turns missing scripts
> into "exit code 2" which happens to be the same as
> QueueEngine.handled_error_code = 2
> 
> We obviously could change handled_error_code....