Our abstract interfaces are more useful if we guarantee that
implementations conform to certain rules. Namely, we want to ensure
that objects implementing interfaces don't expose new public
attributes that aren't part of the interface. That way, as long as
consumers don't access "internal" attributes (those beginning with
"_") then (in theory) objects implementing interfaces can be swapped
out and everything will "just work."
We add a test that enforces our "no public attributes not part
of the abstract interface" rule.
We /could/ implement "interface compliance detection" at run-time.
However, that is littered with problems.
The obvious solutions are custom __new__ and __init__ methods.
These rely on derived types actually calling the parent's
implementation, which is no sure bet. Furthermore, __new__ and
__init__ will likely be called before instance-specific attributes
are assigned. In other words, they won't detect public attributes
set on self.__dict__. This means public attribute detection won't
be robust.
We could work around lack of robust self.__dict__ public attribute
detection by having our interfaces implement a custom __getattribute__,
__getattr__, and/or __setattr__. However, this incurs an undesirable
run-time penalty. And, subclasses could override our custom
method, bypassing the check.
The most robust solution is a non-runtime test. So that's what this
commit implements. We have a generic function for validating that an
object only has public attributes defined by abstract classes. Then,
we instantiate some peers and verify a newly constructed object
plays by the rules.
Differential Revision: https://phab.mercurial-scm.org/D339
The wirepeer class provides concrete implementations of peer interface
methods for calling wire protocol commands. It makes sense for this
class to inherit from the peer abstract base class. So we change
that.
Since httppeer and sshpeer have already been converted to the new
interface, peerrepository is no longer adding any value. So it has
been removed. httppeer and sshpeer have been updated to reflect the
loss of peerrepository and the inheritance of the abstract base
class in wirepeer.
The code changes in wirepeer are reordering of methods to group
by interface.
Some Python code in tests was updated to reflect changed APIs.
.. api::
peer.peerrepository has been removed. Use repository.peer abstract
base class to represent a peer repository.
Differential Revision: https://phab.mercurial-scm.org/D338
Invoking *.sh on Windows leads to the "what program should open this?" prompt,
which stalls the test and led to the recent series of exceptions on the Windows
test machine as the runner times out.
The old error message "cannot use revision REV as base, result would have 3
parents" is confusing - why use REV as base? why add a new parent?.
This patch changes it to "cannot move parent", which seems better.
Differential Revision: https://phab.mercurial-scm.org/D342
"defineparents" is the core algorithm of rebase. The old code has too many
tech debts (like outdated comments, confusing assertions, etc) and is very
error-prone to be improved. This patch rewrites "defineparents" to make the
code easier to reason about, and solve a bunch of issues, including:
- Assertion error: no base found (demonstrated by D212, issue5578)
- Asymmetric result (demonstrated by D211, "F with one parent")
- Wrong new parent (demonstrated by D262, "C':A'A'")
- "revlog index out of range" (demonstrated by D262, issue5630)
- "nothing to merge" (demonstrated by D262)
As a side effect, merge base decision has been made more solid - rebase now
prints out explicitly what could go wrong when it cannot find a unique
suitable merge base.
.. fix:: Issue 5578, Issue 5630
Core rebase algorithm has been rewritten to be more robust.
Differential Revision: https://phab.mercurial-scm.org/D21
evolution* config has been rewritten in stabilization* in the previous patch,
update tests file to use the new names.
Differential Revision: https://phab.mercurial-scm.org/D249
morestatus extension in fbext use to show more context about the state of the
repo like the repository is in a unfinished merge state, or a rebase is going
on, or histedit is going on, listing the files which need to be resolved and
also suggesting ways to handle the situation.
This patch moves the extension directly to core by plugging it into the
--verbose flag of the status command. So now if you are in any unfinished state
and you do hg status -v, it will show you details and help related to the state.
The extension in fbext also shows context about unfinished update state
which is not ported to core as that plug in hooks to update command which need
to be tackled somewhat differently.
The following configuration will turn the behaviour on by default
[commands]
status.verbose = 1
You can also skip considering some states like bisect as follows:
[commands]
status.skipstates=bisect
This patch also adds test for the feature.
.. feature::
``hg status -v`` can now show unfinished state. For example, when in
an unfinished rebase state, ``hg status -v`` might show::
# The repository is in an unfinished *rebase* state.
# No unresolved merge conflicts.
# To continue: hg rebase --continue
# To abort: hg rebase --abort
Differential Revision: https://phab.mercurial-scm.org/D219
Changeset aa97e972460f introduce more complex logic around
'bundleoperation.gettransaction'. In that process it turns the old "attribute"
into a proper method which breaks the code that detects the "transaction
availability".
The change was visible in 'test-acl.t', fixing this reverts the test changes.
Differential Revision: https://phab.mercurial-scm.org/D303
The last use of this API was removed in 3bcb9f9a4a63 in 2016. While
not formally deprecated, as of the last commit the code is no longer
explicitly tested. I think the new API has existed long enough for
people to transition to it.
I also have plans to more formalize the peer API and removing batch()
makes that work easier.
I'm not convinced the current client-side API around batching is
great. But it's the best we have at the moment.
.. api:: remove peer.batch()
Replace with peer.iterbatch().
Differential Revision: https://phab.mercurial-scm.org/D320
The remote batching code is difficult to read. Let's improve it.
As part of the refactor, the future returned by method calls on
batchiter() instances is now populated. However, you still need to
consume the results() generator for the future to be set. But at
least now we can stuff the future somewhere and not have to worry
about aligning method call order with result order since you can
use a future to hold the result.
Also as part of the change, we now verify that @batchable generators
yield exactly 2 values. In other words, we enforce their API.
The non-iter batcher has been unused since 3bcb9f9a4a63. And to my
surprise we had no explicit unit test coverage of it! test-batching.py
has been overhauled to use the iterating batcher.
Since the iterating batcher doesn't allow non-batchable method
calls nor local calls, tests have been updated to reflect reality.
The iterating batcher has been used for multiple releases apparently
without major issue. So this shouldn't cause alarm.
.. api::
@peer.batchable functions must now yield exactly 2 values
Differential Revision: https://phab.mercurial-scm.org/D319
@peer.batchable decorated generator functions have two forms:
yield value, None
and
yield args, future
yield value
These forms have been present since the decorator was introduced.
There are currently no in-repo consumers of the first form. So this
commit removes support for it.
Note that remoteiterbatcher.submit() asserts the 2nd form. And
3bcb9f9a4a63 removed the last user of remotebatcher, forcing everyone
to remoteiterbatcher. So anything relying on this in the wild would
have been broken since 3bcb9f9a4a63.
.. api::
@peer.batchable can no longer emit local values
Differential Revision: https://phab.mercurial-scm.org/D318
If someone changes "pick" to "roll" or "fold" for the first
changeset in a histedit rule Mercurial could remove a wrong
changeset if the phase is non-public.
roll or fold for the first changeset should be invalid.
This vulnerability was fixed by the previous patch and there were more ways
to exploit than using '|shellcmd'. So it doesn't make sense to reject only
pipe character.
Test cases are updated to actually try to exploit the bug. As the SSH bridge
of git/svn subrepos are not managed by our code, the tests for non-hg subrepos
are just removed.
This may be folded into the original patches.
Before this patch, explicit --pager=on is unintentionally ignored by
any disabling factor, even if priority of it is less than --pager=on
(e.g. "[ui] paginate = off").
'ssh://' has an exploit that will pass the url blindly to the ssh
command, allowing a malicious person to have a subrepo with
'-oProxyCommand' which could run arbitrary code on a user's machine. In
addition, at least on Windows, a pipe '|' is able to execute arbitrary
commands.
When this happens, let's throw a big abort into the user's face so that
they can inspect what's going on.
'ssh://' has an exploit that will pass the url blindly to the ssh
command, allowing a malicious person to have a subrepo with
'-oProxyCommand' which could run arbitrary code on a user's machine. In
addition, at least on Windows, a pipe '|' is able to execute arbitrary
commands.
When this happens, let's throw a big abort into the user's face so that
they can inspect what's going on.
'ssh://' has an exploit that will pass the url blindly to the ssh
command, allowing a malicious person to have a subrepo with
'-oProxyCommand' which could run arbitrary code on a user's machine. In
addition, at least on Windows, a pipe '|' is able to execute arbitrary
commands.
When this happens, let's throw a big abort into the user's face so that
they can inspect what's going on.
The test for duplicate flag processors depended on the timestamps
being set in the dirstate to work. If the time between the the
previous failed commit (which would set the timestamp, due to bug
5645) and the attempted commit with the duplicate flag processors was
small enough, it would fail. The failure was caused by a call to
commands.status() early in the commit process. If the dirstate did not
have the timestamp set, it would need to fetch the file content to
compare with. Since two flag processors had been registered, it would
attempted to base64 decode the contents twice, which would of course
fail.
This patch adds a "hg debugrebuilddirstate" to make it deterministic
and also replaces the test case's "hg commit" by simply "hg status",
since that will trigger reading of the contents and thereby use of the
flag processors as noted above.
Differential Revision: https://phab.mercurial-scm.org/D202
The flag processors test for duplicate processors for a single flag
was misleading because the file from the previous test case caused it
to fail (making the "echo 'this should fail' > file" part
irrelevant). Let's remove the leftover from the previous test case to
make it clear that duplicate flag processors results only in a
warning.
Note that duplicate flag processors would have resulted in a failure
(not just a warning) until b319e3173a95 (extensions: catch uisetup and
extsetup failures and don't let them break hg, 2017-06-06). I remember
expressing my concern about ending up with half-loaded extensions. It
would be pretty unfortunate to have double-encoded revlog content
enter a repo, so maybe we should reconsider?
Differential Revision: https://phab.mercurial-scm.org/D201
Invocation of "diff tool.py" in test-extdiff.t tests whether
shellquote() is applied on specified command as expected.
But direct invocation of "*.py" file might cause unexpected result on
Windows according to suffix binding.
For example, starting IDE, showing dialog to choose program to be
used, and so on. In such case, running test-extdiff.t is easily timed
out.
This patch uses intermediate *.bat file on Windows, to avoid such
unexpected result. Naming that intermediate file as "diff tool.bat" is
enough to test applying shellquote().
"diff" command might cause redundant message, "No differences
encountered" on Solaris for example. But suppressing option like "-q"
isn't portable, because POSIX specification doesn't define it.
pdiff script was introduced by f4cba8b2e7b4 to stabilize output of
standard diff command on each platforms.
Before this patch, pdiff script returns 0, even if diff is detected.
This issue doesn't cause failure of tests using it, if it is invoked
via extdiff extension, because extdiff itself examines changes between
specified revisions and decides exit code.
BTW, this patch ignores recursive comparison case, because:
- there is no portable way for current while-read based
implementation to return 1 at detecting changes
- it isn't yet needed to replace direct "diff -r" invocation by
pdiff for portability
We had the first 3 tests in test-profile.t fail because the output
didn't match. I have not yet confirmed that this was because no
samples were collected, but we shouldn't require samples to be
collected for the test to pass either way.
Differential Revision: https://phab.mercurial-scm.org/D199
In Pypy 5.6.0, traceback exception classes are not displayed with their full
qualified name.
Instead of displaying mercurial.error.ProgrammingError, Pypy displays
ProgrammingError.
Update the test to support both version.
Pypy 5.6.0 saves cached bytecode files in __pycache__ directory, clean them in
tests to fix loading old test extensions code.
Doing so should also helps for Python3.x migration.
statichttprepo inherits from localrepository. In doing so, it
obtains default implementations of various methods, like wlock().
Before this change, tags cache writing would call repo.wlock().
This failed on statichttprepo due to localrepository's wlock()
looking for an instance attribute that doesn't exist on statichttprepo
(statichttprepo doesn't call localrepository.__init__).
We /could/ define missing attributes until the base wlock() works.
However, a statichttprepo is remote and read-only and can't be
locked. The class already has a lock() that short circuits. So
it makes sense to implement a short-circuited wlock() as well. That
is what this patch does.
LockError is expected to be raised when locking fails. The constructor
takes a number of arguments that are local repository centric. Rather
than rework LockError to not require them (which would not be
appropriate for stable), this commit populates dummy values. I don't
believe they'll ever be seen by the user, as lock failures on
static http repos should be limited to well-defined (and tested)
scenarios. We can and should revisit the LockError type to improve
this.
