( )⚙ D231 httppeer: add support for httppostargs when we're sending a file

This is an archive of the discontinued Mercurial Phabricator instance.

httppeer: add support for httppostargs when we're sending a file
ClosedPublic

Authored by durin42 on Aug 4 2017, 5:05 PM.

Details

Summary

This is probably only used in the 'unbundle' command, but the code
ended up being cleaner to make it generic and treat *all* httppostargs
with a non-args request body as though they were file-like in
nature. It also means we get test coverage more or less for free. A
previous version of this change didn't use io.BytesIO, and it was a
lot more complicated.

This also fixes a server-side bug, so anyone using httppostargs should
update all of their servers to this revision or later *before* this
gets to their clients, otherwise servers will hang trying to over-read
the POST body.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durin42 created this revision.Aug 4 2017, 5:05 PM
martinvonz added inline comments.
mercurial/httppeer.py
95

I'm guessing 'fileprepender' here and a few placed below is the old name for '_multifile' (so please update)

96

It doesn't matter much since it's just a programming error if it happens, but how will these arguments to ValueError be rendered?

105

i read that negative amount means the same thing (read all). do we care to follow that contract here?

111

nit: i think this can be "if got <= amt" to avoid an unnecessary 0-length read the next time _multifile.read() (and it does look like that will be the normal case, that the reader will read exactly the size of the first "file" first)

125

also need to set self._index=0, no?

213–214

Nit: to reduce indentation, it looks like you can change the condition above to "if postargsok and args" instead (because bool(strargs) == bool(args) AFAICT)

durin42 marked 3 inline comments as done.Aug 8 2017, 10:41 AM
durin42 updated this revision to Diff 627.
durin42 added inline comments.Aug 8 2017, 10:41 AM
mercurial/httppeer.py
96

ValueError: ('_multifile only supports file objects that have a length but this one does not:', <type 'file'>, <open file 'README', mode 'r' at 0x110ff0b70>)

It ain't pretty, but it's got all the information we need.

111

No, because consider these file buffers:

'foo', 'bar'

if we read(2), we'll pull 2 bytes off the first buffer, which is not yet exhausted, but got == amt.

durin42 updated this revision to Diff 745.Aug 10 2017, 4:04 PM
martinvonz accepted this revision.Aug 15 2017, 4:31 PM
This revision is now accepted and ready to land.Aug 15 2017, 4:31 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Aug 21 2017, 9:21 AM
yuja added inline comments.
mercurial/httppeer.py
105

Just nits:

  • read(0) should return an empty string.
  • None <= 0 is TypeError on Python 3.
223

Does this mean data must have length attribute if it isn't
a string? A plain file object has no such attribute.