This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11239

debugcommands: introduce a debug command to repair repos affected by issue6528
ClosedPublic

Authored by Alphare on Aug 2 2021, 9:36 AM.

Details

Reviewers
marmoute
pulkit
Group Reviewers
hg-reviewers
Commits
rHGb30a53ffbf9b: debugcommands: introduce a debug command to repair repos affected by issue6528
Summary

This command is quite basic and slow, it will loop over the entirety of the
filelogs in the repository and check each revision for corruption, then fixes
the affected filelogs. It takes under 25 minutes for Mozilla-Central on my
not-top-of-the-line laptop, using the --to-report and --from-report options
will make this pretty tolerable to use, I think.

This change also introduces a test for the fix.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Aug 2 2021, 9:36 AM
marmoute requested changes to this revision.Aug 2 2021, 3:03 PM
marmoute added a subscriber: marmoute.

We are moving in the right direction, but I made multiple comment about adjustement I feel necessary before we can consider this function safe.

mercurial/debugcommands.py
1457

small nits: maybe "debug-repair-issue6528" instead of "debug-fix-issue6528" ?

1475

idea for improvement (might be useful t write them down in comment:

allow to specify filelog pattern and revision pattern to restrict the search. Useful for faster scan when user know what they are looking for.

1482

The following format would seems more suitable:

<ascii-hex of the corrupted file revision> <unencoded-filename>

Putting the hex first is simpler, since we then know the rest of the line if about the filename.

Putting the filename instead of encoded index path means we are more independant of the underlying format variante (with or without dot-encode for example).

1484

(not sure this is really unlikely, I would drop that remark)

1493

Why do we need the bool here ?

1497

Same feedback about fix → repair

mercurial/revlogutils/rewrite.py
479

note: this seems "fine" to me for stable. However this seems a good indication that we need some store/repo level API for "give me all the filelog"

482

It is probably worth adding a programming error if the repository is not locked.

492

This seems the only line that check and enforce the version.

We need:

  1. a programming error to catch the issue on Windows/Optimized setting
  2. a higher level check of the repository format in the top level command
508–512

If there is a crash/interrupt during the operation, we will leave temporary file behind. We should add a try:, finally: clause here, and possibly a no-interrupt context.

522–526

nits: this is the sensitive part, it is probably worth factoring it out in a small dedicated function since most of it is common, but for the offset computation.

What do you think ?

I addition it seems like a good idea to mention the "speedup" idea from Joerg (about only checking the delta when possible)

543

Since this is a sensitive issue, we should probably add a docstring about the detection method here.

562

Not sure what "unfortunate cyclic sitatuation" means here ‽

584

I don't think we capitalize error output in general.

586

tiny nits: this should probably be a msg %= on the previous line

596

You should probably sort the resulting to_fix set to make sure we process things in a predictable order (and start to end seems like a good idea in general)

607

adding uninterruptible for the full command seems quite overkill, especially since the command can apparently run for half an hours.

See my earlier comment about only using ui.uninterruptible() while doing the actual fixing.

653

small nits: if to_fix: seem more pythonic to me

654–657

We should sort to_fix to ensure growing order.

660

I don't think we capitalize output.

tests/test-issue6528.t
170–171

We should make this is ===== level title and use ----- level title (and associated description) for the individual tests within this section.

181–183

We should add debug index call here to show they are swapped

196–197

small nits: the output says we "found" the corruption, but does not state that we fixed it.

We should also check the index content after the command run.

219–220

Testing a multi-lines report seems important too.

This revision now requires changes to proceed.Aug 2 2021, 3:03 PM
Alphare marked 23 inline comments as done.Aug 4 2021, 9:16 AM

Pending CI refresh

mercurial/debugcommands.py
1482

I've updated the patch to a new format that uses unencoded filenames but still groups the revisions per filelog (when exporting, the format still allows for duplicate filelogs, it's just less efficient since it's just looping over lines).

This is done following an IRC discussion where @marmoute and I disagreed on the exact format, and this is the version I feel is simpler. If more people disagree with my choice, I'll change it, but it's probably not worth bikeshedding too hard on this.

tests/test-issue6528.t
219–220

I've created a multiple revisions + multiple files repo

Alphare marked an inline comment as done.Aug 4 2021, 10:22 AM
Alphare retitled this revision from debugcommands: introduce a debug command to fix repos affected by issue6528 to debugcommands: introduce a debug command to repair repos affected by issue6528.
Alphare updated this revision to Diff 29790.
Alphare updated this revision to Diff 29791.Aug 4 2021, 11:07 AM
marmoute requested changes to this revision.Aug 4 2021, 3:55 PM

Added a handful of new comments.

mercurial/debugcommands.py
1482

I am fine with using multiple nodes per lines but I could probably use , for separation between node and still for separation with the filename.

I would also gather all the lines before processing since that isn't very costly. However this is a quite minor changes

mercurial/revlogutils/rewrite.py
482

note: this is not the right location for this comment :-/ the lock checking should be done when we actually rewrite revlog.

We do not usually needs it, but the rewrite code is "exceptionnal" enough to add some double checking.

496–498

This should go in _reorder_filelog_parents (I am starting to suspect phabricator simply shifted all comments a couple of line too low)

534

We need a nointerrupt context here, don't we ?

579–580

It would seem cleaner to have this function only determine the status of the revision and have the warning message only issued by the called of this method.

The "corrupted" is a bit scary too. maybe "affected by issue6528"

This will be useful if we want to detect this in more situation (e.g. during exchange)

619–620

Here it is. It still feel like too wide and in the wrong place:

  • we want it as narrow as possible since it is "a bit hacky"
  • we want it in the function that actually setup the dangerous bit so that it (1) is always in place (2) stay next to the dangerous implementation.
640–644

Looks like you want to:

  1. gather the list of filelog
  2. use its len(xxx) for total
  3. use that list for the iteration
This revision now requires changes to proceed.Aug 4 2021, 3:55 PM
Alphare marked 7 inline comments as done.Aug 5 2021, 4:09 AM
Alphare added inline comments.
mercurial/revlogutils/rewrite.py
640–644

I guess the memory footprint wouldn't get too bad even on larger repos.

baymax updated this revision to Diff 29800.Aug 5 2021, 5:11 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

marmoute added inline comments.Aug 5 2021, 5:22 AM
mercurial/revlogutils/rewrite.py
640–644

Yeah, I dont expect thousand of these, and if so, the associated repository probably have large machine using it. (and worst case the report file can be feed in multiple step).

marmoute accepted this revision.Aug 6 2021, 6:03 AM
marmoute added inline comments.
mercurial/revlogutils/rewrite.py
482

My two past self eventually talked to each other and we probably want is in both. I'll prepare you a patch you can fold into this one.

617

This is probably a bit too verbose for the common case. I'll prepare something to control that.

marmoute added a comment.Aug 6 2021, 6:20 AM

So we have this part covered (slowly). We can now (in other Diffs) move to the next points.

  • implement a faster method
  • fix this issue on the fly during exchange
  • ensure things are fixed on upgrade (cf D11252)
mercurial/revlogutils/rewrite.py
482

Actually, I amd now triple confused about what I meant, so lets leave it that way,

617

Actually, this is only issued when processing report, so this seems fine.

Alphare marked an inline comment as done.Aug 6 2021, 6:41 AM
Alphare updated this revision to Diff 29829.
Alphare updated this revision to Diff 29835.Aug 6 2021, 1:50 PM
marmoute accepted this revision.Aug 6 2021, 4:47 PM
baymax updated this revision to Diff 29843.Aug 7 2021, 9:47 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

baymax updated this revision to Diff 29855.Aug 7 2021, 6:09 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

pulkit accepted this revision.Aug 10 2021, 8:16 AM
This revision is now accepted and ready to land.Aug 10 2021, 8:16 AM

Revision Contents
Changeset List

Diff 29875

mercurial/debugcommands.py

pathutil, pathutil,
phases, phases,
policy, policy,
pvec, pvec,
pycompat, pycompat,
registrar, registrar,
repair, repair,
repoview, repoview,
requirements,
revlog, revlog,
revset, revset,
revsetlang, revsetlang,
scmutil, scmutil,
setdiscovery, setdiscovery,
simplemerge, simplemerge,
sshpeer, sshpeer,
sslutil, sslutil,
procutil, procutil,
stringutil, stringutil,
urlutil, urlutil,
) )
from .revlogutils import ( from .revlogutils import (
deltas as deltautil, deltas as deltautil,
nodemap, nodemap,
rewrite,
sidedata, sidedata,
) )
release = lockmod.release release = lockmod.release
table = {} table = {}
table.update(strip.command._table) table.update(strip.command._table)
command = registrar.command(table) command = registrar.command(table)
if opts[b'show_matcher'] or (opts[b'show_matcher'] is None and ui.verbose): if opts[b'show_matcher'] or (opts[b'show_matcher'] is None and ui.verbose):
ui.writenoi18n(b'* matcher:\n', stringutil.prettyrepr(m), b'\n') ui.writenoi18n(b'* matcher:\n', stringutil.prettyrepr(m), b'\n')
for f in sorted(files): for f in sorted(files):
if not m(f): if not m(f):
continue continue
ui.write(b"%s\n" % f) ui.write(b"%s\n" % f)
@command(
b"debug-repair-issue6528",
marmouteUnsubmitted
Done

small nits: maybe "debug-repair-issue6528" instead of "debug-fix-issue6528" ?

marmoute: small nits: maybe "debug-repair-issue6528" instead of "debug-fix-issue6528" ?
[
(
b'',
b'to-report',
b'',
_(b'build a report of affected revisions to this file'),
_(b'FILE'),
),
(
b'',
b'from-report',
b'',
_(b'repair revisions listed in this report file'),
_(b'FILE'),
),
]
+ cmdutil.dryrunopts,
)
marmouteUnsubmitted
Done

idea for improvement (might be useful t write them down in comment:

allow to specify filelog pattern and revision pattern to restrict the search. Useful for faster scan when user know what they are looking for.

marmoute: idea for improvement (might be useful t write them down in comment: allow to specify filelog…
def debug_repair_issue6528(ui, repo, **opts):
"""find affected revisions and repair them. See issue6528 for more details.
The `--to-report` and `--from-report` flags allow you to cache and reuse the
computation of affected revisions for a given repository across clones.
The report format is line-based (with empty lines ignored):
marmouteUnsubmitted
Not Done

The following format would seems more suitable:

<ascii-hex of the corrupted file revision> <unencoded-filename>

Putting the hex first is simpler, since we then know the rest of the line if about the filename.

Putting the filename instead of encoded index path means we are more independant of the underlying format variante (with or without dot-encode for example).

marmoute: The following format would seems more suitable: ``` <ascii-hex of the corrupted file revision>…
AlphareAuthorUnsubmitted
Done

I've updated the patch to a new format that uses unencoded filenames but still groups the revisions per filelog (when exporting, the format still allows for duplicate filelogs, it's just less efficient since it's just looping over lines).

This is done following an IRC discussion where @marmoute and I disagreed on the exact format, and this is the version I feel is simpler. If more people disagree with my choice, I'll change it, but it's probably not worth bikeshedding too hard on this.

Alphare: I've updated the patch to a new format that uses unencoded filenames but still groups the…
marmouteUnsubmitted
Done

I am fine with using multiple nodes per lines but I could probably use , for separation between node and still for separation with the filename.

I would also gather all the lines before processing since that isn't very costly. However this is a quite minor changes

marmoute: I am fine with using multiple nodes per lines but I could probably use `,` for separation…
```
<ascii-hex of the affected revision>,... <unencoded filelog index filename>
marmouteUnsubmitted
Done

(not sure this is really unlikely, I would drop that remark)

marmoute: (not sure this is really unlikely, I would drop that remark)
```
There can be multiple broken revisions per filelog, they are separated by
a comma with no spaces. The only space is between the revision(s) and the
filename.
Note that this does *not* mean that this repairs future affected revisions,
that needs a separate fix at the exchange level that hasn't been written yet
(as of 5.9rc0).
marmouteUnsubmitted
Done

Why do we need the bool here ?

marmoute: Why do we need the bool here ?
"""
cmdutil.check_incompatible_arguments(
opts, 'to_report', ['from_report', 'dry_run']
)
marmouteUnsubmitted
Done

Same feedback about fix → repair

marmoute: Same feedback about `fix` → `repair`
dry_run = opts.get('dry_run')
to_report = opts.get('to_report')
from_report = opts.get('from_report')
# TODO maybe add filelog pattern and revision pattern parameters to help
# narrow down the search for users that know what they're looking for?
if requirements.REVLOGV1_REQUIREMENT not in repo.requirements:
msg = b"can only repair revlogv1 repositories, v2 is not affected"
raise error.Abort(_(msg))
rewrite.repair_issue6528(
ui, repo, dry_run=dry_run, to_report=to_report, from_report=from_report
)
@command(b'debugformat', [] + cmdutil.formatteropts) @command(b'debugformat', [] + cmdutil.formatteropts)
def debugformat(ui, repo, **opts): def debugformat(ui, repo, **opts):
"""display format information about the current repository """display format information about the current repository
Use --verbose to get extra information about current config value and Use --verbose to get extra information about current config value and
Mercurial default.""" Mercurial default."""
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
maxvariantlength = max(len(fv.name) for fv in upgrade.allformatvariant) maxvariantlength = max(len(fv.name) for fv in upgrade.allformatvariant)

mercurial/revlogutils/rewrite.py

# censor code related to censoring revision # censor code related to censoring revision
# coding: utf8 # coding: utf8
# #
# Copyright 2021 Pierre-Yves David <pierre-yves.david@octobus.net> # Copyright 2021 Pierre-Yves David <pierre-yves.david@octobus.net>
# Copyright 2015 Google, Inc <martinvonz@google.com> # Copyright 2015 Google, Inc <martinvonz@google.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
import binascii
import contextlib import contextlib
import os import os
from ..node import ( from ..node import (
nullrev, nullrev,
) )
from .constants import ( from .constants import (
COMP_MODE_PLAIN, COMP_MODE_PLAIN,
data_compression_mode=COMP_MODE_PLAIN, data_compression_mode=COMP_MODE_PLAIN,
sidedata_compression_mode=COMP_MODE_PLAIN, sidedata_compression_mode=COMP_MODE_PLAIN,
) )
revlog.index.append(new_entry) revlog.index.append(new_entry)
entry_bin = revlog.index.entry_binary(rev) entry_bin = revlog.index.entry_binary(rev)
new_index_file.write(entry_bin) new_index_file.write(entry_bin)
revlog._docket.index_end = new_index_file.tell() revlog._docket.index_end = new_index_file.tell()
revlog._docket.data_end = new_data_file.tell() revlog._docket.data_end = new_data_file.tell()
def _get_filename_from_filelog_index(path):
# Drop the extension and the `data/` prefix
marmouteUnsubmitted
Done

note: this seems "fine" to me for stable. However this seems a good indication that we need some store/repo level API for "give me all the filelog"

marmoute: note: this seems "fine" to me for stable. However this seems a good indication that we need…
path_part = path.rsplit(b'.', 1)[0].split(b'/', 1)
if len(path_part) < 2:
msg = _(b"cannot recognize filelog from filename: '%s'")
marmouteUnsubmitted
Done

It is probably worth adding a programming error if the repository is not locked.

marmoute: It is probably worth adding a programming error if the repository is not locked.
marmouteUnsubmitted
Done

note: this is not the right location for this comment :-/ the lock checking should be done when we actually rewrite revlog.

We do not usually needs it, but the rewrite code is "exceptionnal" enough to add some double checking.

marmoute: note: this is not the right location for this comment :-/ the lock checking should be done when…
marmouteUnsubmitted
Not Done

My two past self eventually talked to each other and we probably want is in both. I'll prepare you a patch you can fold into this one.

marmoute: My two past self eventually talked to each other and we probably want is in both. I'll prepare…
marmouteUnsubmitted
Not Done

Actually, I amd now triple confused about what I meant, so lets leave it that way,

marmoute: Actually, I amd now triple confused about what I meant, so lets leave it that way,
msg %= path
raise error.Abort(msg)
return path_part[1]
def _filelog_from_filename(repo, path):
"""Returns the filelog for the given `path`. Stolen from `engine.py`"""
from .. import filelog # avoid cycle
marmouteUnsubmitted
Done

This seems the only line that check and enforce the version.

We need:

  1. a programming error to catch the issue on Windows/Optimized setting
  2. a higher level check of the repository format in the top level command
marmoute: This seems the only line that check and enforce the version. We need: 1) a programming error…
fl = filelog.filelog(repo.svfs, path)
return fl
def _write_swapped_parents(repo, rl, rev, offset, fp):
marmouteUnsubmitted
Done

This should go in _reorder_filelog_parents (I am starting to suspect phabricator simply shifted all comments a couple of line too low)

marmoute: This should go in `_reorder_filelog_parents` (I am starting to suspect phabricator simply…
"""Swaps p1 and p2 and overwrites the revlog entry for `rev` in `fp`"""
from ..pure import parsers # avoid cycle
if repo._currentlock(repo._lockref) is None:
# Let's be paranoid about it
msg = "repo needs to be locked to rewrite parents"
raise error.ProgrammingError(msg)
index_format = parsers.IndexObject.index_format
entry = rl.index[rev]
new_entry = list(entry)
new_entry[5], new_entry[6] = entry[6], entry[5]
packed = index_format.pack(*new_entry[:8])
fp.seek(offset)
marmouteUnsubmitted
Done

If there is a crash/interrupt during the operation, we will leave temporary file behind. We should add a try:, finally: clause here, and possibly a no-interrupt context.

marmoute: If there is a crash/interrupt during the operation, we will leave temporary file behind. We…
fp.write(packed)
def _reorder_filelog_parents(repo, fl, to_fix):
"""
Swaps p1 and p2 for all `to_fix` revisions of filelog `fl` and writes the
new version to disk, overwriting the old one with a rename.
"""
from ..pure import parsers # avoid cycle
ui = repo.ui
assert len(to_fix) > 0
rl = fl._revlog
if rl._format_version != constants.REVLOGV1:
marmouteUnsubmitted
Done

nits: this is the sensitive part, it is probably worth factoring it out in a small dedicated function since most of it is common, but for the offset computation.

What do you think ?

I addition it seems like a good idea to mention the "speedup" idea from Joerg (about only checking the delta when possible)

marmoute: nits: this is the sensitive part, it is probably worth factoring it out in a small dedicated…
msg = "expected version 1 revlog, got version '%d'" % rl._format_version
raise error.ProgrammingError(msg)
index_file = rl._indexfile
new_file_path = index_file + b'.tmp-parents-fix'
repaired_msg = _(b"repaired revision %d of 'filelog %s'\n")
with ui.uninterruptible():
marmouteUnsubmitted
Done

We need a nointerrupt context here, don't we ?

marmoute: We need a nointerrupt context here, don't we ?
try:
util.copyfile(
rl.opener.join(index_file),
rl.opener.join(new_file_path),
checkambig=rl._checkambig,
)
with rl.opener(new_file_path, mode=b"r+") as fp:
if rl._inline:
marmouteUnsubmitted
Done

Since this is a sensitive issue, we should probably add a docstring about the detection method here.

marmoute: Since this is a sensitive issue, we should probably add a docstring about the detection method…
index = parsers.InlinedIndexObject(fp.read())
for rev in fl.revs():
if rev in to_fix:
offset = index._calculate_index(rev)
_write_swapped_parents(repo, rl, rev, offset, fp)
ui.write(repaired_msg % (rev, index_file))
else:
index_format = parsers.IndexObject.index_format
for rev in to_fix:
offset = rev * index_format.size
_write_swapped_parents(repo, rl, rev, offset, fp)
ui.write(repaired_msg % (rev, index_file))
rl.opener.rename(new_file_path, index_file)
rl.clearcaches()
rl._loadindex()
finally:
util.tryunlink(new_file_path)
marmouteUnsubmitted
Done

Not sure what "unfortunate cyclic sitatuation" means here ‽

marmoute: Not sure what "unfortunate cyclic sitatuation" means here ‽
def _is_revision_affected(ui, fl, filerev, path):
"""Mercurial currently (5.9rc0) uses `p1 == nullrev and p2 != nullrev` as a
special meaning compared to the reverse in the context of filelog-based
copytracing. issue6528 exists because new code assumed that parent ordering
didn't matter, so this detects if the revision contains metadata (since
it's only used for filelog-based copytracing) and its parents are in the
"wrong" order."""
try:
raw_text = fl.rawdata(filerev)
except error.CensoredNodeError:
# We don't care about censored nodes as they never carry metadata
return False
has_meta = raw_text.startswith(b'\x01\n')
if has_meta:
(p1, p2) = fl.parentrevs(filerev)
if p1 != nullrev and p2 == nullrev:
return True
marmouteUnsubmitted
Done

It would seem cleaner to have this function only determine the status of the revision and have the warning message only issued by the called of this method.

The "corrupted" is a bit scary too. maybe "affected by issue6528"

This will be useful if we want to detect this in more situation (e.g. during exchange)

marmoute: It would seem cleaner to have this function only determine the status of the revision and have…
return False
def _from_report(ui, repo, context, from_report, dry_run):
marmouteUnsubmitted
Done

I don't think we capitalize error output in general.

marmoute: I don't think we capitalize error output in general.
"""
Fix the revisions given in the `from_report` file, but still checks if the
marmouteUnsubmitted
Done

tiny nits: this should probably be a msg %= on the previous line

marmoute: tiny nits: this should probably be a `msg %= on the previous line`
revisions are indeed affected to prevent an unfortunate cyclic situation
where we'd swap well-ordered parents again.
See the doc for `debug_fix_issue6528` for the format documentation.
"""
ui.write(_(b"loading report file '%s'\n") % from_report)
with context(), open(from_report, mode='rb') as f:
for line in f.read().split(b'\n'):
if not line:
marmouteUnsubmitted
Done

You should probably sort the resulting to_fix set to make sure we process things in a predictable order (and start to end seems like a good idea in general)

marmoute: You should probably sort the resulting `to_fix` set to make sure we process things in a…
continue
filenodes, filename = line.split(b' ', 1)
fl = _filelog_from_filename(repo, filename)
to_fix = set(
fl.rev(binascii.unhexlify(n)) for n in filenodes.split(b',')
)
excluded = set()
for filerev in to_fix:
if _is_revision_affected(ui, fl, filerev, filename):
msg = b"found affected revision %d for filelog '%s'\n"
marmouteUnsubmitted
Done

adding uninterruptible for the full command seems quite overkill, especially since the command can apparently run for half an hours.

See my earlier comment about only using ui.uninterruptible() while doing the actual fixing.

marmoute: adding uninterruptible for the full command seems quite overkill, especially since the command…
ui.warn(msg % (filerev, filename))
else:
msg = _(b"revision %s of file '%s' is not affected\n")
msg %= (binascii.hexlify(fl.node(filerev)), filename)
ui.warn(msg)
excluded.add(filerev)
to_fix = to_fix - excluded
if not to_fix:
msg = _(b"no affected revisions were found for '%s'\n")
marmouteUnsubmitted
Not Done

This is probably a bit too verbose for the common case. I'll prepare something to control that.

marmoute: This is probably a bit too verbose for the common case. I'll prepare something to control that.
marmouteUnsubmitted
Not Done

Actually, this is only issued when processing report, so this seems fine.

marmoute: Actually, this is only issued when processing report, so this seems fine.
ui.write(msg % filename)
continue
if not dry_run:
marmouteUnsubmitted
Done

Here it is. It still feel like too wide and in the wrong place:

  • we want it as narrow as possible since it is "a bit hacky"
  • we want it in the function that actually setup the dangerous bit so that it (1) is always in place (2) stay next to the dangerous implementation.
marmoute: Here it is. It still feel like too wide and in the wrong place: - we want it as narrow as…
_reorder_filelog_parents(repo, fl, sorted(to_fix))
def repair_issue6528(ui, repo, dry_run=False, to_report=None, from_report=None):
from .. import store # avoid cycle
@contextlib.contextmanager
def context():
if dry_run or to_report: # No need for locking
yield
else:
with repo.wlock(), repo.lock():
yield
if from_report:
return _from_report(ui, repo, context, from_report, dry_run)
report_entries = []
with context():
files = list(
(file_type, path)
for (file_type, path, _e, _s) in repo.store.datafiles()
if path.endswith(b'.i') and file_type & store.FILEFLAGS_FILELOG
marmouteUnsubmitted
Done

Looks like you want to:

  1. gather the list of filelog
  2. use its len(xxx) for total
  3. use that list for the iteration
marmoute: Looks like you want to: 1) gather the list of filelog 2) use its `len(xxx)` for total 3) use…
AlphareAuthorUnsubmitted
Done

I guess the memory footprint wouldn't get too bad even on larger repos.

Alphare: I guess the memory footprint wouldn't get too bad even on larger repos.
marmouteUnsubmitted
Not Done

Yeah, I dont expect thousand of these, and if so, the associated repository probably have large machine using it. (and worst case the report file can be feed in multiple step).

marmoute: Yeah, I dont expect thousand of these, and if so, the associated repository probably have large…
)
progress = ui.makeprogress(
_(b"looking for affected revisions"),
unit=_(b"filelogs"),
total=len(files),
)
found_nothing = True
marmouteUnsubmitted
Done

small nits: if to_fix: seem more pythonic to me

marmoute: small nits: `if to_fix:` seem more pythonic to me
for file_type, path in files:
if (
not path.endswith(b'.i')
or not file_type & store.FILEFLAGS_FILELOG
marmouteUnsubmitted
Done

We should sort to_fix to ensure growing order.

marmoute: We should sort `to_fix` to ensure growing order.
):
continue
progress.increment()
marmouteUnsubmitted
Done

I don't think we capitalize output.

marmoute: I don't think we capitalize output.
filename = _get_filename_from_filelog_index(path)
fl = _filelog_from_filename(repo, filename)
# Set of filerevs (or hex filenodes if `to_report`) that need fixing
to_fix = set()
for filerev in fl.revs():
# TODO speed up by looking at the start of the delta
# If it hasn't changed, it's not worth looking at the other revs
# in the same chain
affected = _is_revision_affected(ui, fl, filerev, path)
if affected:
msg = b"found affected revision %d for filelog '%s'\n"
ui.warn(msg % (filerev, path))
found_nothing = False
if not dry_run:
if to_report:
to_fix.add(binascii.hexlify(fl.node(filerev)))
else:
to_fix.add(filerev)
if to_fix:
to_fix = sorted(to_fix)
if to_report:
report_entries.append((filename, to_fix))
else:
_reorder_filelog_parents(repo, fl, to_fix)
if found_nothing:
ui.write(_(b"no affected revisions were found\n"))
if to_report and report_entries:
with open(to_report, mode="wb") as f:
for path, to_fix in report_entries:
f.write(b"%s %s\n" % (b",".join(to_fix), path))
progress.complete()

tests/bundles/issue6528.tar

  • This binary file was added.

tests/test-completion.t

archive archive
Do not show debug commands if there are other candidates Do not show debug commands if there are other candidates
$ hg debugcomplete d $ hg debugcomplete d
diff diff
Show debug commands if there are no other candidates Show debug commands if there are no other candidates
$ hg debugcomplete debug $ hg debugcomplete debug
debug-repair-issue6528
debugancestor debugancestor
debugantivirusrunning debugantivirusrunning
debugapplystreamclonebundle debugapplystreamclonebundle
debugbackupbundle debugbackupbundle
debugbuilddag debugbuilddag
debugbundle debugbundle
debugcapabilities debugcapabilities
debugchangedfiles debugchangedfiles
branches: active, closed, rev, template branches: active, closed, rev, template
bundle: force, rev, branch, base, all, type, ssh, remotecmd, insecure bundle: force, rev, branch, base, all, type, ssh, remotecmd, insecure
cat: output, rev, decode, include, exclude, template cat: output, rev, decode, include, exclude, template
clone: noupdate, updaterev, rev, branch, pull, uncompressed, stream, ssh, remotecmd, insecure clone: noupdate, updaterev, rev, branch, pull, uncompressed, stream, ssh, remotecmd, insecure
commit: addremove, close-branch, amend, secret, edit, force-close-branch, interactive, include, exclude, message, logfile, date, user, subrepos commit: addremove, close-branch, amend, secret, edit, force-close-branch, interactive, include, exclude, message, logfile, date, user, subrepos
config: untrusted, exp-all-known, edit, local, source, shared, non-shared, global, template config: untrusted, exp-all-known, edit, local, source, shared, non-shared, global, template
continue: dry-run continue: dry-run
copy: forget, after, at-rev, force, include, exclude, dry-run copy: forget, after, at-rev, force, include, exclude, dry-run
debug-repair-issue6528: to-report, from-report, dry-run
debugancestor: debugancestor:
debugantivirusrunning: debugantivirusrunning:
debugapplystreamclonebundle: debugapplystreamclonebundle:
debugbackupbundle: recover, patch, git, limit, no-merges, stat, graph, style, template debugbackupbundle: recover, patch, git, limit, no-merges, stat, graph, style, template
debugbuilddag: mergeable-file, overwritten-file, new-file debugbuilddag: mergeable-file, overwritten-file, new-file
debugbundle: all, part-type, spec debugbundle: all, part-type, spec
debugcapabilities: debugcapabilities:
debugchangedfiles: compute debugchangedfiles: compute

tests/test-help.t

#endif #endif
Test list of internal help commands Test list of internal help commands
$ hg help debug $ hg help debug
debug commands (internal and unsupported): debug commands (internal and unsupported):
debug-repair-issue6528
find affected revisions and repair them. See issue6528 for more
details.
debugancestor debugancestor
find the ancestor revision of two revisions in a given index find the ancestor revision of two revisions in a given index
debugantivirusrunning debugantivirusrunning
attempt to trigger an antivirus scanner to see if one is active attempt to trigger an antivirus scanner to see if one is active
debugapplystreamclonebundle debugapplystreamclonebundle
apply a stream clone bundle file apply a stream clone bundle file
debugbackupbundle debugbackupbundle
lists the changesets available in backup bundles lists the changesets available in backup bundles

tests/test-issue6528.t

=============================================================== ===============================================================
Test non-regression on the corruption associated with issue6528 Test non-regression on the corruption associated with issue6528
=============================================================== ===============================================================
Setup Setup
----- =====
$ hg init base-repo $ hg init base-repo
$ cd base-repo $ cd base-repo
$ cat <<EOF > a.txt $ cat <<EOF > a.txt
> 1 > 1
> 2 > 2
> 3 > 3
3 3
4 4
5 5
bar bar
$ cd .. $ cd ..
Check the lack of corruption Check the lack of corruption
---------------------------- ============================
$ hg clone --pull base-repo cloned $ hg clone --pull base-repo cloned
requesting all changes requesting all changes
adding changesets adding changesets
adding manifests adding manifests
adding file changes adding file changes
added 4 changesets with 4 changes to 2 files added 4 changesets with 4 changes to 2 files
new changesets f5a5a568022f:a1cc2bdca0aa new changesets f5a5a568022f:a1cc2bdca0aa
| date: Thu Jan 01 00:00:00 1970 +0000 | date: Thu Jan 01 00:00:00 1970 +0000
| summary: c_modify_c - modify a.txt | summary: c_modify_c - modify a.txt
| |
o changeset: 0:f5a5a568022f o changeset: 0:f5a5a568022f
user: test user: test
date: Thu Jan 01 00:00:00 1970 +0000 date: Thu Jan 01 00:00:00 1970 +0000
summary: c_base_c - create a.txt summary: c_base_c - create a.txt
Test the command that fixes the issue
=====================================
marmouteUnsubmitted
Done

We should make this is ===== level title and use ----- level title (and associated description) for the individual tests within this section.

marmoute: We should make this is `=====` level title and use `-----` level title (and associated…
Restore a broken repository with multiple broken revisions and a filename that
would get encoded to test the `report` options.
It's a tarball because unbundle might magically fix the issue later.
$ cd ..
$ mkdir repo-to-fix
$ cd repo-to-fix
#if windows
tar interprets `:` in paths (like `C:`) as being remote, force local on Windows
only since some versions of tar don't have this flag.
marmouteUnsubmitted
Done

We should add debug index call here to show they are swapped

marmoute: We should add debug index call here to show they are swapped
$ tar --force-local -xf $TESTDIR/bundles/issue6528.tar
#else
$ tar xf $TESTDIR/bundles/issue6528.tar
#endif
Check that the issue is present
$ hg st
M D.txt
M b.txt
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 05b806ebe5ea 000000000000
2 6 216a5fe8b8ed 000000000000 000000000000
marmouteUnsubmitted
Done

small nits: the output says we "found" the corruption, but does not state that we fixed it.

We should also check the index content after the command run.

marmoute: small nits: the output says we "found" the corruption, but does not state that we fixed it. We…
3 7 ea4f2f2463cc 216a5fe8b8ed 000000000000
$ hg debugrevlogindex D.txt
rev linkrev nodeid p1 p2
0 6 2a8d3833f2fb 000000000000 000000000000
1 7 2a80419dfc31 2a8d3833f2fb 000000000000
Dry-run the fix
$ hg debug-repair-issue6528 --dry-run
found affected revision 1 for filelog 'data/D.txt.i'
found affected revision 1 for filelog 'data/b.txt.i'
found affected revision 3 for filelog 'data/b.txt.i'
$ hg st
M D.txt
M b.txt
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 05b806ebe5ea 000000000000
2 6 216a5fe8b8ed 000000000000 000000000000
3 7 ea4f2f2463cc 216a5fe8b8ed 000000000000
$ hg debugrevlogindex D.txt
rev linkrev nodeid p1 p2
0 6 2a8d3833f2fb 000000000000 000000000000
marmouteUnsubmitted
Done

Testing a multi-lines report seems important too.

marmoute: Testing a multi-lines report seems important too.
AlphareAuthorUnsubmitted
Done

I've created a multiple revisions + multiple files repo

Alphare: I've created a multiple revisions + multiple files repo
1 7 2a80419dfc31 2a8d3833f2fb 000000000000
Run the fix
$ hg debug-repair-issue6528
found affected revision 1 for filelog 'data/D.txt.i'
repaired revision 1 of 'filelog data/D.txt.i'
found affected revision 1 for filelog 'data/b.txt.i'
found affected revision 3 for filelog 'data/b.txt.i'
repaired revision 1 of 'filelog data/b.txt.i'
repaired revision 3 of 'filelog data/b.txt.i'
Check that the fix worked and that running it twice does nothing
$ hg st
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 000000000000 05b806ebe5ea
2 6 216a5fe8b8ed 000000000000 000000000000
3 7 ea4f2f2463cc 000000000000 216a5fe8b8ed
$ hg debugrevlogindex D.txt
rev linkrev nodeid p1 p2
0 6 2a8d3833f2fb 000000000000 000000000000
1 7 2a80419dfc31 000000000000 2a8d3833f2fb
$ hg debug-repair-issue6528
no affected revisions were found
$ hg st
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 000000000000 05b806ebe5ea
2 6 216a5fe8b8ed 000000000000 000000000000
3 7 ea4f2f2463cc 000000000000 216a5fe8b8ed
$ hg debugrevlogindex D.txt
rev linkrev nodeid p1 p2
0 6 2a8d3833f2fb 000000000000 000000000000
1 7 2a80419dfc31 000000000000 2a8d3833f2fb
Try the using the report options
--------------------------------
$ cd ..
$ mkdir repo-to-fix-report
$ cd repo-to-fix
#if windows
tar interprets `:` in paths (like `C:`) as being remote, force local on Windows
only since some versions of tar don't have this flag.
$ tar --force-local -xf $TESTDIR/bundles/issue6528.tar
#else
$ tar xf $TESTDIR/bundles/issue6528.tar
#endif
$ hg debug-repair-issue6528 --to-report $TESTTMP/report.txt
found affected revision 1 for filelog 'data/D.txt.i'
found affected revision 1 for filelog 'data/b.txt.i'
found affected revision 3 for filelog 'data/b.txt.i'
$ cat $TESTTMP/report.txt
2a80419dfc31d7dfb308ac40f3f138282de7d73b D.txt
a58b36ad6b6545195952793099613c2116f3563b,ea4f2f2463cca5b29ddf3461012b8ce5c6dac175 b.txt
$ hg debug-repair-issue6528 --from-report $TESTTMP/report.txt --dry-run
loading report file '$TESTTMP/report.txt'
found affected revision 1 for filelog 'D.txt'
found affected revision 1 for filelog 'b.txt'
found affected revision 3 for filelog 'b.txt'
$ hg st
M D.txt
M b.txt
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 05b806ebe5ea 000000000000
2 6 216a5fe8b8ed 000000000000 000000000000
3 7 ea4f2f2463cc 216a5fe8b8ed 000000000000
$ hg debugrevlogindex D.txt
rev linkrev nodeid p1 p2
0 6 2a8d3833f2fb 000000000000 000000000000
1 7 2a80419dfc31 2a8d3833f2fb 000000000000
$ hg debug-repair-issue6528 --from-report $TESTTMP/report.txt
loading report file '$TESTTMP/report.txt'
found affected revision 1 for filelog 'D.txt'
repaired revision 1 of 'filelog data/D.txt.i'
found affected revision 1 for filelog 'b.txt'
found affected revision 3 for filelog 'b.txt'
repaired revision 1 of 'filelog data/b.txt.i'
repaired revision 3 of 'filelog data/b.txt.i'
$ hg st
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 000000000000 05b806ebe5ea
2 6 216a5fe8b8ed 000000000000 000000000000
3 7 ea4f2f2463cc 000000000000 216a5fe8b8ed
$ hg debugrevlogindex D.txt
rev linkrev nodeid p1 p2
0 6 2a8d3833f2fb 000000000000 000000000000
1 7 2a80419dfc31 000000000000 2a8d3833f2fb
Check that the revision is not "fixed" again
$ hg debug-repair-issue6528 --from-report $TESTTMP/report.txt
loading report file '$TESTTMP/report.txt'
revision 2a80419dfc31d7dfb308ac40f3f138282de7d73b of file 'D.txt' is not affected
no affected revisions were found for 'D.txt'
revision a58b36ad6b6545195952793099613c2116f3563b of file 'b.txt' is not affected
revision ea4f2f2463cca5b29ddf3461012b8ce5c6dac175 of file 'b.txt' is not affected
no affected revisions were found for 'b.txt'
$ hg st
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 000000000000 05b806ebe5ea
2 6 216a5fe8b8ed 000000000000 000000000000
3 7 ea4f2f2463cc 000000000000 216a5fe8b8ed
$ hg debugrevlogindex D.txt
rev linkrev nodeid p1 p2
0 6 2a8d3833f2fb 000000000000 000000000000
1 7 2a80419dfc31 000000000000 2a8d3833f2fb
Try it with a non-inline revlog
-------------------------------
$ cd ..
$ mkdir $TESTTMP/ext
$ cat << EOF > $TESTTMP/ext/small_inline.py
> from mercurial import revlog
> revlog._maxinline = 8
> EOF
$ cat << EOF >> $HGRCPATH
> [extensions]
> small_inline=$TESTTMP/ext/small_inline.py
> EOF
$ mkdir repo-to-fix-not-inline
$ cd repo-to-fix-not-inline
#if windows
tar interprets `:` in paths (like `C:`) as being remote, force local on Windows
only since some versions of tar don't have this flag.
$ tar --force-local -xf $TESTDIR/bundles/issue6528.tar
#else
$ tar xf $TESTDIR/bundles/issue6528.tar
#endif
$ echo b >> b.txt
$ hg commit -qm "inline -> separate"
$ find .hg -name *b.txt.d
.hg/store/data/b.txt.d
Status is correct, but the problem is still there, in the earlier revision
$ hg st
$ hg up 3
1 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ hg st
M b.txt
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 05b806ebe5ea 000000000000
2 6 216a5fe8b8ed 000000000000 000000000000
3 7 ea4f2f2463cc 216a5fe8b8ed 000000000000
4 8 db234885e2fe ea4f2f2463cc 000000000000
$ hg debugrevlogindex D.txt
rev linkrev nodeid p1 p2
0 6 2a8d3833f2fb 000000000000 000000000000
1 7 2a80419dfc31 2a8d3833f2fb 000000000000
2 8 65aecc89bb5d 2a80419dfc31 000000000000
Run the fix on the non-inline revlog
$ hg debug-repair-issue6528
found affected revision 1 for filelog 'data/D.txt.i'
repaired revision 1 of 'filelog data/D.txt.i'
found affected revision 1 for filelog 'data/b.txt.i'
found affected revision 3 for filelog 'data/b.txt.i'
repaired revision 1 of 'filelog data/b.txt.i'
repaired revision 3 of 'filelog data/b.txt.i'
Check that it worked
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 000000000000 05b806ebe5ea
2 6 216a5fe8b8ed 000000000000 000000000000
3 7 ea4f2f2463cc 000000000000 216a5fe8b8ed
4 8 db234885e2fe ea4f2f2463cc 000000000000
$ hg debugrevlogindex D.txt
rev linkrev nodeid p1 p2
0 6 2a8d3833f2fb 000000000000 000000000000
1 7 2a80419dfc31 000000000000 2a8d3833f2fb
2 8 65aecc89bb5d 2a80419dfc31 000000000000
$ hg debug-repair-issue6528
no affected revisions were found
$ hg st