This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7972

recover: don't verify by default
ClosedPublic

Authored by valentin.gatienbaron on Jan 22 2020, 2:49 PM.

Details

Reviewers
marmoute
durin42
Group Reviewers
hg-reviewers
Commits
rHG7a4e1d245f19: recover: don't verify by default
Summary

The reason is:

  • it's not that hard to trigger interrupted transactions: just run out of disk space
  • it takes forever to verify on large repos. Before --no-verify, I told people to C-c hg recover when the progress bar showed up. Now I tell them to pass --no-verify.
  • I don't remember a single case where the verification step was useful

This is technically a change of behavior. Perhaps this would be better
suited for tweakdefaults?

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

pulkit added a subscriber: pulkit.Jan 22 2020, 3:13 PM

I also have experience with C-c thing. Will a config option which enables --no-verify by default will work for you?

marmoute added a subscriber: marmoute.Jan 22 2020, 4:30 PM

+1, the point of having the --no-verify option was to eventualy turn it on by default.

(also, we shoudl tighten the windows the transaction creation that requires hg recover)

valentin.gatienbaron added a comment.Jan 22 2020, 9:22 PM

It's already possible to control the default, like so:

[alias]
recover = recover --no-verify

So the current patch is meant to change a default. Or be abandoned, which would be fine too given that it's easy to change the default for oneself, but I figured I'd propose the change.

marmoute accepted this revision.Jan 23 2020, 4:29 AM
pulkit added a subscriber: durin42.EditedFeb 10 2020, 1:07 PM

Sorry for late reply. By changing the default, I am afraid about the cases where a user has broken repository and we only recover the transaction and don't verify. I am not sure what those cases are. Also I don't know why recover performs verify in the first place. Maybe @durin42, @marmoute or someone else knows?

durin42 accepted this revision as: durin42.Feb 10 2020, 4:37 PM
In D7972#120111, @pulkit wrote:

Sorry for late reply. By changing the default, I am afraid about the cases where a user has broken repository and we only recover the transaction and don't verify. I am not sure what those cases are. Also I don't know why recover performs verify in the first place. Maybe @durin42, @marmoute or someone else knows?

I _think_ it's just paranoia. As long as the bundle wasn't woefully corrupt, it shouldn't be a problem. I _think_ if we set some of the [server]-section bundle validation options (which should be cheap enough) we could ditch this completely safely.

As it stands, I'm fine with this patch if someone else has the confidence to push it.

valentin.gatienbaron added a comment.Feb 11 2020, 9:49 AM

I _think_ it's just paranoia. As long as the bundle wasn't woefully corrupt, it shouldn't be a problem. I _think_ if we set some of the [server]-section bundle validation options (which should be cheap enough) we could ditch this completely safely.
As it stands, I'm fine with this patch if someone else has the confidence to push it.

How does the validity of an input bundle affect recover? I would have thought it's only the validity of the journal that matters, and that's created entirely based on local data (file lengths or contents before writes).

Now I suppose the journal itself may well be truncated or not written at all when running out of disk space or other error situations where the OS does the writes out of order.

durin42 accepted this revision.Feb 12 2020, 11:35 AM
In D7972#120504, @valentin.gatienbaron wrote:

I _think_ it's just paranoia. As long as the bundle wasn't woefully corrupt, it shouldn't be a problem. I _think_ if we set some of the [server]-section bundle validation options (which should be cheap enough) we could ditch this completely safely.
As it stands, I'm fine with this patch if someone else has the confidence to push it.

How does the validity of an input bundle affect recover?

It could have borked linknodes or missing filenodes. That can happen in some cases with subtle revlog corruption. Back when I helped run code.google.com we saw a few cases of that, where clients couldn't push specific changes unless they pushed their whole repo. But anyway, I was mis-thinking about this and this is about hg recover and not recovering something from a backup bundle (sigh) so we've been talking past each other.

I would have thought it's only the validity of the journal that matters, and that's created entirely based on local data (file lengths or contents before writes).

Yes, you're right.

Now I suppose the journal itself may well be truncated or not written at all when running out of disk space or other error situations where the OS does the writes out of order.

Yeah, it's possible on a network FS or something, but this honeslty seems like a safe change to me. Sorry I misread it last time through. :/

This revision is now accepted and ready to land.Feb 12 2020, 11:35 AM
valentin.gatienbaron added a commit: rHG7a4e1d245f19: recover: don't verify by default.Feb 12 2020, 2:25 PM
Closed by commit rHG7a4e1d245f19: recover: don't verify by default (authored by valentin.gatienbaron). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 20168

mercurial/commands.py

elif not result and pushop.bkresult: elif not result and pushop.bkresult:
result = 2 result = 2
return result return result
@command( @command(
b'recover', b'recover',
[(b'', b'verify', True, b"run `hg verify` after successful recover"),], [(b'', b'verify', False, b"run `hg verify` after successful recover"),],
helpcategory=command.CATEGORY_MAINTENANCE, helpcategory=command.CATEGORY_MAINTENANCE,
) )
def recover(ui, repo, **opts): def recover(ui, repo, **opts):
"""roll back an interrupted transaction """roll back an interrupted transaction
Recover from an interrupted commit or pull. Recover from an interrupted commit or pull.
This command tries to fix the repository status after an This command tries to fix the repository status after an

tests/test-fncache.t

$ hg up -q 1 $ hg up -q 1
$ touch z $ touch z
$ hg ci -qAm z 2>/dev/null $ hg ci -qAm z 2>/dev/null
[255] [255]
$ cat .hg/store/fncache | sort $ cat .hg/store/fncache | sort
data/y.i data/y.i
data/z.i data/z.i
$ hg recover $ hg recover --verify
rolling back interrupted transaction rolling back interrupted transaction
checking changesets checking changesets
checking manifests checking manifests
crosschecking files in changesets and manifests crosschecking files in changesets and manifests
checking files checking files
checked 1 changesets with 1 changes to 1 files checked 1 changesets with 1 changes to 1 files
$ cat .hg/store/fncache $ cat .hg/store/fncache
data/y.i data/y.i

tests/test-journal-exists.t

$ echo foo > a $ echo foo > a
$ hg ci -Am0 $ hg ci -Am0
abort: abandoned transaction found! abort: abandoned transaction found!
(run 'hg recover' to clean up transaction) (run 'hg recover' to clean up transaction)
[255] [255]
$ hg recover $ hg recover
rolling back interrupted transaction rolling back interrupted transaction
checking changesets (verify step skipped, run `hg verify` to check your repository content)
checking manifests
crosschecking files in changesets and manifests
checking files
checked 1 changesets with 1 changes to 1 files
recover, explicit verify recover, explicit verify
$ touch .hg/store/journal $ touch .hg/store/journal
$ hg ci -Am0 $ hg ci -Am0
abort: abandoned transaction found! abort: abandoned transaction found!
(run 'hg recover' to clean up transaction) (run 'hg recover' to clean up transaction)
[255] [255]

tests/test-repair-strip.t

> chmod +$3 $4 > chmod +$3 $4
> hg verify > hg verify
> echo % journal contents > echo % journal contents
> if [ -f .hg/store/journal ]; then > if [ -f .hg/store/journal ]; then
> cat .hg/store/journal | "$PYTHON" $TESTTMP/dumpjournal.py > cat .hg/store/journal | "$PYTHON" $TESTTMP/dumpjournal.py
> else > else
> echo "(no journal)" > echo "(no journal)"
> fi > fi
> ls .hg/store/journal >/dev/null 2>&1 && hg recover > if ls .hg/store/journal >/dev/null 2>&1; then
> hg recover --verify
> fi
> ls .hg/strip-backup/* >/dev/null 2>&1 && hg unbundle -q .hg/strip-backup/* > ls .hg/strip-backup/* >/dev/null 2>&1 && hg unbundle -q .hg/strip-backup/*
> rm -rf .hg/strip-backup > rm -rf .hg/strip-backup
> } > }
$ hg init test $ hg init test
$ cd test $ cd test
$ echo a > a $ echo a > a
$ hg -q ci -m "a" -A $ hg -q ci -m "a" -A

tests/test-rollback.t

1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg rollback $ hg rollback
rolling back unknown transaction rolling back unknown transaction
$ cat a $ cat a
a a
corrupt journal test corrupt journal test
$ echo "foo" > .hg/store/journal $ echo "foo" > .hg/store/journal
$ hg recover $ hg recover --verify
rolling back interrupted transaction rolling back interrupted transaction
couldn't read journal entry 'foo\n'! couldn't read journal entry 'foo\n'!
checking changesets checking changesets
checking manifests checking manifests
crosschecking files in changesets and manifests crosschecking files in changesets and manifests
checking files checking files
checked 2 changesets with 2 changes to 1 files checked 2 changesets with 2 changes to 1 files