Process first branch status request before spawning threads.

Process first branch status request before spawning threads.

BUG=341052
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=266219

[jrobbins, 2014-03-26 23:21:40]

Robbie, in crbug.com/341052 you identified the problem as parallel threads all
prompting the user for authentication at the same time.   

In the rietveld upload.py file there is code that uses threads, but it always
does one request to /upload before spawning threads.

I think that CMDstatus() in git_cl.py is the actual place that spawns threads to
make requests to rietveld which caused the errors that the issue reporter saw.

I am taking the approach of sending one request first, then doing the rest in
parallel.  That first request should work through authentication and expose any
fatal errors (such as having the wrong hostname or no internet connectivity) to
the user just once.

[iannucci, 2014-03-26 23:34:26]

Thanks for digging into this. I'd be nice if we had a cogent protocol where we
could work out all the auth, etc. up-front instead of just "whatever fetch()
happens to do".

https://codereview.chromium.org/213573004/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/213573004/diff/1/git_cl.py#newcode1267
git_cl.py:1267: threads.append(threading.Thread(target=fetch, args=(b,)))
why not

if branches:
  fetch(branches[0])
  threads = [threading.Thread(target=fetch, args=(b,) for b in branches[1:]]

?

[Jason Robbins -- corp, 2014-03-26 23:43:07]

https://codereview.chromium.org/213573004/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/213573004/diff/1/git_cl.py#newcode1267
git_cl.py:1267: threads.append(threading.Thread(target=fetch, args=(b,)))
On 2014/03/26 23:34:26, iannucci wrote:
> why not
> 
> if branches:
>   fetch(branches[0])
>   threads = [threading.Thread(target=fetch, args=(b,) for b in branches[1:]]
> 
> ?

I had it that way until I realized that branches is a dictionary.   

If you like, I could go back to that by putting the list of the dictionary keys
in a new variable.  The variable naming seems awkward because the branches is
{branch: branch_url, ...}, sigh.

[M-A Ruel, 2014-03-26 23:53:55]

What about "git cl auth" or a formal auth.py like the one at
https://code.google.com/p/swarming/source/browse/auth.py?repo=client

cc'ing Vadim, who write it.

[Jason Robbins -- corp, 2014-03-27 21:02:07]

iannucci: Here's the change we discussed in chat.

M-A: Interesting, but I'd like to keep this change small and not introduce any
new subcommands.

[iannucci, 2014-04-04 00:42:42]

lgtm assuming you still wanted to land this, sorry for the review lag.

[Jason Robbins -- corp, 2014-04-09 19:47:42]

On 2014/04/04 00:42:42, iannucci wrote:
> lgtm assuming you still wanted to land this, sorry for the review lag.

This is my first change to depot_tools, how exactly do I land it?
How do I verify that I have commit permission?

[iannucci, 2014-04-10 18:09:33]

On 2014/04/09 19:47:42, Jason Robbins (corp) wrote:
> On 2014/04/04 00:42:42, iannucci wrote:
> > lgtm assuming you still wanted to land this, sorry for the review lag.
> 
> This is my first change to depot_tools, how exactly do I land it?
> How do I verify that I have commit permission?

You can tick the Commit checkbox and CQ will pick it up

[Jason Robbins -- corp, 2014-04-25 19:03:55]

The CQ bit was checked by jrobbins@google.com

[commit-bot: I haz the power, 2014-04-25 19:04:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrobbins@chromium.org/213573004/20001

[commit-bot: I haz the power, 2014-04-25 19:06:37]

Change committed as 266219