Issue 3783 - Redirect auth page to review after upload

Issue 3783 - Redirect auth page to review after upload

[Wladimir Palant, 2016-03-10 14:17:19]

https://codereview.adblockplus.org/29338096/diff/29338097/hgreview.py
File hgreview.py (right):

https://codereview.adblockplus.org/29338096/diff/29338097/hgreview.py#newcode86
hgreview.py:86: execfile(upload_path, scope)
Side-effect of using execfile rather than subprocess.call: this should work on
Windows as well now (no Python interpreter there).

[Sebastian Noack, 2016-03-10 14:30:20]

https://codereview.adblockplus.org/29338096/diff/29338097/hgreview.py
File hgreview.py (right):

https://codereview.adblockplus.org/29338096/diff/29338097/hgreview.py#newcode86
hgreview.py:86: execfile(upload_path, scope)
Wow, downloading and executing code from the web that seems dangerous.

Any reason that it needs to inherit global varibles? Or couldn't we better pass
in a fresh disctionary?

[Wladimir Palant, 2016-03-10 14:52:18]

https://codereview.adblockplus.org/29338096/diff/29338097/hgreview.py
File hgreview.py (right):

https://codereview.adblockplus.org/29338096/diff/29338097/hgreview.py#newcode86
hgreview.py:86: execfile(upload_path, scope)
On 2016/03/10 14:30:20, Sebastian Noack wrote:
> Wow, downloading and executing code from the web that seems dangerous.

Oh, you only noticed now? :)

On the bright side, not any worse than where we are now - only that people are
supposed to download that file manually so far.

> Any reason that it needs to inherit global varibles? Or couldn't we better
pass
> in a fresh disctionary?

No reason, works both ways. Fixed.

[Sebastian Noack, 2016-03-10 15:08:23]

https://codereview.adblockplus.org/29338096/diff/29338105/hgreview.py
File hgreview.py (right):

https://codereview.adblockplus.org/29338096/diff/29338105/hgreview.py#newcode84
hgreview.py:84: port = random.randrange(2000, 60000)
I'm not sure whether it's a good idea to use a random port, at least not without
fallback logic if that port is already used. What if a different server, or an
instance of this script, is already running on the port we randomly choosed?

I think we should rather handle the error when the port is already in use and
fall back to another port. Whether the next port is picked incrementally or
randomly shouldn't matter much.

[Wladimir Palant, 2016-03-11 10:12:42]

https://codereview.adblockplus.org/29338096/diff/29338105/hgreview.py
File hgreview.py (right):

https://codereview.adblockplus.org/29338096/diff/29338105/hgreview.py#newcode84
hgreview.py:84: port = random.randrange(2000, 60000)
On 2016/03/10 15:08:22, Sebastian Noack wrote:
> I'm not sure whether it's a good idea to use a random port, at least not
without
> fallback logic if that port is already used.

Ok, now we will try ten different ports. In the unlikely event that none of them
work we'll simply fall back to the original upload.py logic.

[Sebastian Noack, 2016-03-11 14:57:37]

LGTM