Bug 34439

Summary: webkit-patch does not prompt username if the default one is incorrect
Product: WebKit Reporter: Victor Wang <victorw>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: abarth, commit-queue, dbates, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Description Flags
Patch with unit tests
Patch with unit tests
eric: review-
Patch with unit tests none

Description Victor Wang 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.
Comment 1 Eric Seidel (no email) 2010-02-01 13:42:07 PST
I'm not sure what the "default" username is.

And is this for bugzilla or svn?
Comment 2 Daniel Bates 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?
Comment 3 Victor Wang 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.
Comment 4 Daniel Bates 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.
Comment 5 Adam Barth 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.
Comment 6 Daniel Bates 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.
Comment 7 Daniel Bates 2010-02-18 08:17:14 PST
Created attachment 49015 [details]
Patch with unit tests
Comment 8 Eric Seidel (no email) 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.
Comment 9 Eric Seidel (no email) 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.
Comment 10 Daniel Bates 2010-02-19 17:27:20 PST
Created attachment 49114 [details]
Patch with unit tests

Updated patch based on Eric's comments.
Comment 11 Eric Seidel (no email) 2010-02-22 13:03:53 PST
Comment on attachment 49114 [details]
Patch with unit tests

Comment 12 WebKit Commit Bot 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>
Comment 13 WebKit Commit Bot 2010-02-23 01:31:14 PST
All reviewed patches have been landed.  Closing bug.