Bug 31500

Summary: webkit-patch land hangs if svn prompts for credentials
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aa, abarth, abecsi, arv, cjerdonek, darin, dbates, ddkilzer, dominicc, eric, evan, galpeter, levin, ojan, ossy, rolandsteiner, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 45862    
Bug Blocks:    
Attachments:
Description Flags
proof of concept patch
none
v2
none
[Work-in-progress] Detect SVN credentials when using git-svn
none
[Work-in-progress] Detect SVN credentials when using git-svn
none
Patch and updated unit tests
none
Patch and updated unit tests
none
Patch and updated unit tests
none
Patch and unit tests
none
Patch and unit tests ossy: review+

Description Eric Seidel (no email) 2009-11-13 17:17:36 PST
bugzilla-tool land-diff hangs if svn prompts for credentials

We need to find a way to detect this and let the prompting go through to the user.

Both Aaron and I hit this today.  It only happens once, but it's a crappy first-run experience for new committers.
Comment 1 Eric Seidel (no email) 2009-11-17 16:24:27 PST
--non-interactive
Would be one way to solve this.
Comment 2 Eric Seidel (no email) 2010-01-04 21:31:25 PST
Ojan hit this this evening with a git checkout.  land-diff hung with git svn dcommit.
Comment 3 Ojan Vafai 2010-06-02 16:03:51 PDT
Until this is fixed properly, maybe we should just have a timeout for the actual commit step. If we hit the timeout, we error out and give a description of what the problem might be and how to fix it.
Comment 4 Eric Seidel (no email) 2010-06-02 16:04:50 PDT
The unfortunate thing about this bug is that it only hits new contributers, and thus gets ignored since all of us who actually contribute to webkit-patch have long-since stored our credentials on our machines. :)
Comment 5 Eric Seidel (no email) 2010-06-02 16:09:20 PDT
There should be some svn command which would check if it has credentials cached or not.  We could very easily run such before commit to confirm that they're cached if anyone knows what such a command would be.
Comment 6 Eric Seidel (no email) 2010-06-02 16:14:21 PDT
Actually, a solution (for svn) is to just pass --non-interactive to svn commit.  That will make it at least fail instead of hang.  I don't have any suggestions for git yet.
Comment 7 Daniel Bates 2010-06-02 16:17:01 PDT
(In reply to comment #5)
> There should be some svn command which would check if it has credentials cached or not.  We could very easily run such before commit to confirm that they're cached if anyone knows what such a command would be.

Unless I am misunderstanding the issue, support was added for the SVN case in <https://bugs.webkit.org/show_bug.cgi?id=34439>. In particular, SVN.has_authorization_for_realm() was added to determine if the SVN credentials are cached.

We may need something similar for Git.
Comment 8 Eric Seidel (no email) 2010-06-02 16:22:19 PDT
git svn dcommit uses svn (and thus the same cache) under the covers.  So it would be possible to fix this by checking SVN.has_authorization_for_realm() before calling commit and then passing --non-interactive to svn.  Git would still hang if the cached credentials are wrong, but that's much less common that first-run. :)
Comment 9 Eric Seidel (no email) 2010-09-15 18:53:26 PDT
I've started work on this.  Ran in to bug 45862.
Comment 10 Csaba Osztrogonác 2011-04-04 15:56:45 PDT
It is a so annoying bug. We can't use webkit-patch land, because 
we don't want to save our svn password as a plain text. :S

AFAIK Peter you have an experimental patch for it. We should 
check it tomorrow and fix this bug as soon as possible.
Comment 11 Adam Barth 2011-04-04 16:13:58 PDT
Roland just ran into this bug too.
Comment 12 Eric Seidel (no email) 2011-04-04 18:12:39 PDT
Everyone runs into this bug the first time they try and use webkit-patch. :)
Comment 13 Peter Gal 2011-04-05 01:10:33 PDT
(In reply to comment #10)
> AFAIK Peter you have an experimental patch for it. We should 
> check it tomorrow and fix this bug as soon as possible.

I'll check my magic box for the patch in a few hours, and upload it here. It's not a complete patch, just a proof of concept test for the problem.
Comment 14 Peter Gal 2011-04-07 02:36:53 PDT
Created attachment 88597 [details]
proof of concept patch

Took a little to long to find and update the patch. By default there is no change in the behavior, but if you add '-p' switch while executing the webkit-patch then it'll ask for a password. Currently I can't test it because I'm not a committer.
Comment 15 Adam Barth 2011-04-07 02:42:23 PDT
Comment on attachment 88597 [details]
proof of concept patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88597&action=review

> Tools/Scripts/webkitpy/tool/main.py:52
> +        make_option("-p", "--password", action="store_true", dest="prompt_password", default=False, help="prompt for password"),

Why should you have to pass an option in order to enter a password?  I would have expected webkit-patch to prompt you for a password if you needed it.

Ideally, if we could detect that a password was needed, we could handle this all at the scm.py layer.

> Tools/Scripts/webkitpy/tool/steps/commit.py:28
> +import getpass

This looks extra.

> Tools/Scripts/webkitpy/tool/steps/commit.py:70
> -                commit_text = scm.commit_with_message(self._commit_message, git_commit=self._options.git_commit, username=username, force_squash=force_squash, changed_files=self._changed_files(state))
> +                commit_text = scm.commit_with_message(self._commit_message, git_commit=self._options.git_commit,
> +                                                      username=username, force_squash=force_squash,
> +                                                      changed_files=self._changed_files(state), input=password)

instead of "input=password", it seems like we'd want to say "password=password".
Comment 16 Peter Gal 2011-04-07 05:26:51 PDT
(In reply to comment #15)
> (From update of attachment 88597 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88597&action=review
> 
> > Tools/Scripts/webkitpy/tool/main.py:52
> > +        make_option("-p", "--password", action="store_true", dest="prompt_password", default=False, help="prompt for password"),
> 
> Why should you have to pass an option in order to enter a password?  I would have expected webkit-patch to prompt you for a password if you needed it.
> 
> Ideally, if we could detect that a password was needed, we could handle this all at the scm.py layer.
> 

As I mentioned this is just a proof-of-concept fix for the problem. Password detection could be added later, if this fixes the initial problem. 

> > Tools/Scripts/webkitpy/tool/steps/commit.py:28
> > +import getpass
> 
> This looks extra.
> 
> > Tools/Scripts/webkitpy/tool/steps/commit.py:70
> > -                commit_text = scm.commit_with_message(self._commit_message, git_commit=self._options.git_commit, username=username, force_squash=force_squash, changed_files=self._changed_files(state))
> > +                commit_text = scm.commit_with_message(self._commit_message, git_commit=self._options.git_commit,
> > +                                                      username=username, force_squash=force_squash,
> > +                                                      changed_files=self._changed_files(state), input=password)
> 
> instead of "input=password", it seems like we'd want to say "password=password".
I overlooked these during the update. 

I'll update the patch with the password detection in a minute. Stay tuned.
Comment 17 Peter Gal 2011-04-07 05:34:12 PDT
Created attachment 88610 [details]
v2

updated patch.
Comment 18 Daniel Bates 2011-04-07 09:23:25 PDT
Created attachment 88650 [details]
[Work-in-progress] Detect SVN credentials when using git-svn

Work in progress patch based on Eric Seidel's remarks (in comment 8) that Git uses the same SVN cache as SVN; => we can reuse the functionality in SVN.has_authorization_for_realm().

I am posting this patch as a work-in-progress. I haven't tested it yet. It could be further refined, needs a unit test(s) and a proper change log. I also don't mean to interrupt the work by Peter Gal. Should we chose to land Peter's patch then I suggest that we follow up with a patch that reuses SVN.has_authorization_for_realm() or implement a mechanism that provides similar functionality for git-svn.
Comment 19 Daniel Bates 2011-04-17 12:44:54 PDT
Created attachment 89956 [details]
[Work-in-progress] Detect SVN credentials when using git-svn

Updated work-in-progress patch.
Comment 20 Daniel Bates 2011-04-17 14:30:59 PDT
Created attachment 89963 [details]
Patch and updated unit tests
Comment 21 Adam Barth 2011-04-17 14:36:39 PDT
Comment on attachment 89963 [details]
Patch and updated unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=89963&action=review

Look beautiful.

> Tools/Scripts/webkitpy/common/net/credentials.py:151
> -            password = getpass.getpass("%s password for %s: " % (self.host, username))
> +            password = User.prompt_password("%s password for %s: " % (self.host, username))

This should really get the user object off the tool so we can mock it out during testing.
Comment 22 Eric Seidel (no email) 2011-04-17 15:09:12 PDT
Comment on attachment 89963 [details]
Patch and updated unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=89963&action=review

> Tools/Scripts/webkitpy/common/checkout/scm.py:384
> +        find_output = run_command(find_args, cwd=home_directory).rstrip()

Sad.  Why not move this onto some helper object which both SVN and Git share (which could have an Executive).  Or maybe Git should have an SVN object pointing to its git-svn checkout?

> Tools/Scripts/webkitpy/common/checkout/scm.py:848
> +                return self._commit_on_branch(message, 'HEAD', username=username, password=password)

Do we really want to pass these around everywhere?  Does SVN?  Or does it store it on the SVN object?
Comment 23 Daniel Bates 2011-04-17 15:24:58 PDT
(In reply to comment #21)
> (From update of attachment 89963 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89963&action=review
> [...]
> > Tools/Scripts/webkitpy/common/net/credentials.py:151
> > -            password = getpass.getpass("%s password for %s: " % (self.host, username))
> > +            password = User.prompt_password("%s password for %s: " % (self.host, username))
> 
> This should really get the user object off the tool so we can mock it out during testing.

Currently, the tool isn't passed to the Credentials object.
Comment 24 Daniel Bates 2011-04-17 15:55:08 PDT
(In reply to comment #22)
> (From update of attachment 89963 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89963&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/scm.py:384
> > +        find_output = run_command(find_args, cwd=home_directory).rstrip()
> 
> Sad.  Why not move this onto some helper object which both SVN and Git share (which could have an Executive).  Or maybe Git should have an SVN object pointing to its git-svn checkout?

Will change to be a mixin class.

> 
> > Tools/Scripts/webkitpy/common/checkout/scm.py:848
> > +                return self._commit_on_branch(message, 'HEAD', username=username, password=password)
> 
> Do we really want to pass these around everywhere? 

Notice, _commit_on_branch() is specific to the Git code path. It is called from Git.commit_with_message() and both this method and Git._commit_on_branch() call Git.push_local_commits_to_server() which needs these credentials so as to pass to git svn dcommit.

We could look to hoist the call to Git.push_local_commits_to_server() from  Git._commit_on_branch() into Git.commit_with_message() so that we don't have to pass the username and password arguments to Git.commit_with_message(). At the moment I'm unsure how best to do this given the current structure of Git._commit_on_branch(). Further investigation is needed.

> Does SVN?  Or does it store it on the SVN object?

All of the logic for committing with SVN is contained in SVN.commit_with_message(). Hence, we don't see the same passing of these arguments to sub-functions as we do with the Git code path.
Comment 25 Daniel Bates 2011-04-17 15:57:36 PDT
(In reply to comment #24)
> > Do we really want to pass these around everywhere? 
> 
> [...]
> 
> We could look to hoist the call to Git.push_local_commits_to_server() from  Git._commit_on_branch() into Git.commit_with_message() so that we don't have to pass the username and password arguments to Git.commit_with_message(). 

I meant to write "...don't have to pass the username and password arguments to Git._commit_on_branch()."
Comment 26 Daniel Bates 2011-04-17 16:05:33 PDT
Created attachment 89966 [details]
Patch and updated unit tests

Updated patch based on Eric Seidel's comments.
Comment 27 WebKit Review Bot 2011-04-17 16:07:09 PDT
Attachment 89966 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/common/checkout/scm.py:329:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Daniel Bates 2011-04-17 16:09:10 PDT
Created attachment 89967 [details]
Patch and updated unit tests

Removed semicolon to fix style issue found by style bot.
Comment 29 Eric Seidel (no email) 2011-04-17 16:17:59 PDT
Comment on attachment 89967 [details]
Patch and updated unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=89967&action=review

Seems good.

> Tools/Scripts/webkitpy/common/checkout/scm.py:324
> +class SVNAndGitSVNHasAuthorizationForRealmMixin:

I would have done this as an SVNRepository class, but this is OK.
Comment 30 Daniel Bates 2011-04-17 16:23:01 PDT
(In reply to comment #29)
> > Tools/Scripts/webkitpy/common/checkout/scm.py:324
> > +class SVNAndGitSVNHasAuthorizationForRealmMixin:
> 
> I would have done this as an SVNRepository class, but this is OK.

Will rename before I landed.
Comment 31 Daniel Bates 2011-04-17 16:28:25 PDT
Committed r84114: <http://trac.webkit.org/changeset/84114>
Comment 32 Daniel Bates 2011-04-17 16:35:54 PDT
Fixed change log entry in changeset 84115 <http://trac.webkit.org/changeset/84115> to both acknowledge Adam Barth for reviewing the patch and to fix my email address.
Comment 33 Eric Seidel (no email) 2011-04-17 16:50:01 PDT
I meant that I would have use a has-a model instead of an is-a model.  A member class instead of a mixin.  BUt agian, this si fine. :)
Comment 34 Csaba Osztrogonác 2011-04-21 04:33:15 PDT
Reopen, because it still fails for me. :(

Unfortunately has_authorization_for_realm is incorrect, because it returns 
true when it finds "<http://svn.webkit.org:80> Mac OS Forge" in a file in
~/.subversion directory. But it doesn't mean that username and password 
are cached. We don't want to store our password as plain text, that's why
the file exists but there isn't any password in it.

Peter checks the password key in his patch (v2), it would be 
great to integrate this mechanism to has_authorization_for_realm.
Comment 35 Daniel Bates 2011-04-21 12:37:52 PDT
Created attachment 90579 [details]
Patch and unit tests
Comment 36 Daniel Bates 2011-04-21 12:41:13 PDT
Created attachment 90580 [details]
Patch and unit tests

Updated change log to reflect the rename of unit test test_not_have_authorization_for_realm() to test_not_have_authorization_for_realm_when_missing_credentials_file() so as to better describe what it's testing.
Comment 37 Daniel Bates 2011-04-22 15:48:18 PDT
Committed r84697: <http://trac.webkit.org/changeset/84697>
Comment 38 WebKit Review Bot 2011-04-22 16:04:42 PDT
http://trac.webkit.org/changeset/84697 might have broken SnowLeopard Intel Release (Build)
Comment 39 Dominic Cooney 2011-06-09 07:04:48 PDT
I think that this is broken for git users. If svn credentials aren't cached:

Commit.run calls scm.commit_with_message, passing None for user/pass.
...
Git.push_local_commits_to_server asks if has_authorization_for_realm ... and finds that it doesn't; raises an AuthenticationError.
Commit.run catches that, prompts for user/pass and retries.
...
Back in Git.push_local_commits_to_server, it again asks if has_authorization_for_realm ... which it still doesn't. Git doesn't do anything with the provided user/pass; so raise AuthenticationError and around we go.
Comment 40 Darin Adler 2011-06-18 11:45:12 PDT
If this change caused a bug, please file a bug report about it. Don't reopen a bug to report that the fix caused a problem. We should reopen a bug if:

    1) The fix didn’t work, or
    2) The fix was rolled out.