Bug 31500 – webkit-patch land hangs if svn prompts for credentials

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

--non-interactive
Would be one way to solve this.

[Eric Seidel (no email)]

Ojan hit this this evening with a git checkout.  land-diff hung with git svn dcommit.

[Ojan Vafai]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Daniel Bates]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

I've started work on this.  Ran in to bug 45862.

[Csaba Osztrogonác]

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.

[Adam Barth]

Roland just ran into this bug too.

[Eric Seidel (no email)]

Everyone runs into this bug the first time they try and use webkit-patch. :)

[Peter Gal]

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

[Peter Gal]

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.

[Adam Barth]

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

[Peter Gal]

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

[Peter Gal]

Created attachment 88610 [details]
v2

updated patch.

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 89956 [details]
[Work-in-progress] Detect SVN credentials when using git-svn

Updated work-in-progress patch.

[Daniel Bates]

Created attachment 89963 [details]
Patch and updated unit tests

[Adam Barth]

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.

[Eric Seidel (no email)]

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?

[Daniel Bates]

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

[Daniel Bates]

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

[Daniel Bates]

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

[Daniel Bates]

Created attachment 89966 [details]
Patch and updated unit tests

Updated patch based on Eric Seidel's comments.

[WebKit Review Bot]

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.

[Daniel Bates]

Created attachment 89967 [details]
Patch and updated unit tests

Removed semicolon to fix style issue found by style bot.

[Eric Seidel (no email)]

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.

[Daniel Bates]

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

[Daniel Bates]

Committed r84114: <http://trac.webkit.org/changeset/84114>

[Daniel Bates]

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.

[Eric Seidel (no email)]

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

[Csaba Osztrogonác]

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.

[Daniel Bates]

Created attachment 90579 [details]
Patch and unit tests

[Daniel Bates]

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.

[Daniel Bates]

Committed r84697: <http://trac.webkit.org/changeset/84697>

[WebKit Review Bot]

http://trac.webkit.org/changeset/84697 might have broken SnowLeopard Intel Release (Build)

[Dominic Cooney]

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.

[Darin Adler]

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.