Patchwork D7972: recover: don't verify by default

login
register
mail settings
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

phabricator - Jan. 22, 2020, 7:50 p.m.
valentin.gatienbaron created this revision.
Herald added subscribers: mercurial-devel, mjpieters.
Herald added a reviewer: hg-reviewers.

REVISION 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?

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/commands.py
  tests/test-fncache.t
  tests/test-journal-exists.t
  tests/test-repair-strip.t
  tests/test-rollback.t

CHANGE DETAILS




To: valentin.gatienbaron, #hg-reviewers
Cc: mjpieters, mercurial-devel
phabricator - Jan. 22, 2020, 8:14 p.m.
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
phabricator - Jan. 22, 2020, 9:30 p.m.
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
phabricator - Jan. 23, 2020, 2:22 a.m.
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
phabricator - Feb. 10, 2020, 6:07 p.m.
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
phabricator - Feb. 10, 2020, 9:37 p.m.
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
phabricator - Feb. 11, 2020, 2:49 p.m.
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
phabricator - Feb. 12, 2020, 4:35 p.m.
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):