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.
--non-interactive Would be one way to solve this.
Ojan hit this this evening with a git checkout. land-diff hung with git svn dcommit.
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.
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. :)
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.
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.
(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.
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. :)
I've started work on this. Ran in to bug 45862.
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.
Roland just ran into this bug too.
Everyone runs into this bug the first time they try and use webkit-patch. :)
(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.
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 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".
(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.
Created attachment 88610 [details] v2 updated patch.
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.
Created attachment 89956 [details] [Work-in-progress] Detect SVN credentials when using git-svn Updated work-in-progress patch.
Created attachment 89963 [details] Patch and updated unit tests
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 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?
(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.
(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.
(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()."
Created attachment 89966 [details] Patch and updated unit tests Updated patch based on Eric Seidel's comments.
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.
Created attachment 89967 [details] Patch and updated unit tests Removed semicolon to fix style issue found by style bot.
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.
(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.
Committed r84114: <http://trac.webkit.org/changeset/84114>
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.
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. :)
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.
Created attachment 90579 [details] Patch and unit tests
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.
Committed r84697: <http://trac.webkit.org/changeset/84697>
http://trac.webkit.org/changeset/84697 might have broken SnowLeopard Intel Release (Build)
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.
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.