Increase coverage to 91%; Much stricter about header parsing.

Increase coverage to 91%; Much stricter about header parsing.

Add is_new to be used in a later patch by checkout classes.

R=dpranke@chromium.org
BUG=
TEST=

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

[M-A Ruel, 2011-06-03 17:12:37]

(Updated description)

[Dirk Pranke, 2011-06-03 19:17:27]

generally looks fine. Assuming you agree it makes sense to break up the big
verify_ functions, I'd like to take another quick pass over the result before
approving.

http://codereview.chromium.org/7054048/diff/1/patch.py
File patch.py (right):

http://codereview.chromium.org/7054048/diff/1/patch.py#newcode158
patch.py:158: def _verify_git_header(self):
This function is getting awfully long. Can it be broken up?

http://codereview.chromium.org/7054048/diff/1/patch.py#newcode202
patch.py:202: def process_line(line):
Does this really need to be a nested function?

http://codereview.chromium.org/7054048/diff/1/tests/checkout_test.py
File tests/checkout_test.py (right):

http://codereview.chromium.org/7054048/diff/1/tests/checkout_test.py#newcode155
tests/checkout_test.py:155: patch.FilePatchBinary('bin_file', '\x00', [], True),
Nit: can you change this to is_new=True just so it's clear which flag is being
set?

http://codereview.chromium.org/7054048/diff/1/tests/patch_test.py
File tests/patch_test.py (right):

http://codereview.chromium.org/7054048/diff/1/tests/patch_test.py#newcode160
tests/patch_test.py:160: c = patch.FilePatchBinary('foo', 'data', [], False)
Nit: I'd normally name the parameter here as well (and below).

[M-A Ruel, 2011-06-03 19:42:31]

http://codereview.chromium.org/7054048/diff/1/patch.py
File patch.py (right):

http://codereview.chromium.org/7054048/diff/1/patch.py#newcode158
patch.py:158: def _verify_git_header(self):
On 2011/06/03 19:17:27, Dirk Pranke wrote:
> This function is getting awfully long. Can it be broken up?

done

http://codereview.chromium.org/7054048/diff/1/patch.py#newcode202
patch.py:202: def process_line(line):
On 2011/06/03 19:17:27, Dirk Pranke wrote:
> Does this really need to be a nested function?

not anymore. so much state to keep

http://codereview.chromium.org/7054048/diff/1/tests/checkout_test.py
File tests/checkout_test.py (right):

http://codereview.chromium.org/7054048/diff/1/tests/checkout_test.py#newcode155
tests/checkout_test.py:155: patch.FilePatchBinary('bin_file', '\x00', [], True),
On 2011/06/03 19:17:27, Dirk Pranke wrote:
> Nit: can you change this to is_new=True just so it's clear which flag is being
> set?

done

http://codereview.chromium.org/7054048/diff/1/tests/patch_test.py
File tests/patch_test.py (right):

http://codereview.chromium.org/7054048/diff/1/tests/patch_test.py#newcode160
tests/patch_test.py:160: c = patch.FilePatchBinary('foo', 'data', [], False)
On 2011/06/03 19:17:27, Dirk Pranke wrote:
> Nit: I'd normally name the parameter here as well (and below).

done everywhere

[Dirk Pranke, 2011-06-03 19:51:15]

LGTM, thanks.