If we're running in nohooks mode, don't bother to accumulate file_list in gclient.

If we're running in nohooks mode, don't bother to accumulate file_list in gclient.

This enables significant time savings, especially since file_list only exists to
enable file-specific hooks (which, AFAIK, nothing actually uses). On a z620
(linux) using the cached git repos, a first-time `gclient sync --nohooks` takes:
  * (with)    131.06s user 14.10s  system 117% cpu 2:03.89 total
  * (without) 482.13s user 189.35s system 144% cpu 7:45.63 total

This change makes nohooks cause file_list to be None if we don't need to
accumulate it, and updates GitWrapper and SvnWrapper appropriately.

R=szager@chromium.org
BUG=

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

[szager1, 2013-07-03 17:22:40]

lgtm (cringe) with nits.

This is a good change, but this code is fragile and perilous, I can't help but
duck and cover.

https://codereview.chromium.org/18541006/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/18541006/diff/1/gclient_scm.py#newcode227
gclient_scm.py:227: def cleanup(options, args, _file_list):
Thank you for marking unused args!

https://codereview.chromium.org/18541006/diff/1/gclient_scm.py#newcode332
gclient_scm.py:332: files = [] if file_list is not None else None
Lines like this make me hate python and miss perl.

https://codereview.chromium.org/18541006/diff/1/gclient_scm.py#newcode633
gclient_scm.py:633: if file_list is not None:
Combine this with the preceding 'if file_list is not None' stanza.

[iannucci, 2013-07-03 18:37:40]

Frankly, I'm inclined to rip the entire file_list crap out. It's inefficient,
and it's not safe (because it's not idempotent). If we DID actually have hooks
which used it, then they'd be flaky because if gclient crashed/was killed, it
loses the state of file_list between runs, and so on the second run you might
omit a hook which needed to be run!

A better implementation /might/ be to analyze all the hook filters, determine
which extensions to look for, and then flip on hooks which match files along the
way, accumulating those mark bits to disk before altering the state of the SCM.
Once all the hooks are flipped on, there's no more need to look for changing
files. For operations where the repo is cloned/checked out from scratch, we
would flip all the hooks on before running update.

An even better implementation would be to remove the functionality from gclient,
and make a library so that hooks like gyp can be smarter (timestamps, md5,
etc.), and gclient would just always run all hooks.

https://codereview.chromium.org/18541006/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/18541006/diff/1/gclient_scm.py#newcode227
gclient_scm.py:227: def cleanup(options, args, _file_list):
On 2013/07/03 17:22:40, szager1 wrote:
> Thank you for marking unused args!

Yeah they bug the heck out of me >_<

https://codereview.chromium.org/18541006/diff/1/gclient_scm.py#newcode332
gclient_scm.py:332: files = [] if file_list is not None else None
On 2013/07/03 17:22:40, szager1 wrote:
> Lines like this make me hate python and miss perl.

I'm actually OK with that. I think there should have been a shortcut in the
inline syntax like:

files = [] if file_list is not None

where the 'else None' is implied, if omitted. It also might have been nice to
have a None-detector syntax with '!' (which would be read as 'is None'), or
something of that sort:

files = [] if not file_list!

but oh well...

At the risk of Terrible Things: https://github.com/lihaoyi/macropy

https://codereview.chromium.org/18541006/diff/1/gclient_scm.py#newcode633
gclient_scm.py:633: if file_list is not None:
On 2013/07/03 17:22:40, szager1 wrote:
> Combine this with the preceding 'if file_list is not None' stanza.

I kept it the way it was because file_list will retain the new files, even if
one of the _Run's throws. I was inclined to combine them initially, but decided
against it for that reason.

[commit-bot: I haz the power, 2013-07-03 19:39:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/iannucci@chromium.org/18541006/9001

[commit-bot: I haz the power, 2013-07-03 19:41:05]

Change committed as 210026