hgweb: remove wsgirequest.form (API) Now that everything is ported to consume from parsedrequest.qsparams, we no longer have a need for wsgirequest.form. Let's remove all references to it. .. api:: The WSGI request object no longer exposes a ``form`` attribute containing parsed query string data. Use the ``qsparams`` attribute instead. Differential Revision: https://phab.mercurial-scm.org/D2782

hgweb: perform all parameter lookup via qsparams I think I managed to update all call sites using wsgirequest.form to use parsedrequest.qsparams. Since behavior of qsparams is to retrieve last value, behavior will change if a parameter was specified multiple times. But I think this is acceptable. I'm not a fan of the `req.req.qsparams` pattern. And some of the modified code could be written better. But I was aiming for a straight port with this change. Cleanup can come later. Differential Revision: https://phab.mercurial-scm.org/D2781

hgweb: set variables in qsparams We currently mutate wsgireq.form in a few places. Since it is independent from req.qsparams, we will need to make changes on req.qsparams as well before consumers can use qsparams. So let's do that. Eventually, we'll delete wsgireq.form and all references to it. Differential Revision: https://phab.mercurial-scm.org/D2780

hgweb: use our new request object for "style" parameter The "style" parameter is kind of wonky because it is explicitly set and has lookups in random locations. Let's port it to qsparams first because it isn't straightforward. There is subtle change in behavior. But I don't think it is worth calling out in a BC. Our multidict's __getitem__ returns the last set value for a key, not the first. So if the query string set a variable multiple times, before we would get the first value and now we would get the last value. It makes no sense to specify these things multiple times. And I think last write wins is more sensible than first write wins. Differential Revision: https://phab.mercurial-scm.org/D2779

hgweb: use a multidict for holding query string parameters My intention with refactoring the WSGI code was to make it easier to read. I initially wanted to vendor and use WebOb, because it seems to be a pretty reasonable abstraction layer for WSGI. However, it isn't using relative imports and I didn't want to deal with the hassle of patching it. But that doesn't mean we can't use good ideas from WebOb. WebOb has a "multidict" data structure for holding parsed query string and POST form data. It quacks like a dict but allows you to store multiple values for each key. It offers mechanisms to return just one value, all values, or return 1 value asserting that only 1 value is set. I quite like its API. This commit implements a read-only "multidict" in the spirit of WebOb's multidict. We replace the query string attributes of our parsed request with an instance of it. Differential Revision: https://phab.mercurial-scm.org/D2776

hgweb: create dedicated type for WSGI responses We have refactored the request side of WSGI processing into a dedicated type. Now let's do the same thing for the response side. We invent a ``wsgiresponse`` type. It takes an instance of a request (for consulation) and the WSGI application's "start_response" handler. The type basically allows setting the HTTP status line, response headers, and the response body. The WSGI application calls sendresponse() to start sending output. Output is emitted as a generator to be fed through the WSGI application. According to PEP 3333, this is the preferred way for output to be transmitted. (Our legacy ``wsgirequest`` exposed a write() to send data. We do not wish to support this API because it isn't recommended by PEP 3333.) The wire protocol code has been ported to use the new API. Differential Revision: https://phab.mercurial-scm.org/D2775

tests: add test for a wire protocol request to wrong base URL We have code that validates that wire protocol commands (which are specified via query string) must occur at the base URL of a repo. But we have no test coverage for this behavior. Let's add some. Differential Revision: https://phab.mercurial-scm.org/D2778

hgweb: remove support for short query string based aliases (BC) Form data exposed by hgweb is post-processed to expand certain shortcuts. For example, URLs with "?cs=@" is essentially expanded to "?cmd=changeset&node=@". And the URL router treats this the same as "/changeset/@". These shortcuts were initially added in 2005 in 34cb3957d875 and 964baa35faf8. They have rarely been touched in the last decade (just moving code around a bit). We have almost no test coverage of this feature. AFAICT no templates reference URLs of this form. I even looked at the initial version of paper and coal from ~2008 and they use the "/command/params" URL form and not these shortcuts. Furthermore, I couldn't even get some shortcuts to work! For example, "?sl=@" attempts to do a revision search instead of showing shortlog starting at revision @. Maybe I'm just doing it wrong? Because this is ancient, mostly untested code, there is a migration path to something better, and because anyone passionate enough to preserve URLs can install URL redirects, let's nuke the feature. .. bc:: Query string shorts in hgweb like ``?cs=@`` have been removed. Use URLs of the form ``/:cmd`` instead. Differential Revision: https://phab.mercurial-scm.org/D2773

hgweb: remove support for POST form data (BC) Previously, we called out to cgi.parse(), which for POST requests parsed multipart/form-data and application/x-www-form-urlencoded Content-Type requests for form data, combined it with query string parameters, returned a union of the values. As far as I know, nothing in Mercurial actually uses this mechanism to submit data to the HTTP server. The wire protocol has its own mechanism for passing parameters. And the web interface only does GET requests. Removing support for parsing POST data doesn't break any tests. Another reason to not like this feature is that cgi.parse() may modify the QUERY_STRING environment variable as a side-effect. In addition, it merges both POST data and the query string into one data structure. This prevents consumers from knowing whether a variable came from the query string or POST data. That can matter for some operations. I suspect we use cgi.parse() because back when this code was initially implemented, it was the function that was readily available. In other words, I don't think there was conscious choice to support POST data: we just got it because cgi.parse() supported it. Since nothing uses the feature and it is untested, let's remove support for parsing POST form data. We can add it back in easily enough if we need it in the future. .. bc:: Hgweb no longer reads form data in POST requests from multipart/form-data and application/x-www-form-urlencoded requests. Arguments should be specified as URL path components or in the query string in the URL instead. Differential Revision: https://phab.mercurial-scm.org/D2774

hgweb: expose input stream on parsed WSGI request object Our next step towards moving away from wsgirequest to our newer, friendlier parsedrequest type is input stream access. This commit exposes the input stream on the instance. Consumers in the HTTP protocol server switch to it. Because there were very few consumers of the input stream, we stopped storing a reference to the input stream on wsgirequest directly. All access now goes through parsedrequest. However, wsgirequest still may read from this stream as part of cgi.parse(). So we still need to create the stream from wsgirequest. Differential Revision: https://phab.mercurial-scm.org/D2771

hgweb: make parsedrequest part of wsgirequest This is kind of ugly. But an upcoming commit will teach parsedrequest about the input stream. Because the input stream is global state and can't be accessed without side-effects, we need to take actions to ensure that multiple consumers don't read from it independently. The easiest way to do this is for one object to hold a reference to both items having access to the input stream so that when a copy is made, we can remove the attribute from the other instance. So we create our parsed request instance from the wsgirequest constructor and hold a reference to it there. This is better than our new type holding a reference to wsgirequest because all the code for managing access will be temporary and we shouldn't pollute parsedrequest with this ugly history. Differential Revision: https://phab.mercurial-scm.org/D2770

hgweb: refactor the request draining code The previous code for draining was only invoked in a few places in the wire protocol. Behavior wasn't consist. Furthermore, it was difficult to reason about. With us converting the input stream to a capped reader, it is now safe to always drain the input stream when its size is known because we can never overrun the input and read into the next HTTP request. The only question is "should we?" This commit changes the draining code so every request is examined. Draining now kicks in for a few requests where it wouldn't before. But I think the code is sufficiently restricted so the behavior is safe. Possibly the most dangerous part of this code is the issuing of Connection: close for POST and PUT requests that don't have a Content-Length. I don't think there are any such uses in our WSGI application, so this should be safe. In the near future, I plan to significantly refactor the WSGI response handling. I anticipate this code evolving a bit. So any minor regressions around draining or connection closing behavior might be fixed as a result of that work. All tests pass with this change. That scares me a bit because it means we are lacking low-level tests for the HTTP protocol. Differential Revision: https://phab.mercurial-scm.org/D2769

hgweb: use a capped reader for WSGI input stream Per PEP 3333, the input stream from WSGI should respect EOF and prevent reads past the end of the request body. However, not all WSGI servers guarantee this. Notably, our BaseHTTPServer based built-in HTTP server doesn't. Instead, it exposes the raw socket and you can read() from it all you want, getting the connection in a bad state by doing so. We have a "cappedreader" utility class that proxies a file object and prevents reading past a limit. This commit converts the WSGI input stream into a capped reader when the input length is advertised via Content-Length headers. "cappedreader" only exposes a read() method. PEP 3333 states that the input stream MUST also support readline(), readlines(hint), and __iter__(). However, since our WSGI application code only calls read() and since we're not manipulating the stream exposed by the WSGI server, we're not violating the spec here. Differential Revision: https://phab.mercurial-scm.org/D2768

hgweb: document continuereader Differential Revision: https://phab.mercurial-scm.org/D2767

hgweb: remove wsgirequest.__iter__ This was added in d0db3462d568 in 2006. I can't find a justification for this method in PEP 3333. I suspect we were originally intending to use this type as the WSGI application (which should be iterable)? The tests all pass without this method. So let's nuke it. Differential Revision: https://phab.mercurial-scm.org/D2749

hgweb: remove wsgirequest.read() This was just a proxy to self.inp.read(). This method serves little value. Let's nuke it. Callers in the wire protocol server have been updated accordingly. Differential Revision: https://phab.mercurial-scm.org/D2748

hgweb: remove unused methods on wsgirequest writelines() isn't used in our code base. close() was a no-op. It is an optional method per PEP 3333. My eventual goal is to kill the wsgirequest class, hence why I'm removing code. Differential Revision: https://phab.mercurial-scm.org/D2747

wireprotoserver: remove unused argument from _handlehttperror() Differential Revision: https://phab.mercurial-scm.org/D2746

hgweb: store and use request method on parsed request PEP 3333 says that REQUEST_METHOD is always defined. Differential Revision: https://phab.mercurial-scm.org/D2745

hgweb: handle CONTENT_LENGTH PEP 3333 says CONTENT_LENGTH may be set. I /think/ WSGI servers are allowed to invent this key even if the client didn't send it. We had code in wireprotoserver looking for this key. So let's just automagically convert this key to an HTTP request header when parsing the request. Differential Revision: https://phab.mercurial-scm.org/D2744

wireprotoserver: access headers through parsed request Now that we can access headers via the parsed request object, let's do that. Since the new object uses bytes, hyphens, and is case-insensitive, a bit of code around normalizing values has been removed. I think the new code is much more intuitive because it more closely matches what is going out over the wire. Differential Revision: https://phab.mercurial-scm.org/D2743

debugwireproto: close the write end before consuming all available data And make it read all available data deterministically. Otherwise util.poll() may deadlock because both stdout and stderr could have no data. Spotted by the next patch which removes stderr from the fds.

graft: check for missing revision first before scanning working copy Differential Revision: https://phab.mercurial-scm.org/D2753

hook: ensure stderr is flushed when an exception is raised, for test stability Windows has had issues with output order in test-ssh-proto-unbundle.t[1] since it was created a few weeks ago. Each of the problems occurred when an exception was thrown out of the hook. Now the only thing blocking D2720 is the fact that the "abort: ..." lines on stderr are totally AWOL. I have no idea where there are. [1] https://buildbot.mercurial-scm.org/builders/Win7%20x86_64%20hg%20tests/builds/541/steps/run-tests.py%20%28python%202.7.13%29/logs/stdio