Make git try work over svn

[thakis, 2009-09-01 18:27:51]

The --user is merely a convenience, but the --non-interactive breaks stuff when
I use `git try`.

I tested that bot `gcl try` and `git try` work with this change (without this
change, `git try` didn't work for trungl and me).

[chase, 2009-09-01 18:57:12]

http://codereview.appspot.com/110134/diff/3/1002
File trychange.py (right):

http://codereview.appspot.com/110134/diff/3/1002#newcode316
Line 316: '--username', options.user + '@chromium.org',
git-try invokes trychange with both -u and -e.  git-try gets -e from the git
checkout's configured email address, and gets -u by stripping everything at and
after the @ from -e.

I suggest options.email here to get the original address rather than recreating
it with (possibly) a bad @host part.

http://codereview.appspot.com/110134/diff/3/1002#newcode326
Line 326: RunCommand(['svn', 'ls', full_url])
I don't understand fully why I was able to submit a try job via svn from git-try
and you weren't.  Before I tried locally, I accessed the chrome-try repository
with 'svn ls --username=... svn://svn.chromium.org/chrome-try'.

I wonder if the auth data was not cached correctly somehow in your copy, or the
use of --non-interactive impeded svn from accessing the cached data.  Just
guesses at this point.

[thakis, 2009-09-01 19:04:50]

Thanks for the review. Good idea using options.email.

http://codereview.appspot.com/110134/diff/3/1002
File trychange.py (right):

http://codereview.appspot.com/110134/diff/3/1002#newcode316
Line 316: '--username', options.user + '@chromium.org',
On 2009/09/01 18:57:13, chase wrote:
> git-try invokes trychange with both -u and -e.  git-try gets -e from the git
> checkout's configured email address, and gets -u by stripping everything at
and
> after the @ from -e.
> 
> I suggest options.email here to get the original address rather than
recreating
> it with (possibly) a bad @host part.

Done.

http://codereview.appspot.com/110134/diff/3/1002#newcode326
Line 326: RunCommand(['svn', 'ls', full_url])
I have no idea either. But for me it works now, while it didn't before. While
that's not an acceptable basis for a patch that goes into shipping end-user
software, I feel it's ok for infrastructure like this, where it's more about
saving the other devs the 20 minutes they need to do this change themselves.

Since I don't have to enter anything, it sounds more plausible that svn doesn't
want to access the cached data when this flag is present.

[M-A, 2009-09-01 19:31:53]

lgtm

One of the thing that keeps coming about the svn support is "Which password
should I enter there?". Maybe trychange should preemptively print a line about
using svn and that it may be asked for a svn credential.

[M-A, 2009-09-01 19:33:05]

Oh and you misconfigured your depot_tools.git checkout, you should use
codereview.chromium.org and cc chromium-reviews@googlegroups.com. Too bad it's
not automatic.

[chase, 2009-09-01 19:41:21]

lgtm

http://codereview.appspot.com/110134/diff/3/1002
File trychange.py (right):

http://codereview.appspot.com/110134/diff/3/1002#newcode326
Line 326: RunCommand(['svn', 'ls', full_url])
I was thinking about whether things would break for devs it already works for
and costing them time once this change is made.  Anyway, not a big deal.  I
don't suspect this will break them, that feels like an outside chance to me.  As
you say, your patch makes trychange work for you and trungl and that's worth a
lot.

[Mark Mentovai, 2009-09-02 03:13:15]

http://codereview.appspot.com/110134/diff/5/1005
File trychange.py (right):

http://codereview.appspot.com/110134/diff/5/1005#newcode316
Line 316: '--username', options.email,
This line has caused:

Traceback (most recent call last):
  File "/chrome/depot_tools/gcl.py", line 1245, in <module>
    sys.exit(main())
  File "/chrome/depot_tools/gcl.py", line 1233, in main
    TryChange(change_info, args, swallow_exception=False)
  File "/chrome/depot_tools/gcl.py", line 900, in TryChange
    prog='gcl try')
  File "/chrome/depot_tools/trychange.py", line 519, in TryChange
    patch_name = options.send_patch(options)
  File "/chrome/depot_tools/trychange.py", line 317, in _SendChangeSVN
    options.svn_repo, temp_dir])
  File "/chrome/depot_tools/trychange.py", line 108, in RunCommand
    output, retcode = gcl.RunShellWithReturnCode(command)
  File "/chrome/depot_tools/gcl.py", line 213, in RunShellWithReturnCode
    universal_newlines=True)
  File
"/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/subprocess.py",
line 593, in __init__
    errread, errwrite)
  File
"/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/subprocess.py",
line 1079, in _execute_child
    raise child_exception
TypeError: execv() arg 2 must contain only strings

when options.email is None, which it often is when EMAIL_ADDRESS is unset and
--email is not provided.

Perhaps it's reasonable to require EMAIL_ADDRESS or --email, but "gcl try"
should not fail in such an obscure way.

[thakis, 2009-09-02 03:21:02]

http://codereview.appspot.com/110134/diff/5/1005
File trychange.py (right):

http://codereview.appspot.com/110134/diff/5/1005#newcode316
Line 316: '--username', options.email,
Sorry 'bout that. What about:

  cmd = [...]
  if options.email:
    cmd += ['--username', options.email]
  RunCommand(cmd)

to fix this?