Warn when committing code with http:// URLs.

Warn when committing code with http:// URLs.

We prefer HTTPS for everything, to the extent possible.

NOTRY=true
BUG=156396
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176986

[palmer, 2013-01-08 02:37:50]

This is my first time making a presubmit rule, so I figure I've done it at least
somewhat wrong. Let me know. :)

[M-A Ruel, 2013-01-08 02:51:11]

You may want a test but I don't mind.

https://codereview.chromium.org/11779037/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11779037/diff/1/PRESUBMIT.py#newcode241
PRESUBMIT.py:241: inclusion_pattern = input_api.re.compile(r'http:/',
input_api.re.IGNORECASE)
Why not http:// ?

https://codereview.chromium.org/11779037/diff/1/PRESUBMIT.py#newcode242
PRESUBMIT.py:242: # Allow http:/ in comments? Only if it turns out to be
necessary.
At least for me, I know I've been using http://chromium.org/dev/foo/bar in
comments because it's shorter and because https:// didn't work. As an example:
https://chromium.org/developers/testing/isolated-testing will trigger an invalid
certificate. But the https equivalent is much longer:
https://sites.google.com/a/chromium.org/dev/developers/testing/isolated-testing

https://codereview.chromium.org/11779037/diff/1/PRESUBMIT.py#newcode250
PRESUBMIT.py:250: black_list=())
no need to put black_list then.

https://codereview.chromium.org/11779037/diff/1/PRESUBMIT.py#newcode257
PRESUBMIT.py:257: for line in lines:
for line_number, line in enumerate(input_api.ReadFile(f).splitlines()):

https://codereview.chromium.org/11779037/diff/1/PRESUBMIT.py#newcode265
PRESUBMIT.py:265: if not input_api.is_committing:
Remove the not and inverse the lines:
  if input_api.is_committing:
It's less cognitive overhead.

https://codereview.chromium.org/11779037/diff/1/PRESUBMIT.py#newcode270
PRESUBMIT.py:270: else:
optional style nit: "else:" is not needed

[palmer, 2013-01-08 21:05:40]

> Why not http:// ?

On the general principle of maximum matching/maximum paranoia. For example,
http:/important.com works when pasted into the Omnibox, so we should match it
and warn about it.

> At least for me, I know I've been using http://chromium.org/dev/foo/bar in
> comments because it's shorter and because https:// didn't work. As an example:
> https://chromium.org/developers/testing/isolated-testing will trigger an
invalid
> certificate. But the https equivalent is much longer:
>
https://sites.google.com/a/chromium.org/dev/developers/testing/isolated-testing

Yes, you are right. I updated the relevant infrastructure bugs, and I enabled
the exclusion_pattern. Sigh. :)

> no need to put black_list then.

Done.

> for line_number, line in enumerate(input_api.ReadFile(f).splitlines()):

Done.

> Remove the not and inverse the lines:
>   if input_api.is_committing:
> It's less cognitive overhead.

Done.

> optional style nit: "else:" is not needed

Done. I also fixed these problems throughout the file. (Coming up in another
patchset momentarily.)

[palmer, 2013-01-15 00:14:03]

Ping?

[M-A Ruel, 2013-01-15 00:36:43]

lgtm

[commit-bot: I haz the power, 2013-01-15 22:10:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/11779037/14001

[commit-bot: I haz the power, 2013-01-15 22:20:46]

Change committed as 176986

[nduca, 2013-01-16 02:32:47]

This is breaking tools/telemetry/ where this sort of thing is perfectly
acceptable. Please advise.

[palmer, 2013-01-16 18:36:27]

> This is breaking tools/telemetry/ where this sort of thing is perfectly
> acceptable. Please advise.

It shouldn't break anything; when committing, it only nags you but does not
prompt:

 254   if problems:
 255     if input_api.is_committing:
 256       # We don't warn on commit, to avoid stopping commits going through
CQ.
 257       return [output_api.PresubmitNotifyResult(_HTTPS_ONLY_WARNING,
problems)]
 258     else:
 259       return [output_api.PresubmitPromptWarning(_HTTPS_ONLY_WARNING,
problems)]

If you are sure the nags are all false positives, you don't have to worry about
it. From a quick skim it looks like they are.