adds a "safesync" parameter to gclient

adds a "safesync_url" option to .gclient solutions. Over-rideable with --head

[M-A, 2009-04-07 01:26:01]

So I'll discuss with other to make sure it's a good idea before the lgtm. Feel free to try to convince folks separately :) http://codereview.appspot.com/32135/diff/1/3 File gclient.py (right): http://codereview.appspot.com/32135/diff/1/3#newcode254 Line 254: # If a \"safesync_url\" is specified, it will be consulted when updating to the "consulted" how? why? I asks for the phase of the moon? That's unclear. Ok I *know* why but it still unclear. :) http://codereview.appspot.com/32135/diff/1/3#newcode265 Line 265: \"safesync_url\": \"%s\", Remove the last comma here otherwise it's not a valid json file. http://codereview.appspot.com/32135/diff/1/3#newcode1470 Line 1470: rev = handle.read() .strip() http://codereview.appspot.com/32135/diff/1/3#newcode1474 Line 1474: rev = str(int(rev)) I don't think it's needed. http://codereview.appspot.com/32135/diff/1/3#newcode1645 Line 1645: # vim: ai:ts=2:et:sw=2:tw=80: Please god forgives! A vim user! :) That's fine, you can leave the line.

[slightlylate, 2009-04-07 01:39:21]

http://codereview.appspot.com/32135/diff/1/3 File gclient.py (right): http://codereview.appspot.com/32135/diff/1/3#newcode254 Line 254: # If a \"safesync_url\" is specified, it will be consulted when updating to the On 2009/04/07 01:26:02, M-A wrote: > "consulted" how? why? I asks for the phase of the moon? That's unclear. Ok I > *know* why but it still unclear. :) Done. http://codereview.appspot.com/32135/diff/1/3#newcode265 Line 265: \"safesync_url\": \"%s\", On 2009/04/07 01:26:02, M-A wrote: > Remove the last comma here otherwise it's not a valid json file. Done. http://codereview.appspot.com/32135/diff/1/3#newcode1470 Line 1470: rev = handle.read() On 2009/04/07 01:26:02, M-A wrote: > .strip() Done. http://codereview.appspot.com/32135/diff/1/3#newcode1474 Line 1474: rev = str(int(rev)) On 2009/04/07 01:26:02, M-A wrote: > I don't think it's needed. Done.

[M-A, 2009-04-08 00:57:30]

I still have a few issues (gclient config is broken at the moment) but I definitely prefer this implementation. http://codereview.appspot.com/32135/diff/2001/2003 File gclient.py (right): http://codereview.appspot.com/32135/diff/2001/2003#newcode265 Line 265: \"safesync_url\": \"%s\" Humm. You're adding a parameter here, and a parameter to SetDefaultConfig() but you never SetDefaultConfig callee. I think you should write instead: # \"safesync_url\": \"\" http://codereview.appspot.com/32135/diff/2001/2003#newcode876 Line 876: def SetDefaultConfig(self, solution_name, solution_url, safesync_url): Revert modifications to this function http://codereview.appspot.com/32135/diff/2001/2003#newcode1449 Line 1449: This empty line is really extraneous. :) http://codereview.appspot.com/32135/diff/2001/2003#newcode1457 Line 1457: if len(rev): I prefer "if rev:" here since it will deny '0' http://codereview.appspot.com/32135/diff/2001/2003#newcode1462 Line 1462: if r.split("@")[0] == s["name"]: Shouldn't it check _before_ doing the urlopen call? The http stuff is much more "costly" than processing an array. http://codereview.appspot.com/32135/diff/2001/2003#newcode1503 Line 1503: I really miss this empty line. http://codereview.appspot.com/32135/diff/2001/2003#newcode1588 Line 1588: "configured solutions")) Usually, we align with the =(

[slightlylate, 2009-04-08 17:24:18]

On 2009/04/08 00:57:30, M-A wrote: > I still have a few issues (gclient config is broken at the moment) but I > definitely prefer this implementation. Thanks for the review. > http://codereview.appspot.com/32135/diff/2001/2003 > File gclient.py (right): > > http://codereview.appspot.com/32135/diff/2001/2003#newcode265 > Line 265: \"safesync_url\": \"%s\" > Humm. You're adding a parameter here, and a parameter to SetDefaultConfig() but > you never SetDefaultConfig callee. I think you should write instead: > # \"safesync_url\": \"\" This is done by the tests. > http://codereview.appspot.com/32135/diff/2001/2003#newcode876 > Line 876: def SetDefaultConfig(self, solution_name, solution_url, safesync_url): > Revert modifications to this function Again, not sure why...the test harness uses it. > http://codereview.appspot.com/32135/diff/2001/2003#newcode1449 > Line 1449: > This empty line is really extraneous. :) Fixed. > http://codereview.appspot.com/32135/diff/2001/2003#newcode1457 > Line 1457: if len(rev): > I prefer "if rev:" here since it will deny '0' > > http://codereview.appspot.com/32135/diff/2001/2003#newcode1462 > Line 1462: if r.split("@")[0] == s["name"]: > Shouldn't it check _before_ doing the urlopen call? The http stuff is much more > "costly" than processing an array. Fixed. > http://codereview.appspot.com/32135/diff/2001/2003#newcode1503 > Line 1503: > I really miss this empty line. Fixed. > http://codereview.appspot.com/32135/diff/2001/2003#newcode1588 > Line 1588: "configured solutions")) > Usually, we align with the =( Fixed.

[M-A, 2009-04-08 18:43:43]

lgtm with the 2 last changes. http://codereview.appspot.com/32135/diff/1020/24 File gclient.py (right): http://codereview.appspot.com/32135/diff/1020/24#newcode171 Line 171: usage: config [option | url] Change it to usage: config [options] url [safesync url] http://codereview.appspot.com/32135/diff/1020/24#newcode876 Line 876: def SetDefaultConfig(self, solution_name, solution_url, safesync_url=""): Could you not make safesync_url a default parameter?

[M-A, 2009-04-08 20:06:37]

Sorry, the test doesn't pass anymore. Please sync and fix TestDoConfig.