WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
34439
webkit-patch does not prompt username if the default one is incorrect
https://bugs.webkit.org/show_bug.cgi?id=34439
Summary
webkit-patch does not prompt username if the default one is incorrect
Victor Wang
Reported
2010-02-01 11:10:24 PST
webkit-patch grabs and shows a default user name when landing a patch. If the default one is incorrect, it should prompt user to change it. It would be also nice if the script supports a "username" command line option.
Attachments
Patch with unit tests
(6.93 KB, patch)
2010-02-05 00:20 PST
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Patch with unit tests
(7.17 KB, patch)
2010-02-18 08:17 PST
,
Daniel Bates
eric
: review-
Details
Formatted Diff
Diff
Patch with unit tests
(7.31 KB, patch)
2010-02-19 17:27 PST
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1
2010-02-01 13:42:07 PST
I'm not sure what the "default" username is. And is this for bugzilla or svn?
Daniel Bates
Comment 2
2010-02-01 13:46:07 PST
Not sure if this bug is related, but I can confirm that for svn, unless you have initially checked out the repo. with a username or committed a change by hand (and overridden the username with the --username flag) then the default username is the login username. (In reply to
comment #1
)
> I'm not sure what the "default" username is. > > And is this for bugzilla or svn?
Victor Wang
Comment 3
2010-02-01 15:00:10 PST
This is for svn. Agree with Daniel, sounds like the default username is from the login username. In this bug, I am more concerning about webkit-patch does not prompt "username" option if the one it tries is incorrect. I would like to be able to specify a username when I run the script or enter the right one if the one the script is using is incorrect.
Daniel Bates
Comment 4
2010-02-05 00:20:02 PST
Created
attachment 48209
[details]
Patch with unit tests I am open to suggestions on better names for methods and variables as well as suggestions on the approach.
Adam Barth
Comment 5
2010-02-07 19:07:40 PST
The code around raw_input should be in the user.py module. Maybe add a retry option to prompt() somehow? I think we have a similar idiom in bugzilla.py for the bugzilla password that should use the same code.
Daniel Bates
Comment 6
2010-02-08 00:16:38 PST
Comment on
attachment 48209
[details]
Patch with unit tests Will look into factoring the code based on Adam's comment.
Daniel Bates
Comment 7
2010-02-18 08:17:14 PST
Created
attachment 49015
[details]
Patch with unit tests
Eric Seidel (no email)
Comment 8
2010-02-19 16:18:58 PST
Comment on
attachment 49015
[details]
Patch with unit tests I would have made this generic: 307 def has_webkit_authorization_file(home_directory=os.getenv("HOME")): not webkit-specific. Or at least have a generic version. Since it looks at the values from SVN itself, it seems no need to have _webkit_ in the name. It could use those values as default values to arguments passed to it. I would have written this using List.extend: 373 svn_commit_args.append('--username') 374 svn_commit_args.append(username) svn_commit_args.extend(["--username", username]) I think we're trying to standardize on " instead of ' anyway. You can ask Adam for more infos. Making this static just makes it harder to mock/unit test: 6 @staticmethod 307 def has_webkit_authorization_file(home_directory=os.getenv("HOME")): I suggest not making it static. Otherwise I think this looks OK. Clearly we don't have any unit testing of the actual commit, since the SVN.has_webkit_authorization_file call would actually be hitting disk for the real auth file in those cases! r- for the above nits.
Eric Seidel (no email)
Comment 9
2010-02-19 16:20:34 PST
svn_server_realm = "<
http://svn.webkit.org:80
> Mac OS Forge" 261 svn_server_host = "svn.webkit.org" Should just be instance variables. They can have some class-level defaults if you like. But eventually we really need some sort of webkit_config.py module which holds all this webkit-specific stuff. Not that we ever plan to re-use webkit-patch for some other project, but it makes things read cleaner if we have all the webkit-specifc config in as few places as possible.
Daniel Bates
Comment 10
2010-02-19 17:27:20 PST
Created
attachment 49114
[details]
Patch with unit tests Updated patch based on Eric's comments.
Eric Seidel (no email)
Comment 11
2010-02-22 13:03:53 PST
Comment on
attachment 49114
[details]
Patch with unit tests OK.
WebKit Commit Bot
Comment 12
2010-02-23 01:31:09 PST
Comment on
attachment 49114
[details]
Patch with unit tests Clearing flags on attachment: 49114 Committed
r55131
: <
http://trac.webkit.org/changeset/55131
>
WebKit Commit Bot
Comment 13
2010-02-23 01:31:14 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug