Previously the worker error handling is like:
pid = os.fork() --+
if pid == 0: |
.... | problematic
.... --+
try: --+
.... | worker error handling
--+
If a signal arrives when Python is executing the "problematic" lines, an
external error handling (dispatch.py) will take over the control flow and
it's no longer guaranteed "os._exit" is called (see 560cc0db7f01 for why it
is necessary).
This patch rewrites the error handling so it covers all possible code paths
for a worker even during fork.
Note: "os.getpid() == parentpid" is used to test if the process is parent or
not intentionally, instead of checking "pid", because "pid = os.fork()" may
be not atomic - it's possible that that a signal hits the worker before the
assignment completes [1]. The newly added test replaces "os.fork" to
exercise that extreme case.
[1]: CPython compiles "pid = os.fork()" to 2 byte codes: "CALL_FUNCTION" and
"STORE_FAST", so it's probably not atomic:
def f():
pid = os.fork()
dis.dis(f)
2 0 LOAD_GLOBAL 0 (os)
3 LOAD_ATTR 1 (fork)
6 CALL_FUNCTION 0
9 STORE_FAST 0 (pid)
12 LOAD_CONST 0 (None)
15 RETURN_VALUE
dispatch handles KeyboardInterrupt already. This makes the code more
consistent, and makes worker not print "killed!" if it receives SIGTERM in
most cases (in rare cases there is still "killed!" printed, which will be
fixed by the next patch).
If a marker has no parent information, parents field is set to None, which
caused TypeError. I think this shouldn't normally happen, but somehow
I have such marker in my repo.
We point out at the help of the config option for user who wants to learn more
about the possible config value.
The suggested command returns some other extra (color related) results, but this
is bug to fix outside of the freeze.
The --color option is described to accept "boolean, always, auto,
never, or debug". Let's use a similar description for ui.color. Also
fix the formatting to use double quotes as we seem to use for about
half the values in config.txt (the other half uses double
backticks). Also use uppercase Boolean for consistency within the
file.
Acquiring lock by vfs.makelock() and getting lock holder (aka
"locker") information by vfs.readlock() aren't atomic action.
Therefore, failure of the former doesn't ensure success of the latter.
Before this patch, lock is unintentionally acquired, if ENOENT
causes failure of vfs.readlock() while 5 times retrying, because
lock._trylock() returns to caller silently after retrying, and
lock.lock() assumes that lock._trylock() returns successfully only if
lock is acquired.
In this case, lock symlink (or file) isn't created, even though lock
is treated as acquired in memory.
To avoid this issue, this patch makes lock._trylock() raise
LockHeld(EAGAIN) at the end of it, if lock isn't acquired while
retrying.
An empty "locker" meaning "busy for frequent lock/unlock by many
processes" might appear in an abortion message, if lock acquisition
fails. Therefore, this patch also does:
- use '%r' to increase visibility of "locker", even if it is empty
- show hint message to explain what empty "locker" means
Acquiring lock by vfs.makelock() and getting lock holder (aka
"locker") information by vfs.readlock() aren't atomic action.
Therefore, failure of the former doesn't ensure success of the latter.
Before this patch, lock is unintentionally acquired, if
self.parentlock is None (this is default value), and lock._readlock()
returns None for ENOENT at vfs.readlock(), because these None are
recognized as equal to each other.
In this case, lock symlink (or file) isn't created, even though lock
is treated as acquired in memory.
To avoid this issue, this patch retries lock acquisition immediately,
if lock._readlock() returns None "locker".
This issue will be covered by a test added in subsequent patch,
because simple emulation of ENOENT at vfs.readlock() easily causes
another issue. "raising ENOENT only at the first vfs.readlock()
invocation" is too complicated for unit test, IMHO.
Another raising PeerTransportError for "incomplete response" in
httppeer.py uses this (changed) hint message. This unification reduces
cost of translation.
There are some paragraphs, which aren't rendered in online help as
expected because of indentation and literal blocking issues.
- hgext/rebase.py
- paragraph before example code ends with ":", which treats
subsequent indented paragraphs as normal block
=> replace ":" with "::" to treat subsequent paragraphs as literal block
- help/pager.txt
- paragraph before a list of --pager option values ends with "::",
which treats subsequent indented paragraphs as literal block
=> replace "::" with ":" to treat subsequent paragraphs as normal block
- the second line of explanation for no/off --pager option value is
indented incorrectly (this also causes failure of "make" in doc)
=> indent correctly
- help/revisions.txt
- explanation following example code of "[revsetalias]" section
isn't suitable for literal block
=> un-indent explanation paragraph to treat it as normal block
- indentation of "For example" before example of tag() revset
predicate matching is meaningless
- descriptive text for tag() revset predicate matching isn't
suitable for literal block
=> un-indent concatenated two paragraphs to treat them as normal block
When on a filelog head, we are certain that there will be no descendant so the
target of the "descending" link will lead to an empty log result. Do not
display the link in this case.
We set parent._descendantrev = child.rev() when walking parents in
blockancestors() so that, when linkrev adjustment is perform for these, it
starts from a close descendant instead of possibly topmost introrev. (See
`self._adjustlinkrev(self._descendantrev)` in filectx._changeid().)
This is similar to changeset 8758896efb1c, which added a "f._changeid"
instruction in annotate() for the same purpose.
However, here, we set _descendantrev explicitly instead of relying on the
'_changeid' cached property being accessed (with effect to set _changeid
attribute) so that, in _parentfilectx() (called from parents()), we go through
`if '_changeid' in vars(self) [...]` branch in which instruction
`fctx._descendantrev = self.rev()` finally appears and does what we want.
With this, we can roughly get a 3x speedup (including in example of issue5538
from mozilla-central repository) on usage of followlines revset (and
equivalent hgweb request).
Update the hunk selector help message to use the operation name instead
of using "record" for all operations. Extract the help message in the same way
as other single and multiple message line.
Update tests to make sure that both "revert" and "discard" variants are tested.
Previously, calling blockancestors() with a fctx not touching file would
sometimes yield this filectx first, instead of the first "block ancestor",
because when compared to its parent it may have changes in specified line
range despite not touching the file at all.
Fixing this by starting the algorithm from the "base" filectx obtained using
fctx.introrev() (as done in annotate()).
In tests, add a changeset not touching file we want to follow lines of to
cover this case. Do this in test-annotate.t for followlines revset tests and
in test-hgweb-filelog.t for /log/<rev>/<file>?linerange=<from>:<to> tests.
Resolves test failures on FreeBSD, but I'm not happy about the fix.
A previous version of this also wrapped readline by putting the hack
in the _call method on doublepipe. That was confusing for readers and
wasn't necessary - just doing this on read() is sufficient to fix the
bugs I'm observing. We can always come back and do readline later if
needed.
`hg show work` is much more usable if output is colored. This patch
implements coloring via label() in a very hacky way.
In a default Mercurial install, you'll see yellow node labels for all
phases. Branches and bookmarks use the same formatting as the commit
message. So this change doesn't help much in a default install. But if
you have a custom colors defined for these things, output is much more
readable.
The implementation obviously needs some work. But for a minor change
on a feature that isn't convered by BC, this seems like a clear win
for the feature in 4.2.
Durham and I both like this better than "underway." We can add aliases
and bikeshed on the name during the 4.3 cycle, as this whole extension is
highly experimental.
See the inline comment how this could mitigate the issue.
I couldn't reproduce the exact problem on my Linux machine, but there are
at least two people who got EINTR in progress.py, and it seems file_write()
of Python 2 is fundamentally broken [1]. Let's make something in on 4.2.
[1]: https://hg.python.org/cpython/file/v2.7.13/Objects/fileobject.c#l1850
Some shared-ssh installations assume that 'hg serve --stdio' is a safe
command to run for minimally trusted users. Unfortunately, the messy
implementation of argument parsing here meant that trying to access a
repo named '--debugger' would give the user a pdb prompt, thereby
sidestepping any hoped-for sandboxing. Serving repositories over HTTP(S)
is unaffected.
We're not currently hardening any subcommands other than 'serve'. If
your service exposes other commands to users with arbitrary repository
names, it is imperative that you defend against repository names of
'--debugger' and anything starting with '--config'.
The read-only mode of hg-ssh stopped working because it provided its hook
configuration to "hg serve --stdio" via --config parameter. This is banned for
security reasons now. This patch switches it to directly call ui.setconfig().
If your custom hosting infrastructure relies on passing --config to
"hg serve --stdio", you'll need to find a different way to get that configuration
into Mercurial, either by using ui.setconfig() as hg-ssh does in this patch,
or by placing an hgrc file someplace where Mercurial will read it.
mitrandir@fb.com provided some extra fixes for the dispatch code and
for hg-ssh in places that I overlooked.
If we use the normal vfs, store encoding will be applied when we
.join() the path to be copied. This results in attempting to copy
a file that (likely) doesn't exist. Using the rawvfs operates on
the raw file path, which is returned by vfs.readdir().
Users at Mozilla are encountering this, as I've instructed them to
run `hg debugupgraderepo` to upgrade to generaldelta. While Mercurial
shouldn't deposit any files under .hg/store that require encoding, it
is possible for e.g. .DS_Store files to be created by the operating
system.
Previously, the "oldheads" variable was a list. On a repository at
Mozilla with 46,492 heads, profiling revealed that list membership
testing was dominating execution time of applying small changegroups.
This patch converts the list of old heads to a set. This makes
membership testing significantly faster. On the aforementioned
repository with 46,492 heads:
$ hg unbundle <file with 1 changeset>
before: 18.535s wall
after: 1.303s
Consumers of this variable only check for truthiness (`if oldheads`),
length (`len(oldheads)`), and (most importantly) item membership
(`h not in oldheads` - which occurs twice). So, the change to a set
should be safe and suitable for stable.
The practical effect of this change is that changegroup application
and related operations (like `hg push`) no longer exhibit an O(n^2)
CPU explosion as the number of heads grows.
Previously, `hg bundle zstd` on a non-generaldelta repo would
attempt to use a v1 bundle. This would fail because zstd is not
supported on v1 bundles.
This patch changes the behavior to automatically use a v2 bundle
when the user explicitly requests a bundlespec that is a compression
engine not supported on v1. If the bundlespec is <engine>-v1, it is
still explicitly rejected because that request cannot be fulfilled.
Version 1 bundles only support a fixed set of compression engines.
Before this change, we would accept any compression engine for v1
bundles, even those that may not work on v1. This could lead to
an error.
We define a fixed set of compression engines known to work with v1
bundles and we add checking to ensure a newer engine (like zstd)
won't work with v1 bundles.
I also took the liberty of adding test coverage for unknown compression
names because I noticed we didn't have coverage of it before.
For consistency with the other template options. Quotes are necessary if
you want to preserve leading/trailing whitespace, which would be stripped
by config parser.
I got the following error by running "hg log" and quitting the pager
immediately. Any output here may trigger another SIGPIPE, so only thing
we can do is to swallow the exception and exit with an error status.
Traceback (most recent call last):
File "./hg", line 45, in <module>
mercurial.dispatch.run()
File "mercurial/dispatch.py", line 83, in run
status = (dispatch(req) or 0) & 255
File "mercurial/dispatch.py", line 167, in dispatch
req.ui.warn(_("interrupted!\n"))
File "mercurial/ui.py", line 1224, in warn
self.write_err(*msg, **opts)
File "mercurial/ui.py", line 790, in write_err
self._write_err(*msgs, **opts)
File "mercurial/ui.py", line 798, in _write_err
self.ferr.write(a)
File "mercurial/ui.py", line 129, in _catchterm
raise error.SignalInterrupt
mercurial.error.SignalInterrupt
Perhaps this wasn't visible before ee4f321cd621 because the original stderr
handle was restored very late.
Before, it crashed because mapping['templ'] was missing. As it didn't support
the legacy list template from the beginning, we can simply use hybridlist().
See the previous commit for why.
splitlines() does not pass a mapping dict, which would probably mean the
legacy template didn't work from the beginning.
Now that all known format variants exists outside of the function, we can gather
them in a lists. This build a single entry point other code can use (current
target: extensions).
The repository upgrade code is updated to simply use entries from this list.
As a side effect this will also allow extensions to register their own format
variants, to do this "properly" we should introduce a "registrar" for this
category of object. However I prefer to keep this series simple, and that will
be adventure for future time.
Our goal here is to get top level definition for all the format variants. Having
them defined outside of the function enabled other users of that logic.
They are two keys components of a format variant:
1) the name and various descriptions of its effect,
2) the code that checks if the repo is using this variant and if the config
enables it.
That second items make us pick a class-based approach, since different variants
requires different code (even if in practice, many can reuse the same logic).
Each variants define its own class that is then used like a singleton. The
class-based approach also clarify the definitions part a bit since each are
simple assignment in an indented block.
The 'fromdefault' and 'fromconfig' are respectively replaced by a class
attribute and a method to be called at the one place where "fromconfig"
matters.
Overall, they are many viable approach for this, but this is the one I picked.
The 'deficiency' type has multiple specificities. We create a dedicated class to
host them. More logic will be added incrementally in future changesets.
I've not found anything related to color + windows on the bug tracker. So I'm
suggesting we get bolder and turn it on for windows too in the release
candidate. We can always backout this changeset if we find serious issue on
windows.
Color support is all in core for a couple of months. I've browsed the bug tracker
without finding any blocker bug. So I'm moving forward and enable color on by
default before '4.2-rc'. In the worse case, having it on in the release
candidate will help us to find blocker bug and we can turn it off for the final
release.
I remember people talking about issue with Windows during the freeze so I'm
keeping it off by default on that OS.
We could do various cleaning of the color used and the label issued. However
the label are probably already in our backward compatibility envelope since the
color extensions has been around since for ever and I do not think the color
choice themself should be considered BC. So I think we should rather gives color
to all user sooner than later.
A couple of test needs to be updated to avoid having color related control code
spoil the tested output.
Many have seen a "stream ended unexpectedly" error. This message is
raised from changegroup.readexactly() when a read(n) operation fails
to return exactly N bytes.
I believe most occurrences of this error in the wild stem from
the code changed in this patch. Before, if bundle2's part applicator
raised an Exception when processing/applying parts, the exception
handler would attempt to iterate the remaining parts. If I/O
during this iteration failed, it would likely raise the
"stream ended unexpectedly" exception.
The problem with this approach is that if we already encountered
an I/O error iterating the bundle2 data during application, then
any further I/O would almost certainly fail. If the original stream
were closed, changegroup.readexactly() would obtain an empty
string, triggering "stream ended unexpectedly" with "got 0." This is
the error message that users would see. What's worse is that the
original I/O related exception would be lost since a new exception
would be raised. This made debugging the actual I/O failure
effectively impossible.
This patch changes the exception handler for bundle2 application to
ignore errors when seeking the underlying stream. When the underlying
error is I/O related, the seek should fail fast and the original
exception will be re-raised. The output changes in
test-http-bad-server.t demonstrate this.
When the underlying error is not I/O related and the stream can be
seeked, the old behavior is preserved.
As part of writing test-http-bad-server.t, I noticed that some
requests include an empty Vary HTTP request header.
The Vary HTTP request header indicates which headers should be taken
into account when determining if a cached response can be used. It also
accepts the special value of "*".
The previous code unconditionally added a Vary header. This could lead
to an empty header value. While I don't believe this violates the HTTP
spec, this is weird and just wastes bytes. So this patch changes
behavior to only send a Vary header when it has a value.
Some low-level wire protocol byte reporting tests changed. In some
cases, the exact point of data termination changed. However, the
behavior being tested - that clients react when the connection is
closed in the middle of an HTTP request line or header - remains
unchanged.
The previous logic had many shortcoming (eg: looking at the head only, not
handling prune, etc...), the new logic use a more robust approach:
For each head, we check if after the push all changesets exclusive to this heads
will be obsolete. If they are, the branch considered be "replaced".
To check if a changeset will be obsolete, we simply checks:
* the changeset phase
* the existence of a marker relevant to the "pushed set" that affects the
changesets..
This fixes two major issues of the previous algorithm:
* branch partially rewritten (eg: head but not root) are no longer detected as
replaced,
* Prune are now properly handled.
(This implementation was introduction in the evolve extension, version 6.0.0.)
This new algorithm has an extended number of tests, a basic one is provided
in this patch. The others will be introduced in their own changeset for clarity.
In addition, we stop trying to process heads unknown locally, we do not have
enough data to take an informed decision so we should stop pretending we do.
This reflect a test that is now update.
I've been using `hg serve --web-conf ...` with a simple '/=projects/**' [paths]
configuration for awhile without issue. Let's ditch the need for the manual
configuration in this case, and limit the repos served to the actual subrepos.
This doesn't attempt to handle the case where a new subrepo appears while the
server is running. That could probably be handled with a hook if somebody wants
it. But it's such a rare case, it probably doesn't matter for the temporary
serves.
The main repo is served at '/', just like a repository without subrepos. I'm
not sure why the duplicate 'adding ...' lines appear on Linux. They don't
appear on Windows (see 3f4ff1bdf101), so they are optional.
Subrepositories that are configured with '../path' or absolute paths are not
cloneable from the server. (They aren't cloneable locally either, unless they
also exist at their configured source, perhaps via the share extension.) They
are still served, so that they can be browsed, or cloned individually. If we
care about that cloning someday, we can probably just add the extra entries to
the webconf dictionary. Even if the entries use '../' to escape the root, only
the related subrepositories would end up in the dictionary.
Now that the response instance itself is wrapped with error
handling, we no longer need this code. This code became dead
with the previous patch because the added code catches
HTTPException and re-raises as something else.
There were a handful of places in the code where HTTPResponse.read()
was called with no explicit error handling or with inconsistent
error handling. In order to eliminate this class of bug, we globally
swap out HTTPResponse.read() with a unified error handler.
I initially attempted to fix all call sites. However, after
going down that rabbit hole, I figured it was best to just change
read() to do what we want. This appears to be a worthwhile
change, as the tests demonstrate many of our uncaught exceptions
go away.
To better represent this class of failure, we introduce a new
error type. The main benefit over IOError is it can hold a hint.
I'm receptive to tweaking its name or inheritance.
An upcoming patch will add low-level testing of the bytes being sent
over the wire. As part of developing that test, I discovered that the
order of headers in HTTP requests wasn't deterministic. This patch
makes the order deterministic to make things easier to test.
We parse "descend" symbol as a Boolean using getboolean (prior extraction by
getargsdict already checked that it is a symbol).
In tests, check for error cases and vary Boolean values here and there.
The tag changes information we compute is now written to disk. This gives
hooks full access to that data.
The format picked for that file uses a 2 characters prefix for the action:
-R: tag removed
+A: tag added
-M: tag moved (old value)
+M: tag moved (new value)
This format allows hooks to easily select the line that matters to them without
having to post process the file too much. Here is a couple of examples:
* to select all newly tagged changeset, match "^+",
* to detect tag move, match "^.M",
* to detect tag deletion, match "-R".
Once again we rely on the fact the tag tests run through all possible
situations to test this change.
We now compute the proper actuall differences between tags before and after the
transaction. This catch a couple of false positives in the tests.
The compute the full difference since we are about to make this data available
to hooks in the next changeset.
This changeset introduces detection of tags changes during transaction. When
this happens a 'tag_moved=1' argument is set for hooks, similar to what we do
for bookmarks and phases.
This code is disabled by default as there are still various performance
concerns. Some require a smarter use of our existing tag caches and some other
require rework around the transaction logic to skip execution when unneeded.
These performance improvements have been delayed, I would like to be able to
experiment and stabilize the feature behavior first.
Later changesets will push the concept further and provide a way for hooks to
know what are the actual changes introduced by the transaction. Similar work
is needed for the other families of changes (bookmark, phase, obsolescence,
etc). Upgrade of the transaction logic will likely be performed at the same
time.
The current code can report some false positive when .hgtags file changes but
resulting tags are unchanged. This will be fixed in the next changeset.
For testing, we simply globally enable a hook in the tag test as all the
possible tag update cases should exist there. A couple of them show the false
positive mentioned above.
See in code documentation for more details.
This will get used to compare tags between two set of revisions during a
transaction (pre and post heads). The end goal is to be able to track tags
movement in transaction hooks.
If initial 'fctx' has changes in line range with respect to its parents, we
yield it first. This makes 'followlines(..., descend=True)' consistent with
'descendants()' revset which yields the starting revision.
We reuse one iteration of blockancestors() which does exactly what we want.
In test-annotate.t, adjust 'startrev' in one case to cover the situation where
the starting revision does not touch specified line range.
If this assertion fails, this indicates a flaw in the algorithm. So fail fast
instead of possibly producing wrong results.
Also extend the target line range in test to catch a merge changeset with all
its parents.
Module 'appdirs' tries to import win32com.shell (and catch ImportError as an
indication of failure) to check whether some further functionality should
be implemented one or another way [1]. Of course, demandimport lets it down, so
if we want appdirs to work we have to add it to demandimport's ignore list.
The reason we want appdirs to work is becuase it is used by setuptools [2] to
determine egg cache location. Only fairly recent versions of setuptools depend
on this so people don't see this often.
[1] https://github.com/ActiveState/appdirs/blob/master/appdirs.py#L560
[2] aae0a92811/pkg_resources/__init__.py (L1369)
The prior code used to ignore all errors, which was intended to
deal with a decade-old problem with writing to broken pipes on
Windows.
However, that code inadvertantly went a lot further, making it
impossible to detect *all* I/O errors on stdio ... but only sometimes.
What actually happened was that if Mercurial wrote less than a stdio
buffer's worth of output (the overwhelmingly common case for most
commands), any error that occurred would get swallowed here. But
if the buffering strategy changed, an unhandled IOError could be
raised from any number of other locations.
Because we now have a top-level StdioError handler, and ui._write
and ui._write_err (and now flush!) will raise that exception, we
have one rational place to detect and handle these errors.
We attempt to report what went wrong, and more importantly exit the
program with an error code.
(The exception we catch is not yet raised anywhere in the code.)
Mercurial currently fails to notice failures to write to stdout or
stderr. A correctly functioning command line tool should detect
this and exit with an error code.
To achieve this, we need a little extra plumbing, which we start
adding here.
In spite of its longstanding use, Python's built-in atexit code is
not suitable for Mercurial's purposes, for several reasons:
* Handlers run after application code has finished.
* Because of this, the code that runs handlers swallows exceptions
(since there's no possible stacktrace to associate errors with).
If we're lucky, we'll get something spat out to stderr (if stderr
still works), which of course isn't any use in a big deployment
where it's important that exceptions get logged and aggregated.
* Mercurial's current atexit handlers make unfortunate assumptions
about process state (specifically stdio) that, coupled with the
above problems, make it impossible to deal with certain categories
of error (try "hg status > /dev/full" on a Linux box).
* In Python 3, the atexit implementation is completely hidden, so
we can't hijack the platform's atexit code to run handlers at a
time of our choosing.
As a result, here's a perfectly cromulent atexit-like implementation
over which we have control. This lets us decide exactly when the
handlers run (after each request has completed), and control what
the process state is when that occurs (and afterwards).
In the initial implementation of blockdescendants (and thus followlines(...,
descend=True) revset), only the first branch encountered in descending
direction was followed.
Update the algorithm so that all children of a revision ('x' in code) are
considered. Accordingly, we need to prevent a child revision to be yielded
multiple times when it gets visited through different path, so we skip 'i'
when this occurs. Finally, since we now consider all parents of a possible
child touching a given line range, we take care of yielding the child if it
has a diff in specified line range with at least one of its parent (same logic
as blockancestors()).
Our current deprecation warning mechanism relies on ui object. They are case
where we cannot have access to the UI object. On a general basis we avoid using
the python mechanism for deprecation warning because up to Python 2.6 it is
exposing warning to unsuspecting user who cannot do anything to deal with them.
So we build a "safe" strategy to hide this warnings behind a flag in an
environment variable. The test runner set this flag so that tests show these
warning. This will help us marker API as deprecated for extensions to update
their code.
Mostly copy CSS rules from style-paper.css into style-gitweb.css. The only
modification is addition of !important on "background-color" rule for
"pre.sourcelines > span.followlines-selected" selector as the background color
is otherwise overriden by "pre.sourcelines.stripes > :nth-child(4n+4)" rule.