Make apply_issue fail if the content of a large file is missing.

Make apply_issue fail if the content of a large file is missing.

Currently apply_issue fail if the content of a newly added file is
missing (because of the 900ko threshold in upload.py). But this doesn't
apply to the large file that get modified (instead of being added). This
is wrong because apply_issue will indicates a success without really
applying the patch.

It looks like the patch content is set to "None" in this cases (the string
value, not the empty value None).

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

[Sébastien Marchand, 2016-01-22 19:19:56]

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

[Sébastien Marchand, 2016-01-22 19:19:56]

PTAL.

[tandrii(chromium), 2016-01-22 21:08:25]

LGTM

thanks!

https://codereview.chromium.org/1612323004/diff/1/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/1612323004/diff/1/rietveld.py#newcode151
rietveld.py:151: if not content or content == 'None':
ugh, but whatever works.

[Sébastien Marchand, 2016-01-22 21:11:21]

https://codereview.chromium.org/1612323004/diff/1/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/1612323004/diff/1/rietveld.py#newcode151
rietveld.py:151: if not content or content == 'None':
On 2016/01/22 21:08:25, tandrii(chromium) wrote:
> ugh, but whatever works.

Yeah, this is ugly and it took me quite some time to debug this (because I was
printing the value of |content|, so I was confused by the fact that no exception
was raised although content was 'None'...). I don't know who's emitting this
string, but we should probably change it to something less confusing (or set it
to the 'real' None).

[Sébastien Marchand, 2016-01-22 21:11:26]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2016-01-22 21:11:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1612323004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1612323004/1

[commit-bot: I haz the power, 2016-01-22 21:13:24]

Description was changed from

==========
Make apply_issue fail if the content of a large file is missing.

Currently apply_issue fail if the content of a newly added file is
missing (because of the 900ko threshold in upload.py). But this doesn't
apply to the large file that get modified (instead of being added). This
is wrong because apply_issue will indicates a success without really
applying the patch.

It looks like the patch content is set to "None" in this cases (the string
value, not the empty value None).
==========

to

==========
Make apply_issue fail if the content of a large file is missing.

Currently apply_issue fail if the content of a newly added file is
missing (because of the 900ko threshold in upload.py). But this doesn't
apply to the large file that get modified (instead of being added). This
is wrong because apply_issue will indicates a success without really
applying the patch.

It looks like the patch content is set to "None" in this cases (the string
value, not the empty value None).

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

[commit-bot: I haz the power, 2016-01-22 21:13:25]

Committed patchset #1 (id:1) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=298366

[tandrii(chromium), 2016-01-22 21:26:07]

On 2016/01/22 21:11:21, Sébastien Marchand wrote:
> https://codereview.chromium.org/1612323004/diff/1/rietveld.py
> File rietveld.py (right):
> 
> https://codereview.chromium.org/1612323004/diff/1/rietveld.py#newcode151
> rietveld.py:151: if not content or content == 'None':
> On 2016/01/22 21:08:25, tandrii(chromium) wrote:
> > ugh, but whatever works.
> 
> Yeah, this is ugly and it took me quite some time to debug this (because I was
> printing the value of |content|, so I was confused by the fact that no
exception
> was raised although content was 'None'...). I don't know who's emitting this
> string, but we should probably change it to something less confusing (or set
it
> to the 'real' None).

if you file a bug for Infra-codereview, we might have a look, though thanks to
this patch of yours, we may never do so, as we intend to deprecate Rietveld.