Patchwork [8,of,8,upgraderepo,V3] repair: clean up stale lock file from store backup

login
register
mail settings
Submitter Gregory Szorc
Date Dec. 19, 2016, 1:08 a.m.
Message ID <1efcda54217b590bd076.1482109684@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/17963/
State Accepted
Headers show

Comments

Gregory Szorc - Dec. 19, 2016, 1:08 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1480041929 28800
#      Thu Nov 24 18:45:29 2016 -0800
# Node ID 1efcda54217b590bd076bffaaf18a98fe9096dc7
# Parent  96b6f747358116643b039d9c91157cde27106662
repair: clean up stale lock file from store backup

Since we did a directory rename on the stores, the source
repository's lock path now references the dest repository's
lock path and the dest repository's lock path now references
a non-existent filename.

So releasing the lock on the source will unlock the dest and
releasing the lock on the dest will no-op because it fails due
to file not found. So we clean up the dest's lock manually.
Augie Fackler - Dec. 24, 2016, 8:23 p.m.
On Sun, Dec 18, 2016 at 05:08:04PM -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1480041929 28800
> #      Thu Nov 24 18:45:29 2016 -0800
> # Node ID 1efcda54217b590bd076bffaaf18a98fe9096dc7
> # Parent  96b6f747358116643b039d9c91157cde27106662
> repair: clean up stale lock file from store backup

This series LGTM. It's got a few big commits in it, but they're all
configurational code that is pretty easy to read over and doesn't
merit further splitting of the series or asking for more roundtrips.

>
> Since we did a directory rename on the stores, the source
> repository's lock path now references the dest repository's
> lock path and the dest repository's lock path now references
> a non-existent filename.
>
> So releasing the lock on the source will unlock the dest and
> releasing the lock on the dest will no-op because it fails due
> to file not found. So we clean up the dest's lock manually.
>
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -920,6 +920,12 @@ def _upgraderepo(ui, srcrepo, dstrepo, r
>                 'again\n'))
>      scmutil.writerequires(srcrepo.vfs, requirements)
>
> +    # The lock file from the old store won't be removed because nothing has a
> +    # reference to its new location. So clean it up manually. Alternatively, we
> +    # could update srcrepo.svfs and other variables to point to the new
> +    # location. This is simpler.
> +    backupvfs.unlink('store/lock')
> +
>      return backuppath
>
>  def upgraderepo(ui, repo, run=False, optimize=None):
> diff --git a/tests/test-upgrade-repo.t b/tests/test-upgrade-repo.t
> --- a/tests/test-upgrade-repo.t
> +++ b/tests/test-upgrade-repo.t
> @@ -303,7 +303,6 @@ old store should be backed up
>    00manifest.i
>    data
>    fncache
> -  lock
>    phaseroots
>    undo
>    undo.backup.fncache
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Jan. 12, 2017, 4:53 p.m.
On 12/19/2016 02:08 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1480041929 28800
> #      Thu Nov 24 18:45:29 2016 -0800
> # Node ID 1efcda54217b590bd076bffaaf18a98fe9096dc7
> # Parent  96b6f747358116643b039d9c91157cde27106662
> repair: clean up stale lock file from store backup

[hooray finally got my stack to this series]

In short, There is many things I would do differently in terms of input 
and output for this commands. But Greg had a good point last time we 
chatted. This is a debug command so Backward compatibility does not 
apply. In its current UI state, the command is already helpful to some 
people and serving some purpose. So lets get make sure this get in 4.1 
and see how much improvement Greg and I can get on top of that.

I also had various style concerns that I discussed with Yuya. As a 
result of this discussion, I'm taking that series as is and will follow 
up with Greg about these style question.

Below are more details feedback about the input and output. Hopefully, 
Greg an I will refine them to the best we can get ;-)

Input:
======

I think all inputs should be read from a config option. All formats 
choice already have a config knob and we should focus on that for all 
aspect of the command. This means going further and having config knob 
with the optimization. I can fully imagine a config section dedicated to 
aggressive optimization. That could be used by central server to 
aggressively process change at push time or local client doing it at 
commit time. As a general rules on thumb I think everything this command 
achieves should be achievable with a server properly configured from the 
start.

    redelta = <none|parent|multibase|all>

output:
=======

I would take a more terse, yet more informative output by default. See below

  ----

format                   repo    config  default
fncache                  on              on
removecldeltachain       off <!>         on
generaldelta             off     off <!> on

optimisation             config   default
redelta                  multibase none


(you have 1 update pending, use --run to upgrade your repo)
(xxx something about the kept back value in config ?)
(use --verbose to get help text on each value)

   -----

Note that the above would be able to list and perform -downgrade-, 
Running with this state would "demote" a repository.

   format                   repo       config   default
   generaldelta             on <!>     off      on


Cheers,

Patch

diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -920,6 +920,12 @@  def _upgraderepo(ui, srcrepo, dstrepo, r
                'again\n'))
     scmutil.writerequires(srcrepo.vfs, requirements)
 
+    # The lock file from the old store won't be removed because nothing has a
+    # reference to its new location. So clean it up manually. Alternatively, we
+    # could update srcrepo.svfs and other variables to point to the new
+    # location. This is simpler.
+    backupvfs.unlink('store/lock')
+
     return backuppath
 
 def upgraderepo(ui, repo, run=False, optimize=None):
diff --git a/tests/test-upgrade-repo.t b/tests/test-upgrade-repo.t
--- a/tests/test-upgrade-repo.t
+++ b/tests/test-upgrade-repo.t
@@ -303,7 +303,6 @@  old store should be backed up
   00manifest.i
   data
   fncache
-  lock
   phaseroots
   undo
   undo.backup.fncache