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

hgweb: garbage collect on every request There appears to be a cycle in localrepository or hgweb that is preventing repositories from being garbage collected when hgwebdir dispatches to hgweb. Every request creates a new repository instance and then leaks that object and other referenced objects. A periodic GC to find cycles will eventually collect the old repositories. But these don't run reliably and rapid requests to hgwebdir can result in rapidly increasing memory consumption. With the Firefox repository, repeated requests to raw-file URLs leak ~100 MB per hgwebdir request (most of this appears to be cached manifest data structures). WSGI processes quickly grow to >1 GB RSS. Breaking the cycles in localrepository is going to be a bit of work. Because we know that hgwebdir leaks localrepository instances, let's put a band aid on the problem in the form of an explicit gc.collect() on every hgwebdir request. As the inline comment states, ideally we'd do this in a finally block for the current request iff it dispatches to hgweb. But _runwsgi() returns an explicit value. We need the finally to run after generator exhaustion. So we'd need to refactor _runwsgi() to "yield" instead of "return." That's too much change for a patch to stable. So we implement this hack one function above and run it on every request. The performance impact of this change should be minimal. Any impact should be offset by benefits from not having hgwebdir processes leak memory.

amend: abort if unresolved merge conflicts found (issue5805) It was checked by repo.commit() before e8a7c1a0565a "cmdutil: remove the redundant commit during amend."

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

wireproto: raise ProgrammingError instead of Abort This isn't a user-facing error and can only be caused by bad Python code. Thanks to Yuya for spotting this. Differential Revision: https://phab.mercurial-scm.org/D2777

py3: make test-commit-interactive.t byte-safe

py3: open patch file in binary mode and convert eol manually Here we don't introduce a reader wrapper since it wouldn't be easy to make read(n) handle partial data and length correctly.

py3: wrap file object to write patch in native eol preserving byte-ness

py3: drop b'' from debug message "moving bookmarks"

py3: use r'' instead of sysstr('') to get around code transformer Fewer function calls should be better.

ui: remove any combinations of CR|LF from prompt response On Windows, we have to accept both CR+LF and LF. This patch simply makes any trailing CRs and LFs removed from a user input instead of doing stricter parsing, as an input must be a readable text.

sshpeer: check pipe validity before forwarding output from it After the previous fix, fileobjectproxy._observedcall() (called when win32.peekpipe() accesses .fileno) started exploding. With this fix, similar checks are needed inside debugwireproto(). Since that is hardcoded to not use os.devnull, IDK if those are worth fixing.

util: forward __bool__()/__nonzero__() on fileobjectproxy In trying to debug the Windows process hang in D2720, I changed the stderr pipe to the peer to be os.devnull instead. That caused sshpeer._cleanuppipes()[1] to explode, complaining NoneType has no __iter__ attribute, even though the previous line checked for None. [1] https://www.mercurial-scm.org/repo/hg/file/b434965f984e/mercurial/sshpeer.py#l133

py3: fix slicing of bisect label in templatefilters.shortbisect()

templatefilters: inline hbisect.shortlabel() It's pretty simple. I don't think the business logic has to be placed in hbisect.py.

py3: make test-bisect.t bytes-safe

py3: fix integer formatting in bisect error

py3: silence f.write() in test-annotate.t

xdiff: resolve signed unsigned comparison warning Since the value won't be changed inside the code (because context lines feature was removed by D2705), let's just remove the variable and inline the 0 value. The code might be potentially further simplified. But I'd like to make sure correctness is easily verifiable in this patch. Differential Revision: https://phab.mercurial-scm.org/D2766

xdiff: use int64 for hash table size Follow-up of the previous "long" -> "int64" change. Now xdiff only uses int for return values and small integers (ex. booleans, shifting score, bits in hash table size, etc) so it should be able to handle large input. Differential Revision: https://phab.mercurial-scm.org/D2765

xdiff: remove unused xpp and xecfg parameters They are unused. Thus removed. Differential Revision: https://phab.mercurial-scm.org/D2764

xdiff: remove unused flags parameter After D2683, the flags parameter in some functions is no longer needed. Thus removed. Differential Revision: https://phab.mercurial-scm.org/D2763

xdiff: replace {unsigned ,}long with {u,}int64_t MSVC treats "long" as 4-byte. That could cause overflows since the xdiff code uses "long" in places where "size_t" or "ssize_t" should be used. Let's use explicit 8 byte integers to avoid FWIW git avoids that overflow by limiting diff size to 1GB [1]. After examining the code, I think the remaining risk (the use of "int") is low since "int" is only used for return values and hash table size. Although a wrong hash table size would not affect the correctness of the code, but that could make the code extremely slow. The next patch will change hash table size to 8-byte integer so the 1GB limit is unlikely needed. This patch was done by using `sed`. [1]: https://github.com/git/git/commit/dcd1742e56ebb944c4ff62346da4548e1e3be67 Differential Revision: https://phab.mercurial-scm.org/D2762

xdiff: add comments for fields in xdfile_t This makes the related code easier to understand. Differential Revision: https://phab.mercurial-scm.org/D2685

xdiff: add a preprocessing step that trims files xdiff has a `xdl_trim_ends` step that removes common lines, unmatchable lines. That is in theory good, but happens too late - after splitting, hashing, and adjusting the hash values so they are unique. Those splitting, hashing and adjusting hash values steps could have noticeable overhead. Diffing two large files with minor (one-line-ish) changes are not uncommon. In that case, the raw performance of those preparation steps seriously matter. Even allocating an O(N) array and storing line offsets to it is expensive. Therefore my previous attempts [1] [2] cannot be good enough since they do not remove the O(N) array assignment. This patch adds a preprocessing step - `xdl_trim_files` that runs before other preprocessing steps. It counts common prefix and suffix and lines in them (needed for displaying line number), without doing anything else. Testing with a crafted large (169MB) file, with minor change: ``` open('a','w').write(''.join('%s\n' % (i % 100000) for i in xrange(30000000) if i != 6000000)) open('b','w').write(''.join('%s\n' % (i % 100000) for i in xrange(30000000) if i != 6003000)) ``` Running xdiff by a simple binary [3], this patch improves the xdiff perf by more than 10x for the above case: ``` # xdiff before this patch 2.41s user 1.13s system 98% cpu 3.592 total # xdiff after this patch 0.14s user 0.16s system 98% cpu 0.309 total # gnu diffutils 0.12s user 0.15s system 98% cpu 0.272 total # (best of 20 runs) ``` It's still slightly slower than GNU diffutils. But it's pretty close now. Testing with real repo data: For the whole repo, this patch makes xdiff 25% faster: ``` # hg perfbdiff --count 100 --alldata -c d334afc585e2 --blocks [--xdiff] # xdiff, after ! wall 0.058861 comb 0.050000 user 0.050000 sys 0.000000 (best of 100) # xdiff, before ! wall 0.077816 comb 0.080000 user 0.080000 sys 0.000000 (best of 91) # bdiff ! wall 0.117473 comb 0.120000 user 0.120000 sys 0.000000 (best of 67) ``` For files that are long (ex. commands.py), the speedup is more than 3x, very significant: ``` # hg perfbdiff --count 3000 --blocks commands.py.i 1 [--xdiff] # xdiff, after ! wall 0.690583 comb 0.690000 user 0.690000 sys 0.000000 (best of 12) # xdiff, before ! wall 2.240361 comb 2.210000 user 2.210000 sys 0.000000 (best of 4) # bdiff ! wall 2.469852 comb 2.440000 user 2.440000 sys 0.000000 (best of 4) ``` [1]: https://phab.mercurial-scm.org/D2631 [2]: https://phab.mercurial-scm.org/D2634 [3]: ``` // Code to run xdiff from command line. No proper error handling. #include <stdlib.h> #include <unistd.h> #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> #include "mercurial/thirdparty/xdiff/xdiff.h" #define ensure(x) if (!(x)) exit(255); mmfile_t readfile(const char *path) { struct stat st; int fd = open(path, O_RDONLY); fstat(fd, &st); mmfile_t file = { malloc(st.st_size), st.st_size }; ensure(read(fd, file.ptr, st.st_size) == st.st_size); close(fd); return file; } int main(int argc, char const *argv[]) { mmfile_t a = readfile(argv[1]), b = readfile(argv[2]); xpparam_t xpp = {0}; xdemitconf_t xecfg = {0}; xdemitcb_t ecb = {0}; xdl_diff(&a, &b, &xpp, &xecfg, &ecb); return 0; } ``` Differential Revision: https://phab.mercurial-scm.org/D2686

transaction: add a name and a __repr__ implementation (API) This has been useful for me for debugging. Differential Revision: https://phab.mercurial-scm.org/D2758

phabricator: update doc string for deprecated token argument Differential Revision: https://phab.mercurial-scm.org/D2755

phabricator: print deprecation warning only once Differential Revision: https://phab.mercurial-scm.org/D2754

tests: add a few tests involving --collapse and rebase.singletransaction=1 I'm about to change the rebase code quite a bit and this was poorly tested. Differential Revision: https://phab.mercurial-scm.org/D2757

tests: simplify test-rebase-transaction.t The file was extracted from test-rebase-base.t in 8cef8f7d51d0 (test-rebase-base: clarify it is about the "--base" flag, 2017-10-05). This patch follows up that and clarifies the new file's purpose and simplifies it a bit. Differential Revision: https://phab.mercurial-scm.org/D2756

hgweb: parse and store HTTP request headers WSGI transmits HTTP request headers as HTTP_* environment variables. We teach our parser about these and hook up a dict-like data structure that supports case insensitive header manipulation. Differential Revision: https://phab.mercurial-scm.org/D2742

wireprotoserver: remove broken optimization for non-httplib client There was an experimental non-httplib client in core for several years. It was removed a week or so ago. We kept the optimization for this client in the server code. I'm not sure if that was intended or not. But it doesn't matter: the code was wrong. Because the code was accessing a WSGI environment dict, it needed to access the HTTP_X_HGHTTP2 key to actually read the HTTP header. So the code deleted by this commit wasn't actually doing anything meaningful. Doh. Differential Revision: https://phab.mercurial-scm.org/D2741

wireprotoserver: move all wire protocol handling logic out of hgweb Previous patches from several days ago worked to isolate processing of HTTP wire protocol requests to wireprotoserver. We still had a little logic in hgweb. If feels like the right time to finish the job. This commit moves WSGI request servicing from hgweb to wireprotoserver. The ugly dict holding the parsed request is no more. I think the new code is cleaner. As part of this, we now process wire protocol requests before the block to obtain the "query" variable. This makes it clear that this wonky "query" variable is not used by the wire protocol. The wonkiest part about this code is the HTTP 404. I'm actually not sure what all is going on here. It looks like the code is trying to prevent URL with path components that specify a command from not working. That part I grok. What I don't grok is why we need to send a 404. I would think it would be OK to no-op and let another handler try to service the request. But if we do this, we get some subrepo test failures. So it looks like something is expecting the HTTP 404 and reacting to it in a specific way. It /might/ be possible to change the behavior here. But it isn't something I'm comfortable doing because I don't understand the problem space. Differential Revision: https://phab.mercurial-scm.org/D2740