[Updated] D11239: debugcommands: introduce a debug command to fix repos affected by issue6528

Mon Aug 2 19:04:20 UTC 2021

This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.

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

INLINE COMMENTS

> debugcommands.py:1456
> + at command(
> +    b"debug-fix-issue6528",
> +    [

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

> debugcommands.py:1474
> +    + cmdutil.dryrunopts,
> +)
> +def debug_fix_issue6528(ui, repo, **opts):

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.

> debugcommands.py:1481
> +    The report format is line-based (with empty lines ignored):
> +        <encoded filelog index file path> <ascii-hex of the corrupted revision>...
> +

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

> debugcommands.py:1483
> +
> +    Though unlikely, there can be multiple broken revisions per filelog.
> +

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

> debugcommands.py:1492
> +    )
> +    dry_run = bool(opts.get('dry_run'))
> +    to_report = opts.get('to_report')

Why do we need the bool here ?

> debugcommands.py:1496
> +
> +    rewrite.fix_issue6528(
> +        ui, repo, dry_run=dry_run, to_report=to_report, from_report=from_report

Same feedback about `fix` → `repair`

> rewrite.py:479
> +def _filelog_from_path(repo, path):
> +    """Returns the filelog for the given `path`. Stolen from `engine.py`"""
> +

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"

> rewrite.py:482
> +    from .. import filelog  # avoid cycle
> +
> +    # Drop the extension and the `data/` prefix

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

> rewrite.py:492
> +    rl = fl._revlog
> +    assert rl._format_version == constants.REVLOGV1, rl._format_version
> +    return fl

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

> rewrite.py:508-512
> +    util.copyfile(
> +        rl.opener.join(rl._indexfile),
> +        rl.opener.join(new_file_path),
> +        checkambig=rl._checkambig,
> +    )

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.

> rewrite.py:522-526
> +                    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)

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)

> rewrite.py:543
> +
> +def _is_revision_corrupted(ui, fl, filerev, path):
> +    try:

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

> rewrite.py:562
> +    Fix the revisions given in the `from_report` file, but still checks if the
> +    revisions are indeed corrupted to prevent an unfortunate cyclic situation.
> +

Not sure what "unfortunate cyclic sitatuation" means here ‽

> rewrite.py:584
> +                if not _is_revision_corrupted(ui, fl, filerev, filename):
> +                    msg = _(b"Revision %s of file '%s' is not corrupted\n")
> +                    ui.warn(

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

> rewrite.py:586
> +                    ui.warn(
> +                        msg % (binascii.hexlify(fl.node(filerev)), filename)
> +                    )

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

> rewrite.py:596
> +            if not dry_run:
> +                _reorder_filelog_parents(fl, to_fix)
> +

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)

> rewrite.py:607
> +        else:
> +            with repo.wlock(), repo.lock(), ui.uninterruptible():
> +                yield

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.

> rewrite.py:653
> +
> +            if len(to_fix) > 0:
> +                if to_report:

small nits: `if to_fix:` seem more pythonic to me

> rewrite.py:654-657
> +                if to_report:
> +                    report_entries.append((encoded, to_fix))
> +                else:
> +                    _reorder_filelog_parents(fl, to_fix)

We should sort `to_fix` to ensure growing order.

> rewrite.py:660
> +        if found_nothing:
> +            ui.write(_(b"No corrupted revisions were found\n"))
> +

I don't think we capitalize output.

> test-issue6528.t:170-171
> +
> +Test the command that fixes the issue
> +-------------------------------------
> +

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

> test-issue6528.t:181-183
> +Check that the issue is present
> +  $ hg st
> +  M b.txt

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

> test-issue6528.t:196-197
> +Run the fix
> +  $ hg debug-fix-issue6528
> +  found corrupted revision 1 for filelog 'data/b.txt.i'
> +

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.

> test-issue6528.t:219-220
> +  found corrupted revision 1 for filelog 'data/b.txt.i'
> +  $ cat $TESTTMP/report.txt
> +  data/b.txt.i a58b36ad6b6545195952793099613c2116f3563b
> +

Testing a multi-lines report seems important too.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D11239/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D11239

To: Alphare, #hg-reviewers, marmoute
Cc: marmoute, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20210802/3706c854/attachment-0002.html>