Before this patch, failure of updating subrepos may cause inconsistent
".hgsubstate". For example:
1. dirstate entry for ".hgsubstate" of the parent repo is filled
with valid size/date (via "hg state" or so)
2. "hg update" is invoked at the parent repo
3. ".hgsubstate" of the parent repo is updated on the filesystem as
a part of "g"(et) action in "merge.applyupdates"
4. it is assumed that size/date of ".hgsubstate" on the filesystem
aren't changed from ones at (1)
this is not so difficult condition, because just changing hash
ids (every ids are same in length) in ".hgsubstate" doesn't
change the file size of it
5. "subrepo.submerge()" is invoked to update subrepos
6. failure of updating in one of subrepos raises exception
(e.g. "untracked file differs")
7. "hg update" is aborted without updating dirstate of the parent repo
dirstate entry for ".hgsubstate" still holds size/date at (1)
Then, ".hgsubstate" of the parent repo is treated as "CLEAN"
unexpectedly, because updating ".hgsubstate" at (3) doesn't change
size/date of it on the filesystem: see assumption at (4).
This inconsistent ".hgsubstate" status causes unexpected behavior, for
example:
- "hg revert" forgets to revert ".hgsubstate"
- "hg update" misunderstands that (not yet updated) subrepos diverge
(then, it shows the prompt to confirm user's decision)
To avoid inconsistent ".hgsubstate" status above, this patch marks
".hgsubstate" as possibly dirty before "submerge" invocation.
"normallookup"-ed (= dirty) dirstate should be written out, even if
processing is aborted by failure.
This patch marks ".hgsubstate" as possibly dirty before "submerge",
also when it is removed or merged while merging, for safety. This
should prevent Mercurial from misunderstanding inconsistent
".hgsubstate" as clean.
To satisfy conditions at (1) and (4) above, this patch uses "hg status
--config debug.dirstate.delaywrite=2" (to fill valid size/date into
dirstate) and "touch" (to fix date of the file).
This change touches every module in which repository.wopener was being used, and
changes it for the equivalent repository.wvfs.
It should now be possible to remove localrepo.wopener.
This change touches every module in which repository.opener was being used, and
changes it for the equivalent repository.vfs. This is meant to make it easier
to split the repository.vfs into several separate vfs.
It should now be possible to remove localrepo.opener.
This moves most reading of filelogs out of manifestmerge, making it
easy for a narrow clone extension to filter out or translate unwanted
actions before any filelogs are read. The only call left is inside of
copies.mergecopies(), which can be overridden separately at a lower
level.
We still have one case of a call to _checkunknownfile() in
manifestmerge(): when force=True and branchmerge=True and the remote
side has a file that the local side doesn't. This combination of
arguments is used by 'hg merge --force', but also by rebase and
unshelve. In this scenario, we try to create the file from the
contents from the remote, but if there is already a local untracked
file in place, we merge it instead.
When a directory was renamed and a new untracked file was added in the
new directory and the remote directory added a file by the same name
in the old directory, the local untracked file gets overwritten, as
demonstrated by the broken test case in test-rename-dir-merge.
Fix by checking for unknown files for 'dg' actions too. Since
_checkunknownfile() currently expects the same filename in both
contexts, we need to add a new parameter for the remote filename to
it.
The 'c' and 'dc' actions include creating a file on disk and we need
to check that no conflicting file exists unless force=True. Move two
of the calls to _checkunknownfile() to a single place at the end of
manifestmerge(). This removes some of the reading of filelogs from the
heart of manifestmerge() and collects it in one place close to where
its output (entries in the 'aborts' list) is used.
Note that this removes the unnecessary call to _checkunknownfile()
when force=True in one of the code paths.
_checkunknownfile() reads the filelog of the remote side's file. For
narrow clones, the filelog will not exist for all files and we need a
way to avoid reading them. While it would be easier for the narrow
extension to just override _checkunknownfile() and make it ignore
files outside the narrow clone, it seems cleaner to have
manifestmerge() not care about filelogs (considering its
name).
In order to move the calls to _checkunknownfile() out, we need to be
able to tell in which cases we should check for unknown files. Let's
start by introducing a new action distinct from 'g' for this
purpose. Specifically, the new action will be just like 'g' except
that it will check that for conflicting unknown files first. For now,
just add the new action type and convert it to 'g'.
This simplifies largefiles' overridecalculateupdates(), which no
longer has to do the conversion it started doing in 478d610ca1b0
(largefiles: rewrite merge code using dictionary with entry per file,
2014-12-09).
To keep this patch small, we'll leave the name 'actionbyfile' in
overrides.py. It will be renamed in the next patch.
By moving the conversion from the file->action dict after
_forgetremoved(), we make that method shorter by removing the need for
the confusing 'xactions' variable.
By moving the conversion from the file->action dict after the bid
merge code, bid merge can be simplified a little.
A few tests are affected by this change. Where we used to iterate over
the actions first in order of the action type ('g', 'r', etc.) [1], we
now iterate in order of filename. This difference affects the order of
debug log statements.
[1] And then in the non-deterministic order of files in the manifest
dictionary (the order returned from manifest.diff()).
In the same vein as 478d610ca1b0 (largefiles: rewrite merge code using
dictionary with entry per file, 2014-12-09), rewrite manifestmerge()
itself as dictionary with the filename as key. This will let us
simplify some of the other code in merge.py and eventually drop the
conversion in the largefiles code.
No difference in speed could be detected (well within the noise level
when run in Mozilla repo).
When there are multiple common ancestors, we should check for case
collisions only on the resulting actions after bid merge has run. To
do this, move the code until after bid merge.
Move it past _resolvetrivial() too, since that might update
actions. If the remote changed a file and then reverted the change,
while the local side deleted the file and created a new file with a
name that case-folds like the old file, we should fail before this
patch but not after.
Although the changes to the actions caused by _forgetremoved() should
have no effect on case collisions, move it after that, too, so the
next person reading the code won't have to think about it.
Moving it past these blocks of code takes it to the end of
calculateupdates(), so let's even move it outside of the method, so we
also check collisions in actions produced by extensions overriding the
method.
By moving the cd/dc prompts out of calculateupdates(), we let
largefiles' overridecalculateupdates() so the unresolved values
(i.e. 'cd' or 'dc' rather than 'g', 'r', 'a' and missing). This allows
overridecalculateupdates() to ask the user whether to keep the normal
file or the largefile before the user gets the cd/dc prompt. Whichever
answer the user gives, we make overridecalculateupdates() replace 'cd'
or 'dc' action, saving the user one annoying (and less clear)
question.
We would eventually like to move the resolution of modify/delete and
delete/modify conflicts to the resolve phase. However, we don't want
to move the checks for identical content that were added in
99b29d2bd5ed (merge: before cd/dc prompt, check that changed side
really changed, 2014-12-01). Let's instead move these out to a new
_resolvetrivial() function that processes the actions from
manifestmerge() and replaces any false cd/dc conflicts. The function
will also provide a natural place for us to later add code for
resolving false 'm' conflicts.
As preparation for making 'dr' and 'rd' actions no longer actions,
move the reporting from applyupdates() to its caller update(). This
way we won't have to pass additonal arguments to applyupdates() when
they are no longer actions. Also, the warnings are equally unrelated
to applyupdates() as they are to recordupdates(), as they don't result
in any changes to either the working copy or the dirstate.
See earlier patch for additional motivation.
It is easier to reason about certain algorithms in terms of a
file->action mapping than the current action->list-of-files. Bid merge
is already written this way (but with a list of actions per file), and
largefiles' overridecalculateupdates() will also benefit. However,
that requires us to have at most one action per file. That requirement
is currently violated by 'dr' (divergent rename) and 'rd' (rename and
delete) actions, which can exist for the same file as some other
action.
These actions are only used for displaying warnings to the user; they
don't change anything in the working copy or the dirstate. In this
way, they are similar to the 'k' (keep) action. However, they are even
less action-like than 'k' is: 'k' at least describes what to do with
the file ("do nothing"), while 'dr' and 'rd' or only annotations for
files for which there may exist other, "real" actions.
As a first step towards separating these acitons out, stop including
them in the progress output, just like we already exclude the 'k'
action.
Most merge action messages don't describe the action itself, they
describe the reason the action was taken. The only exeption is the 'k'
action, for which the message is just "keep" and instead there is a
code comment folling it that says "remote unchanged". Let's move that
comment into the merge action message.
When the local side has renamed a directory from a/ to b/ and added a
file b/c in it, and the remote side has added a file a/c, we end up
overwriting the local file b/c with the contents of remote file
a/c. Add a check for this case and use the merge ('m') action in this
case instead of the directory rename get ('dg') action.
When the remote side has renamed a directory from a/ to b/ and added a
file b/c in it, and the local side has added a file a/c, we end up
moving a/c to b/c without considering the remote version of b/c. Add a
check for this case and use the merge ('m') action in this case
instead of the directory rename ('dm') action.
There are three high-level cases that are of interest in
manifestmerge(): 1) The file exists on both sides, 2) The file exists
only on the local side, and 3) The file exists only on the remote
side. Let's make this clearer in the code.
The 'if f in copied' case will be broken up into the two applicable
branches in the next patch.
The order is determined by manifest.diff(), which currently is not
sorted. There are currently no tests for this, but we will soon add
some that would be flaky without this patch.
When looking for untracked files that would conflict with a tracked
file in the target revision (or the remote side of a merge), we
explcitly exclude ignored files. The code was added in f1db75422e70
(merge: refactor unknown file conflict checking, 2012-02-09), but it
seems like only unknown, not ignored, files were considered since the
beginning of time.
Although ignored files are mostly build outputs and backup files, we
should still not overwrite them. Fix by simply removing the explicit
check.
Before, merging would in some cases ask "wrong" questions about
"changed/deleted" conflicts ... and even do it before the resolve phase where
they can be postponed, re"resolved" or answered in bulk operations.
Instead, check that the content of the changed file really did change.
Reading and comparing file content is expensive and should be avoided before
the resolve phase. Prompting the user is however even more expensive. Checking
the content here is thus better.
The 'f in ancestors[0]' should not be necessary but is included to be extra
safe.
Instead of using a file that we know is not in the common ancestor's
maniffest, let's use None. This is safe as the only place that cares
about the value (applyupdates) already checks if the item exists in
the ancestor.
We can further limit the scope of the 2-way merge case by breaking out
the case where the file was not created from scratch on both sides but
rather renamed in the same way (and is therefore a 3-way merge). This
involves copying some code, but it makes it clearer which case the
"Note:" in the code refers to.
When 'f' is not in 'ma', 'a' will be 'nullid' and all the if/elif
conditions that check whether some one nodeid is equal to 'a' will
fail, and the else-clause will instead apply. We can make that more
explicit by creating a separate 'm' action for the case where 'a' is
'nullid'. While it does mean copying some code, perhaps it makes it a
little clearer which codepaths are possible, and which cases the
"Note:" in the code refers to. It also lets us make the debug action
messages a little more specific.
Since 4a56fba99974 (merge: don't use unknown(), 2012-02-09), untracked
files are no longer included in the manifest diff, so there is no need
to check exclude them when renaming files for directory moves with the
'dm' action.
calculateupdates() happens before applyupdates(), so move it before in
the code. That also moves it close to manifestmerge(), which is a good
location as calculateupdates() is the only caller of manifestmerge().
manifestmerge() has a piece of code that's roughly:
if not force and different:
abort
else:
# if different: old untracked f may be overwritten and lost
...
The comment only talks about what happens when 'different' is true,
and in combination with the if-block above, that must mean that it is
only about what happens when 'force and different'. It seems quite
fine that files are overwritten when 'force' is true, so let's remove
the comment. As it stands, it can easily be interpreted as a TODO
(which is how I interpreted it at first).
As far as I and the test suite can tell, the checks in manifestmerge()
already report the errors (whether or not --check is given), so we
don't need to call merge.checkunknown(). Since this is the last call
to the method, also remove the method.
The method has been called from commands.py since 8d9ca2ac2fe8
(update: just merge unknown file collisions, 2012-02-09), so drop the
underscore prefix that suggests that it's private.
From manifest.diff(), we return a dict from filename to pairs of pairs
of file nodeids and flags (values of the form ((n1,n2),(fl1,fl2))). To
create this dict, we currently generate one dict for files (with
(n1,n2) values) and one for flags (with (fl1,fl2) values) and then
join these dicts. Missing files are represented by None and missing
flags by '', but due to the dict joining, the inner pairs themselves
can also be None. The only caller, merge.manifestmerge(), then unpacks
these values while checking for None values.
By inlining the calls to dicthelpers and simplifying it to only
iterate over files (ignoring flags-only differences), we can simplify
life for our caller.
The manifestdict class already has a method for diff flags between two
manifests (presumably because there is no full access to the private
_flags field). The only caller is merge.manifestmerge(), which also
wants a diff of files between the same manifests. Let's combine the
code for diffing files and flags into a single method on
manifestdict. This puts all the manifest diffing in one place and will
allow for further simplification. It might also be useful for it to be
encapsulated in manifestdict if we later decide to to shard
manifests. The docstring is intentionally unclear about missing
entries for now.
