Misc gclient cleanup

Misc gclient cleanup

- Use config --local for properties that should be local.
- Remove git fetch retry logic, it should not be necessary
  with the googlesource hosts.
- Include .strip() in GIT.Capture, simplifying other calls.
- Safer testing of whether dep revision is a sha.
- Fix refs/remotes/ DEPS branch detection

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

[Isaac (away), 2013-07-02 04:11:11]

ping?

[iannucci, 2013-07-02 04:32:01]

https://chromiumcodereview.appspot.com/18262002/diff/5001/gclient_scm.py
File gclient_scm.py (left):

https://chromiumcodereview.appspot.com/18262002/diff/5001/gclient_scm.py#oldc...
gclient_scm.py:415: except subprocess2.CalledProcessError, e:
So... Why drop all the retries? What's the alternative?

https://chromiumcodereview.appspot.com/18262002/diff/5001/gclient_scm.py
File gclient_scm.py (right):

https://chromiumcodereview.appspot.com/18262002/diff/5001/gclient_scm.py#newc...
gclient_scm.py:405: if not scm.GIT.IsValidRevision(self.checkout_path, revision,
sha_only=True):
How does this work if we don't have that hash yet? I don't think we're
guaranteed to have fetched yet....

https://chromiumcodereview.appspot.com/18262002/diff/5001/scm.py
File scm.py (left):

https://chromiumcodereview.appspot.com/18262002/diff/5001/scm.py#oldcode124
scm.py:124: status = GIT.Capture(command, cwd).rstrip()
This was rstrip for a reason?

https://chromiumcodereview.appspot.com/18262002/diff/5001/scm.py
File scm.py (right):

https://chromiumcodereview.appspot.com/18262002/diff/5001/scm.py#newcode169
scm.py:169: GIT.Capture(['config', '--local', '--get-regexp', r'^svn-remote\.'],
Who would have non-local svn-remote properties?

https://chromiumcodereview.appspot.com/18262002/diff/5001/scm.py#newcode244
scm.py:244: ['config', '--local', '--get-regexp', r'^svn-remote\..*\.url'],
Refactor this prefix?

[Isaac (away), 2013-07-02 04:43:50]

https://chromiumcodereview.appspot.com/18262002/diff/5001/gclient_scm.py
File gclient_scm.py (left):

https://chromiumcodereview.appspot.com/18262002/diff/5001/gclient_scm.py#oldc...
gclient_scm.py:415: except subprocess2.CalledProcessError, e:
On 2013/07/02 04:32:02, iannucci wrote:
> So... Why drop all the retries? What's the alternative?

Well I don't think we were retrying before.  Only in the case of a 502 error.  I
don't like that this repeatedly hitting a broken server, and I assume git has
built in retry logic too.

https://chromiumcodereview.appspot.com/18262002/diff/5001/gclient_scm.py
File gclient_scm.py (right):

https://chromiumcodereview.appspot.com/18262002/diff/5001/gclient_scm.py#newc...
gclient_scm.py:405: if not scm.GIT.IsValidRevision(self.checkout_path, revision,
sha_only=True):
On 2013/07/02 04:32:02, iannucci wrote:
> How does this work if we don't have that hash yet? I don't think we're
> guaranteed to have fetched yet....

In that case, function returns false and we fetch.

[iannucci, 2013-07-02 08:50:48]

This basically lgtm (mod nits already up). I'm a bit queasy about so many
seemingly-innocuous changes without good test coverage, but that's not the fault
of this CL. If you land, I would definitely keep an eye on the bots and an eye
on chromium-dev for a bit.

[Isaac (away), 2013-07-03 00:05:28]

committing

https://chromiumcodereview.appspot.com/18262002/diff/5001/scm.py
File scm.py (left):

https://chromiumcodereview.appspot.com/18262002/diff/5001/scm.py#oldcode124
scm.py:124: status = GIT.Capture(command, cwd).rstrip()
On 2013/07/02 04:32:02, iannucci wrote:
> This was rstrip for a reason?

No, I don't think so.  It was added before the other calls by Chase in Oct 2009
(https://codereview.chromium.org/242140)

https://chromiumcodereview.appspot.com/18262002/diff/5001/scm.py
File scm.py (right):

https://chromiumcodereview.appspot.com/18262002/diff/5001/scm.py#newcode169
scm.py:169: GIT.Capture(['config', '--local', '--get-regexp', r'^svn-remote\.'],
On 2013/07/02 04:32:02, iannucci wrote:
> Who would have non-local svn-remote properties?

Who knows, but not taking risk. I assume it also makes git a tiny bit faster.

https://chromiumcodereview.appspot.com/18262002/diff/5001/scm.py#newcode244
scm.py:244: ['config', '--local', '--get-regexp', r'^svn-remote\..*\.url'],
On 2013/07/02 04:32:02, iannucci wrote:
> Refactor this prefix?

Postponed for a future CL.

[commit-bot: I haz the power, 2013-07-03 00:07:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ilevy@chromium.org/18262002/13001

[commit-bot: I haz the power, 2013-07-03 00:09:13]

Change committed as 209825

[M-A Ruel, 2013-07-08 18:44:41]

Only a small nit to make it more case-proof

https://chromiumcodereview.appspot.com/18262002/diff/13001/scm.py
File scm.py (right):

https://chromiumcodereview.appspot.com/18262002/diff/13001/scm.py#newcode460
scm.py:460: return rev == sha
what about lower/upper case?
return rev.lower() == sha.lower()

https://chromiumcodereview.appspot.com/18262002/diff/13001/scm.py#newcode462
scm.py:462: return sha.startswith(rev)
sha.lower().startswith(rev.lower())

[Isaac (away), 2013-07-08 20:03:34]

https://chromiumcodereview.appspot.com/18262002/diff/13001/scm.py
File scm.py (right):

https://chromiumcodereview.appspot.com/18262002/diff/13001/scm.py#newcode460
scm.py:460: return rev == sha
On 2013/07/08 18:44:41, Marc-Antoine Ruel wrote:
> what about lower/upper case?
> return rev.lower() == sha.lower()

Good point.  It looks like git always returns lower case characters but I'll add
lower() on git output for safety.  I'll send out a follow up CL.