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

[marmoute (Pierre-Yves David)]

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


  Added a handful of new comments.

INLINE COMMENTS

> Alphare wrote in 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.

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 wrote in rewrite.py:482
> It is probably worth adding a programming error if the repository is not locked.

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.

> rewrite.py:496-498
> +    if rl._format_version != constants.REVLOGV1:
> +        msg = "expected version 1 revlog, got version '%d'" % rl._format_version
> +        raise error.ProgrammingError(msg)

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

> rewrite.py:534
> +    repaired_msg = _(b"repaired revision %d of 'filelog %s'\n")
> +    try:
> +        util.copyfile(

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

> rewrite.py:579-580
> +        if p1 != nullrev and p2 == nullrev:
> +            msg = b"found corrupted revision %d for filelog '%s'\n"
> +            ui.warn(msg % (filerev, path))
> +            return True

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)

> rewrite.py:619-620
> +            if not dry_run:
> +                with ui.uninterruptible():
> +                    _reorder_filelog_parents(repo, fl, sorted(to_fix))
> +

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.

> rewrite.py:640-644
> +        total = sum(
> +            1
> +            for (t, p, _e, _s) in repo.store.datafiles()
> +            if p.endswith(b'.i') and t & store.FILEFLAGS_FILELOG
> +        )

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

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/20210804/d75c5af7/attachment-0002.html>