Make git-freeze bail out if the user has too much untracked data.

Make git-freeze bail out if the user has too much untracked data.

R=iannucci@chromium.org
BUG=376099
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/02b3c98b76feeeb87c60c347ccc2fcf4512b7d95

[agable, 2016-06-09 22:33:44]

resurrecting https://codereview.chromium.org/311243003/

[iannucci, 2016-06-16 00:24:20]

oops forgot to publish comments

https://codereview.chromium.org/2052113002/diff/1/git_common.py
File git_common.py (right):

https://codereview.chromium.org/2052113002/diff/1/git_common.py#newcode348
git_common.py:348: def get_config(option, default=None):
would you mind terribly doing the config and die refactors in another CL first?

https://codereview.chromium.org/2052113002/diff/1/git_common.py#newcode403
git_common.py:403: untracked_size = sum(
I would consider doing this as a proper loop: if the user has many many files,
this will end up doing a whole bunch of stat's that aren't needed.

https://codereview.chromium.org/2052113002/diff/1/git_common.py#newcode792
git_common.py:792: return dict(parser(tokenizer(run_stream('status', '-z',
bufsize=-1))))
why dict-ify this if you're just going to stream it anyway? may as well yield a
series of tuples from this function (and if someone wants a dict they can just
wrap this with dict())

[agable, 2016-06-16 12:47:32]

https://codereview.chromium.org/2052113002/diff/1/git_common.py
File git_common.py (right):

https://codereview.chromium.org/2052113002/diff/1/git_common.py#newcode348
git_common.py:348: def get_config(option, default=None):
On 2016/06/16 at 00:24:20, iannucci wrote:
> would you mind terribly doing the config and die refactors in another CL
first?

Done: https://codereview.chromium.org/2075603002

[agable, 2016-06-16 15:26:55]

https://codereview.chromium.org/2052113002/diff/1/git_common.py
File git_common.py (right):

https://codereview.chromium.org/2052113002/diff/1/git_common.py#newcode403
git_common.py:403: untracked_size = sum(
On 2016/06/16 at 00:24:20, iannucci wrote:
> I would consider doing this as a proper loop: if the user has many many files,
this will end up doing a whole bunch of stat's that aren't needed.

Done.

https://codereview.chromium.org/2052113002/diff/1/git_common.py#newcode792
git_common.py:792: return dict(parser(tokenizer(run_stream('status', '-z',
bufsize=-1))))
On 2016/06/16 at 00:24:20, iannucci wrote:
> why dict-ify this if you're just going to stream it anyway? may as well yield
a series of tuples from this function (and if someone wants a dict they can just
wrap this with dict())

Done. Resulted in some more efficient code in status() as well.

[agable, 2016-06-21 14:11:58]

agable@chromium.org changed reviewers:
+ tandrii@chromium.org

[agable, 2016-06-21 14:11:59]

+tandrii since iannucci@ is on vacation, and because this depends on
https://codereview.chromium.org/2075603002

[tandrii(chromium), 2016-06-21 14:43:38]

https://codereview.chromium.org/2052113002/diff/60001/git_common.py
File git_common.py (right):

https://codereview.chromium.org/2052113002/diff/60001/git_common.py#newcode404
git_common.py:404: untracked_size += os.stat(f).st_size / MB
don't divide here, otherwise 3 files 400KB each will amount to 0MB.

https://codereview.chromium.org/2052113002/diff/60001/git_common.py#newcode405
git_common.py:405: if untracked_size > limit_mb:
divide by MB here, and better yet multiply by limit_mb by MB.

https://codereview.chromium.org/2052113002/diff/60001/git_common.py#newcode408
git_common.py:408: checkout: %d/%dMB.
personal preference: s/%d/%.1f and make MB a float and then untracked_size/MB.

https://codereview.chromium.org/2052113002/diff/60001/git_common.py#newcode768
git_common.py:768: s = acc.getvalue()
nit: if you'd had used plain StringIO, which for this use case seems totally
fine, you could then rewrite this block as:

if acc.len:
  yield acc.getvalue()
  acc = StringIO()

https://codereview.chromium.org/2052113002/diff/60001/git_common.py#newcode782
git_common.py:782: stat, dest = status_dest[:2], status_dest[3:]
suggestion: why not plain old except to detect END?

while True:
  try:
    status_dest = next(tokens)
  except StopIteration:
    return

and below
  src = next(tokens)
  no need for assert, as above line will raise if not end.

https://codereview.chromium.org/2052113002/diff/60001/tests/git_common_test.py
File tests/git_common_test.py (right):

https://codereview.chromium.org/2052113002/diff/60001/tests/git_common_test.p...
tests/git_common_test.py:832: with open('bigfile', 'w') as f:
3 files, 400KB each, please :)

[agable, 2016-06-22 11:16:51]

https://codereview.chromium.org/2052113002/diff/60001/git_common.py
File git_common.py (right):

https://codereview.chromium.org/2052113002/diff/60001/git_common.py#newcode404
git_common.py:404: untracked_size += os.stat(f).st_size / MB
On 2016/06/21 at 14:43:37, tandrii(chromium) wrote:
> don't divide here, otherwise 3 files 400KB each will amount to 0MB.

Done

https://codereview.chromium.org/2052113002/diff/60001/git_common.py#newcode405
git_common.py:405: if untracked_size > limit_mb:
On 2016/06/21 at 14:43:37, tandrii(chromium) wrote:
> divide by MB here, and better yet multiply by limit_mb by MB.

Done

https://codereview.chromium.org/2052113002/diff/60001/git_common.py#newcode408
git_common.py:408: checkout: %d/%dMB.
On 2016/06/21 at 14:43:37, tandrii(chromium) wrote:
> personal preference: s/%d/%.1f and make MB a float and then untracked_size/MB.

Gonna leave limit in MB, since letting it be a decimal is vaguely silly. But did
change the format to print a float for the amount of untracked data.

https://codereview.chromium.org/2052113002/diff/60001/git_common.py#newcode768
git_common.py:768: s = acc.getvalue()
On 2016/06/21 at 14:43:37, tandrii(chromium) wrote:
> nit: if you'd had used plain StringIO, which for this use case seems totally
fine, you could then rewrite this block as:
> 
> if acc.len:
>   yield acc.getvalue()
>   acc = StringIO()

Good point, done.

https://codereview.chromium.org/2052113002/diff/60001/git_common.py#newcode782
git_common.py:782: stat, dest = status_dest[:2], status_dest[3:]
On 2016/06/21 at 14:43:37, tandrii(chromium) wrote:
> suggestion: why not plain old except to detect END?
> 
> while True:
>   try:
>     status_dest = next(tokens)
>   except StopIteration:
>     return
> 
> and below
>   src = next(tokens)
>   no need for assert, as above line will raise if not end.

Nice, done.

https://codereview.chromium.org/2052113002/diff/60001/tests/git_common_test.py
File tests/git_common_test.py (right):

https://codereview.chromium.org/2052113002/diff/60001/tests/git_common_test.p...
tests/git_common_test.py:832: with open('bigfile', 'w') as f:
On 2016/06/21 at 14:43:37, tandrii(chromium) wrote:
> 3 files, 400KB each, please :)

Added another test case, which fails without responding to your comments in
git_common but passes with them.

[tandrii(chromium), 2016-06-22 14:20:50]

LGTM % comment for even shorter parser()

https://codereview.chromium.org/2052113002/diff/60001/tests/git_common_test.py
File tests/git_common_test.py (right):

https://codereview.chromium.org/2052113002/diff/60001/tests/git_common_test.p...
tests/git_common_test.py:832: with open('bigfile', 'w') as f:
On 2016/06/22 11:16:51, agable wrote:
> On 2016/06/21 at 14:43:37, tandrii(chromium) wrote:
> > 3 files, 400KB each, please :)
> 
> Added another test case, which fails without responding to your comments in
> git_common but passes with them.

Acknowledged.

https://codereview.chromium.org/2052113002/diff/80001/git_common.py
File git_common.py (right):

https://codereview.chromium.org/2052113002/diff/80001/git_common.py#newcode426
git_common.py:426: untracked_bytes / (MB * 1.0), limit_mb, key)
*1.0 is also my preference, but i thought infra preferred explicit / float(MB).
Keep it as is please :)

https://codereview.chromium.org/2052113002/diff/80001/git_common.py#newcode782
git_common.py:782: return
actually, since this parser is also a generator, we don't even need
try-except-return, because return in a generator is sugar for raise
StopIteration()

[agable, 2016-06-22 14:43:39]

https://codereview.chromium.org/2052113002/diff/80001/git_common.py
File git_common.py (right):

https://codereview.chromium.org/2052113002/diff/80001/git_common.py#newcode782
git_common.py:782: return
On 2016/06/22 at 14:20:50, tandrii(chromium) wrote:
> actually, since this parser is also a generator, we don't even need
try-except-return, because return in a generator is sugar for raise
StopIteration()

Shiny, of course! Done.

[tandrii(chromium), 2016-06-22 14:45:55]

lgtm

[agable, 2016-06-22 14:47:23]

The CQ bit was checked by agable@chromium.org

[commit-bot: I haz the power, 2016-06-22 14:47:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052113002/100001

[commit-bot: I haz the power, 2016-06-22 14:51:24]

Description was changed from

==========
Make git-freeze bail out if the user has too much untracked data.

R=iannucci@chromium.org
BUG=376099
==========

to

==========
Make git-freeze bail out if the user has too much untracked data.

R=iannucci@chromium.org
BUG=376099

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/02b3c98b76feee...
==========

[commit-bot: I haz the power, 2016-06-22 14:51:26]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/02b3c98b76feee...