| Submitter | phabricator |
|---|---|
| Date | Jan. 22, 2020, 7:50 p.m. |
| Message ID | <differential-rev-PHID-DREV-hynbzozhryap7ppnhl7i-req@mercurial-scm.org> |
| Download | mbox | patch |
| Permalink | /patch/44577/ |
| State | Superseded |
| Headers | show |
Comments
pulkit added a comment. I also have experience with C-c thing. Will a config option which enables `--no-verify` by default will work for you? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7972/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7972 To: valentin.gatienbaron, #hg-reviewers Cc: pulkit, mjpieters, mercurial-devel
marmoute added a comment. +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`) REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7972/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7972 To: valentin.gatienbaron, #hg-reviewers Cc: marmoute, pulkit, mjpieters, mercurial-devel
valentin.gatienbaron added a comment.
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.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7972/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7972
To: valentin.gatienbaron, #hg-reviewers
Cc: marmoute, pulkit, mjpieters, mercurial-devel
pulkit added a comment. pulkit added a subscriber: durin42. 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 used to do verify in the first place. Maybe @durin42, @marmoute or someone else knows? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7972/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7972 To: valentin.gatienbaron, #hg-reviewers, marmoute Cc: durin42, marmoute, pulkit, mjpieters, mercurial-devel
durin42 added a comment.
durin42 accepted this revision as: durin42.
In D7972#120111 <https://phab.mercurial-scm.org/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.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7972/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7972
To: valentin.gatienbaron, #hg-reviewers, marmoute, durin42
Cc: durin42, marmoute, pulkit, mjpieters, mercurial-devel
valentin.gatienbaron added a comment. > 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. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7972/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7972 To: valentin.gatienbaron, #hg-reviewers, marmoute, durin42 Cc: durin42, marmoute, pulkit, mjpieters, mercurial-devel
This revision is now accepted and ready to land. durin42 added a comment. durin42 accepted this revision. In D7972#120504 <https://phab.mercurial-scm.org/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. :/ REPOSITORY rHG Mercurial BRANCH default CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7972/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7972 To: valentin.gatienbaron, #hg-reviewers, marmoute, durin42 Cc: durin42, marmoute, pulkit, mjpieters, mercurial-devel
Patch
diff --git a/tests/test-rollback.t b/tests/test-rollback.t --- a/tests/test-rollback.t +++ b/tests/test-rollback.t @@ -190,7 +190,7 @@ corrupt journal test $ echo "foo" > .hg/store/journal - $ hg recover + $ hg recover --verify rolling back interrupted transaction couldn't read journal entry 'foo\n'! checking changesets diff --git a/tests/test-repair-strip.t b/tests/test-repair-strip.t --- a/tests/test-repair-strip.t +++ b/tests/test-repair-strip.t @@ -25,7 +25,9 @@ > else > echo "(no journal)" > 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/* > rm -rf .hg/strip-backup > } diff --git a/tests/test-journal-exists.t b/tests/test-journal-exists.t --- a/tests/test-journal-exists.t +++ b/tests/test-journal-exists.t @@ -15,11 +15,7 @@ $ hg recover rolling back interrupted transaction - checking changesets - checking manifests - crosschecking files in changesets and manifests - checking files - checked 1 changesets with 1 changes to 1 files + (verify step skipped, run `hg verify` to check your repository content) recover, explicit verify diff --git a/tests/test-fncache.t b/tests/test-fncache.t --- a/tests/test-fncache.t +++ b/tests/test-fncache.t @@ -356,7 +356,7 @@ $ cat .hg/store/fncache | sort data/y.i data/z.i - $ hg recover + $ hg recover --verify rolling back interrupted transaction checking changesets checking manifests diff --git a/mercurial/commands.py b/mercurial/commands.py --- a/mercurial/commands.py +++ b/mercurial/commands.py @@ -5671,7 +5671,7 @@ @command( 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, ) def recover(ui, repo, **opts):