Applying an empty changegroup has been an error since the
beginning. The only exception was strip, which would allow to apply an
empty changegroup from the temporary bundle. However, the emptyok=True
option was only set for bundle1 bundles. In other words, temporary
bundle2 bundles would fail if they were empty.
Bundle2 has now been used enough that it seems safe to say that we
simply don't create bundle2 bundles with empty changegroups. That also
suggests that we never create bundle1 bundles with empty changegroups
(i.e. empty bundle1 bundles, since bundle1 is just a changegroup),
because, AFAICT, the code leading up to the application of the bundle
is the same for bundle1 and bundle2.
Therefore, let's stop passing emptyok=True, so we more clearly get the
same behavior for bundle1 and bundle2.
The "patterns"/"include" in "patternspat"/"includepat" is redundant,
so drop it. Also a "_" prefix since it's "private".
Inline the "pm"/"im" variables.
The code was refactored during the move to be more procedural
instead of using string formatting. This has the benefit of not
writing empty sections, which changed tests.
The sparse extension maintains caches for the sparse files
to a signature and a signature to a matcher. This allows the
sparse matchers to be resolved quickly, which is apparently
something that can occur in loops.
This patch ports the sparse caches to the localrepo class
pretty much as-is. There is potentially room to improve the
caching mechanism. But that can be done as a follow-up.
The default invalidatecaches() now clears the relevant sparse
cache. invalidatesignaturecache() has been moved to sparse.py.
This method is reasonably well-contained and simple to move.
As part of the move, some light formatting was performed.
A "working copy" reference in an error message was changed to
"working directory."
The biggest change was to _refreshoncommit() in sparse.py. It
was previously checking for the existence of an attribute on
the repo instance. Since the moved function now returns empty
data if sparse isn't enabled, we unconditionally call the
new function. However, we do have to protect another method
call in that function. This will all be unhacked eventually.
Currently, the sparse extension sniffs repo instances for
attributes defined by the sparse extension to determine if
sparse is enabled. As we move code away from repo instances,
these checks will be a bit more brittle.
We introduce a module-level variable to track whether sparse is
enabled as a temporary workaround.
One more step towards weaning off methods on repo instances and
moving code to core. While this function is only used once and
is simple, it needs to exist on its own so Facebook can monkeypatch
it to enable simplecache integration.
This patch marks the beginning of moving code from the sparse
extension into core. The goal is to move as much of the
functionality as possible into core, where it will be an
experimental feature. The extension will likely continue to
exist to enable the feature and provide UI elements.
As part of the move, the repo method was converted to a module
function. It doesn't need to exist on repos.
An error message was also updated to reflect that an error isn't
necessarily from the .hg/sparse file. The API should be updated
later to pass in a filename so the error can be more descriptive.
Copyright of the added file was copied from the sparse extension.
vfs.exists() followed by a file read is an anti-pattern because it
incurs an extra stat() to test for file presence. vfs.tryread()
returns empty string on missing file and avoids the stat().
This was relying on garbage collection to close the opened
file, which is a bug. Both callers simply called into self.vfs
to resolve the path. So refactor to use the vfs layer.
While we're here, rename the method to reflect it is internal
and to break anyone relying on the old behavior.
Sparse checkout is still highly experimental and not protected
by BC guarantees yet. We also haven't had a discussion on the UX.
To discourage use, we rename the sparse command to debugsparse.
This is a 3rd party extension authored by Facebook. References in
core are not appropriate.
It will be possible to restore this code/optimization via
monkeypatching. So Facebook won't lose any functionality.
The removed code is important for performance. So add a comment
tracking it.
Facebook has developed an extension to enable "sparse" checkouts -
a working directory with a subset of files. This feature is a critical
component in enabling repositories to scale to infinite number of
files while retaining reasonable performance. It's worth noting
that sparse checkout is only one possible solution to this problem:
another is virtual filesystems that realize files on first access.
But given that virtual filesystems may not be accessible to all
users, sparse checkout is necessary as a fallback.
Per mailing list discussion at
https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-March/095868.html
we want to add sparse checkout to the Mercurial distribution via
roughly the following mechanism:
1. Vendor extension as-is with minimal modifications (this patch)
2. Refactor extension so it is more clearly experimental and inline
with Mercurial practices
3. Move code from extension into core where possible
4. Drop experimental labeling and/or move feature into core
after sign-off from narrow clone feature owners
This commit essentially copies the sparse extension and tests
from revision 71e0a2aeca92a4078fe1b8c76e32c88ff1929737 of the
https://bitbucket.org/facebook/hg-experimental repository.
A list of modifications made as part of vendoring is as follows:
* "EXPERIMENTAL" added to module docstring
* Imports were changed to match Mercurial style conventions
* "testedwith" value was updated to core Mercurial special value and
comment boilerplate was inserted
* A "clone_sparse" function was renamed to "clonesparse" to appease
the style checker
* Paths to the sparse extension in tests reflect built-in location
* test-sparse-extensions.t was renamed to test-sparse-fsmonitor.t
and references to "simplecache" were removed. The test always skips
because it isn't trivial to run it given the way we currently run
fsmonitor tests
* A double empty line was removed from test-sparse-profiles.t
There are aspects of the added code that are obviously not ideal.
The goal is to make a minimal number of modifications as part of
the vendoring to make it easier to track changes from the original
implementation. Refactoring will occur in subsequent patches.
I'm still cleaning this up, but it's easier to do in bite-size chunks
like this than all at once. The negative lookahead avoids one false
positive category from some output related to finding Subversion
bindings.
Differential Revision: https://phab.mercurial-scm.org/D13
In blockdescendants(), we had an assertion when line range of a merge
changeset was not consistent depending on which parent was considered for
computation. For instance, this might occur when file content (in lookup
range) is significantly different between parent branches of the merge as
demonstrated in added tests (where we almost completely rewrite the "baz" file
while also introducing similarities with its content in the other branch we
later merge to).
Now, in such case, we combine line ranges from all parents by storing the
envelope of both line ranges. This is conservative (the line range is
extended, possibly unnecessarily) but at least this should avoid missing
descendants with changes in a range that would fall in that of one parent but
not in another one (the case of "baz: narrow change (2->2+)" changeset in
tests).
Switch the lone call in merge.py to use it.
As with past refactors, the goal is to make wctx hot-swappable with an
in-memory context in the future. This change should be a no-op today.
Now, files (to be truncated) are opened with checkambig=True, only if
localrepository caches it.
Therefore, straightforward "copy if EPERM" is always reasonable to
avoid file stat ambiguity at closing.
This patch makes checkambigatclosing close wrapper copy the target
file, and advance mtime on it after renaming, if EPERM. This can avoid
file stat ambiguity, even if the target file is owned by another (see
issue5418 and issue5584 for detail).
This patch factors main logic out instead of changing
checkambigatclosing._checkambig() directly, in order to reuse it.
Now, transaction can determine whether avoidance of file stat
ambiguity is needed for each files, by blacklist "checkambigfiles".
For similarity to truncation in _playback(), this patch apply
checkambig=True only on limited files in code paths below.
- restoring files by util.copyfile(), in _playback()
(checkambigfiles itself is examined at first, because it as a
keyword argument might be None)
- writing files at finalization of transaction, in _generatefiles()
This patch reduces cost of checking stat at writing out and restoring
files, which aren't filecache-ed.
Advancing mtime by os.utime() fails for EPERM, if the target file is
owned by another. 0d920bcb0fd1 and related changes made some code
paths give advancing mtime up in such case, to fix issue5418.
This causes file stat ambiguity (again), if it is owned by another.
https://www.mercurial-scm.org/wiki/ExactCacheValidationPlan
To avoid file stat ambiguity in such case, especially for
.hg/dirstate, c75c7b3e3284 made vfs.rename() copy the target file, and
advance mtime of renamed one again, if EPERM (see issue5584 for detail).
But straightforward "copy if EPERM" isn't reasonable for truncation of
append-only files at rollbacking, because rollbacking might cost much
for truncation of many filelogs, even though filelogs aren't
filecache-ed.
Therefore, this patch introduces blacklist "checkambigfiles", and
avoids file stat ambiguity only for files specified in this blacklist.
This patch consists of two parts below, which should be applied at
once in order to avoid regression.
- specify 'checkambig=True' at vfs.open(mode='a') in _playback()
according to checkambigfiles
- invoke _playback() with checkambigfiles
- add transaction.__init__() checkambigfiles argument, for _abort()
- make localrepo instantiate transaction with _cachedfiles
- add rollback() checkambigfiles argument, for "hg rollback/recover"
- make localrepo invoke rollback() with _cachedfiles
After this patch, straightforward "copy if EPERM" will be reasonable
at closing the file opened with checkambig=True, because this policy
is applied only on files, which are listed in blacklist
"checkambigfiles".
This information is used to make transaction handle these files
specially, in order to avoid file stat ambiguity of them.
Gathering information about cached files via annotation classes can
avoid overlooking properties newly introduced in the future.
Add a 'successorssets' template that returns the list of all closest known
sucessorssets for a changectx. The elements of the list are changesets.
The "closest successors" are the first locally known revisions encountered
while, walking successors markers. It uses successorsets previously modified
to support the closest argument.
This logic respect repository filtering. So hidden revision will be skipped by
this logic unless --hidden is specified. Since we only display the visible
predecessors, this template will not display anything in most case. It makes a
good candidate for inclusion in the default log output.
I updated the test-obsmarker-template.t test file introduced with the
predecessors template to test successorssets template.
We add new tests for improving the coverage of existing obs-markers related
template (predecessors) and the new one we are introducing (successorssets).
Add a closest argument to successorssets changing the definition of latest
successors.
With "closest=false" (current behavior), latest successors are "leafs" on the
obsmarker graph. They don't have any successor and are known locally.
With "closest=true", latest successors are the closest locally-known
changesets that are visible in the repository or repoview. Closest successors
can be then obsolete, orphan.
This will be used in a later patch to show the closest successor of
changesets with the successorssets template.
We plan to add a new argument to successorsets. But first we need to update
all callers to pass cache argument explicitly to avoid arguments confusion.
Clarify successorssets documentation before we start updating the main function.
This patch serie will introduce the successorssets template, the opposite of
predecessor templates.
Successors will use successorssets function and requires some improvement so
before doing that, we clean up successorssets a bit.
It's possible to set up non-linear dependencies in Phabricator like:
o D4
|\
| o D3
| |
o | D2
|/
o D1
The old `phabread` code will print D1 twice. This patch adds de-duplication
to prevent that.
Test Plan:
Construct the above dependencies in a Phabricator test instance and make
sure the old code prints D1 twice while the new code won't.
Previously there is a suspicious `if False and delta > 0` which dates back
to the beginning of hgext/record.py (f995f03023c7).
The "trimming context lines" feature could be useful (and is used by the
next patch). So let's enable it. This patch adds a new `maxcontext`
parameter to `recordhunk` and `parsepatch`, changing the `if False`
condition to respect it.
The old `trimcontext` implementation is also wrong - it does not update
`toline` correctly and it does not do the right thing for `before` context.
A doctest was added to guard us from making a similar mistake again.
Since `maxcontext` is set to `None` (unlimited), there is no behavior
change.
Previously, we read Differential Revisions one by one by calling
`differential.query`.
Fetching them one by one is suboptimal. Unfortunately, there is no Conduit
API that allows us to get a stack of diffids using a single API call.
This patch tries to be smarter using a simple heuristic: when fetching D59
as a stack, previous IDs like D51, D52, D53, ..., D58 are likely belonging
to a same stack so just fetch them as well. Since `differential.query` only
returns cheap metadata without expensive diff content, it shouldn't be a big
problem for the server.
Using a test Phabricator instance, this patch reduces `phabread` reading a
10 patch stack from about 13 to 30 seconds to 8 seconds.
Previously, we call differential.getcommitmessage API to get commit
messages. Now we read that from "Differential Revision" object fetched
via "differential.query" API.
This removes one API call per patch.
This patch reworked phabread a bit so it fetches the lightweight
"Differential Revision" metadata for a stack first. Then read other data.
This allows the code to:
a) send 1 request to get all `hg:meta` data instead of N requests
b) patches are read in desired order, no need to buffer the output
"b)" reduces the memory usage from O(N^2) to O(N) since we no longer keep
old patch contents in memory.
The above `N` means the number of patches in the stack.
Previously we didn't abort. Now we abort if revs is empty. This is
consistent with "hg export" behavior. Maybe "return 1" is also a reasonable
behavior, but that's inconsistent with the existing "hg export".
Previously, `phabsend` uploads new diffs as long as the commit hash changes.
That's suboptimal because sometimes the diff is exactly the same as before,
the commit hash change is caused by a parent hash change, or commit message
change which do not affect diff content.
This patch adds a check examining actual diff contents to skip uploading new
diffs in that case.
The "hg:meta" property is for extra metadata to reconstruct the patch.
Previously it does not have node or parent information since I think by
reading the patch again, the commit message will be mangled (like, added the
"Differential Revision" line) and we cannot preserve the commit hash.
However, the "parent" information could be useful. It could be helpful to
locate the "base revision" so in case of a conflict applying the patch
directly, we might be able to use 3-way merge to resolve it correctly.
Note: "local:commits" is an existing "property" used by Phabricator that has
the node and parent information. However, it lacks of timezone information
and requires "author" and "authorEmail" to be separated. So we are using a
different "property" - "hg:meta" to be distinguished from "local:commits".
Previously, only tags can "associate" a changeset to a Differential
Revision. But the usual pattern (arc patch or hg phabread) is to put the
Differential Revision URL in commit message.
This patch makes the code read commit message to find associated
Differential Revision if associated tags are not found.
This makes some workflows possible. For example, if the author loses their
repo, or switch to another computer, they can continue download their own
patches from Phabricator and update them without needing to manually create
tags.
These are some simple cases. More to come in a future change.
Reviewers: krbullock
Reviewed By: krbullock
Differential Revision: https://phab.mercurial-scm.org/D4
This avoids some false positives in an upcoming check-code rule.
Reviewers: krbullock
Reviewed By: krbullock
Differential Revision: https://phab.mercurial-scm.org/D3
