| 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
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
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.
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.)
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
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?
> 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
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