( )⚙ D11239 debugcommands: introduce a debug command to repair repos affected by issue6528

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
Branch
stable
Lint
No Linters Available
Unit
No Unit Test Coverage

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
1456

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

1474

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.

1481

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).

1483

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

1492

Why do we need the bool here ?

1496

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
1481

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
1481

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 29770

mercurial/debugcommands.py

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-fix-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 corrupted revisions to this file'),
_(b'FILE'),
),
(
b'',
b'from-report',
b'',
_(b'fix 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_fix_issue6528(ui, repo, **opts):
"""find corrupted revisions and fix them. See issue6528 for more details.
The `--to-report` and `--from-report` flags allow you to cache and reuse the
computation of corrupted revisions for a given repository across clones.
The report format is line-based (with empty lines ignored):
<encoded filelog index file path> <ascii-hex of the corrupted revision>...
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…
Though unlikely, there can be multiple broken revisions per filelog.
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)
Note that this does *not* mean that this fixes future corrupted revisions, that
needs a separate fix at the exchange level that hasn't been written yet (as of
5.9rc0).
"""
cmdutil.check_incompatible_arguments(
opts, 'to_report', ['from_report', 'dry_run']
)
dry_run = bool(opts.get('dry_run'))
marmouteUnsubmitted
Done

Why do we need the bool here ?

marmoute: Why do we need the bool here ?
to_report = opts.get('to_report')
from_report = opts.get('from_report')
rewrite.fix_issue6528(
marmouteUnsubmitted
Done

Same feedback about fix → repair

marmoute: Same feedback about `fix` → `repair`
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 _filelog_from_path(repo, path):
"""Returns the filelog for the given `path`. Stolen from `engine.py`"""
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…
from .. import filelog # avoid cycle
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,
# Drop the extension and the `data/` prefix
path_part = path.rsplit(b'.', 1)[0].split(b'/', 1)
if len(path_part) < 2:
msg = _(b"cannot recognize filelog from filename: '%s'")
msg %= path
raise error.Abort(msg)
path = path_part[1]
fl = filelog.filelog(repo.svfs, path)
rl = fl._revlog
assert rl._format_version == constants.REVLOGV1, rl._format_version
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…
return fl
def _reorder_filelog_parents(fl, to_fix):
"""
Swaps p1 and p2 for all `to_fix` revisions of filelog `fl` and writes the new
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…
version to disk, overwriting the old one with a rename.
"""
from ..pure import parsers
assert len(to_fix) > 0
rl = fl._revlog
index_format = parsers.IndexObject.index_format
new_file_path = rl._indexfile + b'tmp-parents-fix'
util.copyfile(
rl.opener.join(rl._indexfile),
rl.opener.join(new_file_path),
checkambig=rl._checkambig,
)
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…
with rl.opener(new_file_path, mode=b"r+") as fp:
if rl._inline:
index = parsers.InlinedIndexObject(fp.read())
for rev in fl.revs():
entry = rl.index[rev]
new_entry = list(entry)
if rev in to_fix:
# Swap p1 and p2
new_entry[5], new_entry[6] = entry[6], entry[5]
packed = index_format.pack(*new_entry[:8])
offset = index._calculate_index(rev)
fp.seek(offset)
fp.write(packed)
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…
else:
for rev in to_fix:
entry = rl.index[rev]
new_entry = list(entry)
# Swap p1 and p2
new_entry[5], new_entry[6] = entry[6], entry[5]
packed = index_format.pack(*new_entry[:8])
offset = rev * index_format.size
marmouteUnsubmitted
Done

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

marmoute: We need a nointerrupt context here, don't we ?
fp.seek(offset)
fp.write(packed)
rl.opener.rename(new_file_path, rl._indexfile)
rl.clearcaches()
rl._loadindex()
def _is_revision_corrupted(ui, fl, filerev, path):
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…
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:
msg = b"found corrupted revision %d for filelog '%s'\n"
ui.warn(msg % (filerev, path))
return True
return False
def _from_report(ui, repo, context, from_report, dry_run):
"""
Fix the revisions given in the `from_report` file, but still checks if the
revisions are indeed corrupted to prevent an unfortunate cyclic situation.
marmouteUnsubmitted
Done

Not sure what "unfortunate cyclic sitatuation" means here ‽

marmoute: Not sure what "unfortunate cyclic sitatuation" means here ‽
See the doc for `debug_fix_issue6528` for the format documentation.
"""
from .. import store # prevent cycle
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:
continue
encoded, filenodes = line.split(b' ', 1)
filename = store.decodefilename(encoded)
fl = _filelog_from_path(repo, filename)
to_fix = set(
fl.rev(binascii.unhexlify(n)) for n in filenodes.split()
)
excluded = set()
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…
for filerev in to_fix:
if not _is_revision_corrupted(ui, fl, filerev, filename):
msg = _(b"Revision %s of file '%s' is not corrupted\n")
marmouteUnsubmitted
Done

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

marmoute: I don't think we capitalize error output in general.
ui.warn(
msg % (binascii.hexlify(fl.node(filerev)), filename)
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`
)
excluded.add(filerev)
to_fix = to_fix - excluded
if len(to_fix) == 0:
msg = _(b"No corrupted revisions were found for '%s'\n")
ui.write(msg % filename)
continue
if not dry_run:
_reorder_filelog_parents(fl, to_fix)
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…
def fix_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(), ui.uninterruptible():
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…
yield
if from_report:
return _from_report(ui, repo, context, from_report, dry_run)
report_entries = []
with context():
total = sum(
1
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.
for (t, p, _e, _s) in repo.store.datafiles()
if p.endswith(b'.i') and t & store.FILEFLAGS_FILELOG
)
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…
progress = ui.makeprogress(
_(b"Looking for corrupted revisions"),
unit=_(b"filelogs"),
total=total,
)
found_nothing = True
for file_type, path, encoded, _size in repo.store.datafiles():
if (
not path.endswith(b'.i')
or not file_type & store.FILEFLAGS_FILELOG
):
continue
progress.increment()
fl = _filelog_from_path(repo, path)
# 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
corrupted = _is_revision_corrupted(ui, fl, filerev, path)
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…
if corrupted:
found_nothing = False
if not dry_run:
if to_report:
to_fix.add(binascii.hexlify(fl.node(filerev)))
else:
to_fix.add(filerev)
if len(to_fix) > 0:
marmouteUnsubmitted
Done

small nits: if to_fix: seem more pythonic to me

marmoute: small nits: `if to_fix:` seem more pythonic to me
if to_report:
report_entries.append((encoded, to_fix))
else:
_reorder_filelog_parents(fl, to_fix)
marmouteUnsubmitted
Done

We should sort to_fix to ensure growing order.

marmoute: We should sort `to_fix` to ensure growing order.
if found_nothing:
ui.write(_(b"No corrupted revisions were found\n"))
marmouteUnsubmitted
Done

I don't think we capitalize output.

marmoute: I don't think we capitalize output.
if to_report and len(report_entries) > 0:
with open(to_report, mode="wb") as f:
for path, to_fix in report_entries:
f.write(b"%s %s\n" % (path, b" ".join(to_fix)))
progress.complete()

tests/bundles/issue6528.tar

  • This binary file was added.
PropertyOld ValueNew Value
MIME Typenullapplication/x-tar
File Sizenull110 KB

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-fix-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-fix-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-fix-issue6528
find corrupted revisions and fix 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

| 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
It's a tarball because unbundle might magically fix the issue later
$ cd ..
$ mkdir repo-to-fix
$ cd repo-to-fix
$ tar xf $TESTDIR/bundles/issue6528.tar
Check that the issue is present
$ hg st
M b.txt
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
Dry-run the fix
$ hg debug-fix-issue6528 --dry-run
found corrupted revision 1 for filelog 'data/b.txt.i'
$ hg st
M b.txt
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 05b806ebe5ea 000000000000
Run the fix
$ hg debug-fix-issue6528
found corrupted revision 1 for filelog 'data/b.txt.i'
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…
Check that the fix worked and that running it twice does nothing
$ hg st
$ hg debug-fix-issue6528
No corrupted revisions were found
$ hg st
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 000000000000 05b806ebe5ea
Try the using the report options
--------------------------------
$ cd ..
$ mkdir repo-to-fix-report
$ cd repo-to-fix
$ tar xf $TESTDIR/bundles/issue6528.tar
$ hg debug-fix-issue6528 --to-report $TESTTMP/report.txt
found corrupted revision 1 for filelog 'data/b.txt.i'
$ cat $TESTTMP/report.txt
data/b.txt.i a58b36ad6b6545195952793099613c2116f3563b
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
$ hg debug-fix-issue6528 --from-report $TESTTMP/report.txt --dry-run
Loading report file '$TESTTMP/report.txt'
found corrupted revision 1 for filelog 'data/b.txt.i'
$ hg st
M b.txt
$ hg debugrevlogindex b.txt
rev linkrev nodeid p1 p2
0 2 05b806ebe5ea 000000000000 000000000000
1 3 a58b36ad6b65 05b806ebe5ea 000000000000
$ hg debug-fix-issue6528 --from-report $TESTTMP/report.txt
Loading report file '$TESTTMP/report.txt'
found corrupted revision 1 for 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
Check that the revision is not "fixed" again
$ hg debug-fix-issue6528 --from-report $TESTTMP/report.txt
Loading report file '$TESTTMP/report.txt'
Revision a58b36ad6b6545195952793099613c2116f3563b of file 'data/b.txt.i' is not corrupted
No corrupted revisions were found for '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
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
$ tar xf $TESTDIR/bundles/issue6528.tar
$ 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, 0 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 4 468d0263ae92 a58b36ad6b65 000000000000
Run the fix on the non-inline revlog
$ hg debug-fix-issue6528
found corrupted revision 1 for 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 4 468d0263ae92 a58b36ad6b65 000000000000
$ hg debug-fix-issue6528
No corrupted revisions were found
$ hg st