Patchwork [10,of,10] repair: clean up stale lock file from store backup

login
register
mail settings
Submitter Gregory Szorc
Date Nov. 6, 2016, 4:40 a.m.
Message ID <0e130e8d2156d9f2523c.1478407226@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/17370/
State Changes Requested
Headers show

Comments

Gregory Szorc - Nov. 6, 2016, 4:40 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1478392394 25200
#      Sat Nov 05 17:33:14 2016 -0700
# Node ID 0e130e8d2156d9f2523c711ef4fefe4ba33e6818
# Parent  3d4dd237b705479f8c7475b821ae719893381fa8
repair: clean up stale lock file from store backup

So inline comment for reasons.
Augie Fackler - Nov. 9, 2016, 5:05 p.m.
On Sat, Nov 05, 2016 at 09:40:26PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1478392394 25200
> #      Sat Nov 05 17:33:14 2016 -0700
> # Node ID 0e130e8d2156d9f2523c711ef4fefe4ba33e6818
> # Parent  3d4dd237b705479f8c7475b821ae719893381fa8
> repair: clean up stale lock file from store backup

In general, this looks pretty good. I don't have time to give it a
thorough going-over right now, but what I did have time to look at
seemed cromulent and it's going in the right place. I'd love to hear
from someone that has more experience with our transaction logic.

>
> So inline comment for reasons.
>
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -716,6 +716,12 @@ def _upgraderepo(ui, srcrepo, dstrepo, r
>      ui.write(_('store replacement complete; repository was inconsistent for '
>                 '%0.1fs\n') % elapsed)
>
> +    # 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')
> +
>  def upgraderepo(ui, repo, dryrun=False):
>      """Upgrade a repository in place."""
>      # Avoid cycle: cmdutil -> repair -> localrepo -> cmdutil
> 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
> @@ -209,7 +209,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 - Nov. 10, 2016, 5:30 p.m.
On 11/09/2016 05:05 PM, Augie Fackler wrote:
> On Sat, Nov 05, 2016 at 09:40:26PM -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1478392394 25200
>> #      Sat Nov 05 17:33:14 2016 -0700
>> # Node ID 0e130e8d2156d9f2523c711ef4fefe4ba33e6818
>> # Parent  3d4dd237b705479f8c7475b821ae719893381fa8
>> repair: clean up stale lock file from store backup
>
> In general, this looks pretty good. I don't have time to give it a
> thorough going-over right now, but what I did have time to look at
> seemed cromulent and it's going in the right place. I'd love to hear
> from someone that has more experience with our transaction logic.

In the same spirit of "I like it but did not look too much at it"

I'm planning to eventually take a look at this, this was on my todo and 
I'm very happy to have someone else do that. I'll hopefull come to look 
at it properly in the next 48h.
Gregory Szorc - Nov. 20, 2016, 8:20 p.m.
On Thu, Nov 10, 2016 at 9:30 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 11/09/2016 05:05 PM, Augie Fackler wrote:
>
>> On Sat, Nov 05, 2016 at 09:40:26PM -0700, Gregory Szorc wrote:
>>
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc@gmail.com>
>>> # Date 1478392394 25200
>>> #      Sat Nov 05 17:33:14 2016 -0700
>>> # Node ID 0e130e8d2156d9f2523c711ef4fefe4ba33e6818
>>> # Parent  3d4dd237b705479f8c7475b821ae719893381fa8
>>> repair: clean up stale lock file from store backup
>>>
>>
>> In general, this looks pretty good. I don't have time to give it a
>> thorough going-over right now, but what I did have time to look at
>> seemed cromulent and it's going in the right place. I'd love to hear
>> from someone that has more experience with our transaction logic.
>>
>
> In the same spirit of "I like it but did not look too much at it"
>
> I'm planning to eventually take a look at this, this was on my todo and
> I'm very happy to have someone else do that. I'll hopefull come to look at
> it properly in the next 48h.
>

Gentle review ping.

(I've already made some minor changes to this series locally. I'm holding
off on patchbombing a V2 until I hear back. So please don't queue any of
this.)
Augie Fackler - Nov. 21, 2016, 10:10 p.m.
On Sat, Nov 05, 2016 at 09:40:26PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1478392394 25200
> #      Sat Nov 05 17:33:14 2016 -0700
> # Node ID 0e130e8d2156d9f2523c711ef4fefe4ba33e6818
> # Parent  3d4dd237b705479f8c7475b821ae719893381fa8
> repair: clean up stale lock file from store backup

I've sent some nits on this series, but overall it looks good to
me. Please get an ack from Pierre-yves that he's had a chance to scan
things before doing a resend though, as I suspect he'll catch
important things I'll miss.

>
> So inline comment for reasons.
>
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -716,6 +716,12 @@ def _upgraderepo(ui, srcrepo, dstrepo, r
>      ui.write(_('store replacement complete; repository was inconsistent for '
>                 '%0.1fs\n') % elapsed)
>
> +    # 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')
> +
>  def upgraderepo(ui, repo, dryrun=False):
>      """Upgrade a repository in place."""
>      # Avoid cycle: cmdutil -> repair -> localrepo -> cmdutil
> 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
> @@ -209,7 +209,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 - Nov. 22, 2016, 2:27 a.m.
On 11/06/2016 05:40 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1478392394 25200
> #      Sat Nov 05 17:33:14 2016 -0700
> # Node ID 0e130e8d2156d9f2523c711ef4fefe4ba33e6818
> # Parent  3d4dd237b705479f8c7475b821ae719893381fa8
> repair: clean up stale lock file from store backup
>
> So inline comment for reasons.
>
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -716,6 +716,12 @@ def _upgraderepo(ui, srcrepo, dstrepo, r
>      ui.write(_('store replacement complete; repository was inconsistent for '
>                 '%0.1fs\n') % elapsed)
>
> +    # 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')

I think I get why we need to clean the old lock, however I'm not sure 
how the rest of the old store directory get handled ? Why do we need a 
special case for the lock file itself?
Augie Fackler - Nov. 22, 2016, 3:09 a.m.
> On Nov 21, 2016, at 9:27 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 11/06/2016 05:40 AM, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1478392394 25200
>> #      Sat Nov 05 17:33:14 2016 -0700
>> # Node ID 0e130e8d2156d9f2523c711ef4fefe4ba33e6818
>> # Parent  3d4dd237b705479f8c7475b821ae719893381fa8
>> repair: clean up stale lock file from store backup
>> 
>> So inline comment for reasons.
>> 
>> diff --git a/mercurial/repair.py b/mercurial/repair.py
>> --- a/mercurial/repair.py
>> +++ b/mercurial/repair.py
>> @@ -716,6 +716,12 @@ def _upgraderepo(ui, srcrepo, dstrepo, r
>>     ui.write(_('store replacement complete; repository was inconsistent for '
>>                '%0.1fs\n') % elapsed)
>> 
>> +    # 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')
> 
> I think I get why we need to clean the old lock, however I'm not sure how the rest of the old store directory get handled ? Why do we need a special case for the lock file itself?

He’s built up a new store directory over the course of _upgradrepo, and then does `mv store store.old && mv store.new store` (conceptually) once the upgrade process is done.

> 
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Nov. 23, 2016, 3:48 p.m.
On 11/22/2016 04:09 AM, Augie Fackler wrote:
>
>> On Nov 21, 2016, at 9:27 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>>
>> On 11/06/2016 05:40 AM, Gregory Szorc wrote:
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc@gmail.com>
>>> # Date 1478392394 25200
>>> #      Sat Nov 05 17:33:14 2016 -0700
>>> # Node ID 0e130e8d2156d9f2523c711ef4fefe4ba33e6818
>>> # Parent  3d4dd237b705479f8c7475b821ae719893381fa8
>>> repair: clean up stale lock file from store backup
>>>
>>> So inline comment for reasons.
>>>
>>> diff --git a/mercurial/repair.py b/mercurial/repair.py
>>> --- a/mercurial/repair.py
>>> +++ b/mercurial/repair.py
>>> @@ -716,6 +716,12 @@ def _upgraderepo(ui, srcrepo, dstrepo, r
>>>     ui.write(_('store replacement complete; repository was inconsistent for '
>>>                '%0.1fs\n') % elapsed)
>>>
>>> +    # 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')
>>
>> I think I get why we need to clean the old lock, however I'm not sure how the rest of the old store directory get handled ? Why do we need a special case for the lock file itself?
>
> He’s built up a new store directory over the course of _upgradrepo, and then does `mv store store.old && mv store.new store` (conceptually) once the upgrade process is done.

Okay, the part which failed to connect in my head was the fact we were 
keeping the old data around (but we want to nuke the irrelevant lock 
file within it). There does not seems to be any message about this old 
data remaining in the repository. We should probably informs the user so 
that he can avoid having a twice as big repository after the upgrade.

Cheers,

Patch

diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -716,6 +716,12 @@  def _upgraderepo(ui, srcrepo, dstrepo, r
     ui.write(_('store replacement complete; repository was inconsistent for '
                '%0.1fs\n') % elapsed)
 
+    # 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')
+
 def upgraderepo(ui, repo, dryrun=False):
     """Upgrade a repository in place."""
     # Avoid cycle: cmdutil -> repair -> localrepo -> cmdutil
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
@@ -209,7 +209,6 @@  old store should be backed up
   00manifest.i
   data
   fncache
-  lock
   phaseroots
   undo
   undo.backup.fncache