[webkit-reviews] review requested: [Bug 48275] Teach webkit-patch how to read credentials from the environment : [Attachment 71813] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has asked  for review:
Bug 48275: Teach webkit-patch how to read credentials from the environment
https://bugs.webkit.org/show_bug.cgi?id=48275

Attachment 71813: Patch
https://bugs.webkit.org/attachment.cgi?id=71813&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71813&action=review

I'm a bit scared of people putting this in an environment variable.  I think
it's more common for environment variables to be leaked accidentally (e.g.,
build-webkit shows your env or crash dumps include your env).

It might be better to read a named file on disk or try to read the config file
directly from ~/.subversion/auth.

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:128
> +    def _credentials_from_environment(self):
> +	   return (self._read_environ("username"),
self._read_environ("password"))

I'm not convinced _read_environ and _environ_prefix buys you much over
WEBKIT_BUGZILLA_USERNAME and WEBKIT_BUGZILLA_PASSWORD.	It does make grepping
harder.

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:144
> +	       (username, password) = self._credentials_from_git()
>	   if not username or not password:
>	       (username, password) = self._credentials_from_keychain(username)


Nit: don't need () on the left side of =

> WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py:132
>	   os.rmdir(temp_dir_path)

Do we leak this dir if the test fails?

> WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py:153
> +	   # FIXME: Seems 'with' would work better than a try/finally here.

How would 'with' ensure that os.rmdir is called?