Patchwork [2,of,2,STABLE,MOST-WANTED] push: make locking of source optional (issue3684)

login
register
mail settings
Submitter Pierre-Yves David
Date April 30, 2013, 10:04 a.m.
Message ID <4be1d06df73f0092277b.1367316290@crater1.logilab.fr>
Download mbox | patch
Permalink /patch/1509/
State Superseded
Commit 3f5e75c22585975eaf4ea1c8da71676a88637eec
Headers show

Comments

Pierre-Yves David - April 30, 2013, 10:04 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@logilab.fr>
# Date 1367315511 -7200
#      Tue Apr 30 11:51:51 2013 +0200
# Branch stable
# Node ID 4be1d06df73f0092277b83e243ba4ca583c040e0
# Parent  b5c9626ec444375a6368d40ba5218ede6401c68d
push: make locking of source optional (issue3684)

Having the permission to lock the source repo on push is now optional. When the
repo cannot be locked, phase are not changed locally. A status message is issue
when some actual phase movement are skipped:

    public phase unchanged locally, cannot lock repo

A debug message with the exact reason of the locking failure is issued in all
case.
Pierre-Yves David - April 30, 2013, 1:57 p.m.
On Tue, Apr 30, 2013 at 08:51:31AM -0500, Kevin Bullock wrote:
> On 30 Apr 2013, at 5:04 AM, pierre-yves.david@logilab.fr wrote:
> 
> > # HG changeset patch
> > # User Pierre-Yves David <pierre-yves.david@logilab.fr>
> > # Date 1367315511 -7200
> > #      Tue Apr 30 11:51:51 2013 +0200
> > # Branch stable
> > # Node ID 4be1d06df73f0092277b83e243ba4ca583c040e0
> > # Parent  b5c9626ec444375a6368d40ba5218ede6401c68d
> > push: make locking of source optional (issue3684)
> > 
> > Having the permission to lock the source repo on push is now optional. When the
> > repo cannot be locked, phase are not changed locally. A status message is issue
> > when some actual phase movement are skipped:
> > 
> >    public phase unchanged locally, cannot lock repo
> 
> I think in order to be useful, the message would need to somehow reference what _changesets_ couldn't have their phases changed.

Yes, but then you have to handle the fact that this message could reference a
gazillion different heads.

So it needs to be something like:

    cannot locally turn 4be1d06df73f (and 13 others heads) public, unable to lock repo

That exceeded my "deserves another patch" threshold. Lets put the simplest
version first and then think about the best message.

I'm happy that you brought this up !


> 
> > 
> > A debug message with the exact reason of the locking failure is issued in all
> > case.
> > 
> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > --- a/mercurial/localrepo.py
> > +++ b/mercurial/localrepo.py
> > @@ -1764,11 +1764,30 @@ class localrepository(object):
> >         unfi = self.unfiltered()
> >         def localphasemove(nodes, phase=phases.public):
> >             """move <nodes> to <phase> in the local source repo"""
> >             phases.advanceboundary(self, phase, nodes)
> >         # get local lock as we might write phase data
> > -        locallock = self.lock()
> > +        locallock = None
> > +        try:
> > +            locallock = self.lock()
> > +        except IOError, err:
> > +            if err.errno != errno.EACCES:
> > +                raise
> > +            # source repo cannot be locked.
> > +            # We do not abort the push, but just disable the local phase
> > +            # synchronisation.
> > +            msg = 'cannot lock source repository: %s\n' % err
> > +            self.ui.debug(msg)
> > +            def localphasemove(nodes, phase=phases.public):
> 
> Why not just check 'if locallock is None' inside the closure instead of
> completely redefining it? This way is a bit more confusing to read.

Good idea.
Matt Mackall - April 30, 2013, 3:38 p.m.
On Tue, 2013-04-30 at 12:04 +0200, pierre-yves.david@logilab.fr wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@logilab.fr>
> # Date 1367315511 -7200
> #      Tue Apr 30 11:51:51 2013 +0200
> # Branch stable
> # Node ID 4be1d06df73f0092277b83e243ba4ca583c040e0
> # Parent  b5c9626ec444375a6368d40ba5218ede6401c68d
> push: make locking of source optional (issue3684)
> 
> Having the permission to lock the source repo on push is now optional. When the
> repo cannot be locked, phase are not changed locally. A status message is issue
> when some actual phase movement are skipped:
> 
>     public phase unchanged locally, cannot lock repo

This message will confuse people because it puts cause after effect and
uses the passive voice for the effect. It might be read as:

 "some mysterious phase thing is wrong, therefore cannot lock repo"

How about:

 cannot lock source repo, not marking source changesets public

Or:

 cannot lock source repo, not updating source phases

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1764,11 +1764,30 @@  class localrepository(object):
         unfi = self.unfiltered()
         def localphasemove(nodes, phase=phases.public):
             """move <nodes> to <phase> in the local source repo"""
             phases.advanceboundary(self, phase, nodes)
         # get local lock as we might write phase data
-        locallock = self.lock()
+        locallock = None
+        try:
+            locallock = self.lock()
+        except IOError, err:
+            if err.errno != errno.EACCES:
+                raise
+            # source repo cannot be locked.
+            # We do not abort the push, but just disable the local phase
+            # synchronisation.
+            msg = 'cannot lock source repository: %s\n' % err
+            self.ui.debug(msg)
+            def localphasemove(nodes, phase=phases.public):
+                """repo is not locked, do not change any phases!
+                Informs the user that phases should have been moved when
+                applicable."""
+                actualmoves = [n for n in nodes if phase < self[n].phase()]
+                phasestr = phases.phasenames[phase]
+                if actualmoves:
+                    self.ui.status(_('%s phase unchanged locally, cannot lock '
+                                     'repo\n') % phasestr)
         try:
             self.checkpush(force, revs)
             lock = None
             unbundle = remote.capable('unbundle')
             if not unbundle:
@@ -1916,11 +1935,12 @@  class localrepository(object):
                 obsolete.syncpush(self, remote)
             finally:
                 if lock is not None:
                     lock.release()
         finally:
-            locallock.release()
+            if locallock is not None:
+                locallock.release()
 
         self.ui.debug("checking for updated bookmarks\n")
         rb = remote.listkeys('bookmarks')
         for k in rb.keys():
             if k in unfi._bookmarks:
diff --git a/tests/test-phases-exchange.t b/tests/test-phases-exchange.t
--- a/tests/test-phases-exchange.t
+++ b/tests/test-phases-exchange.t
@@ -1060,7 +1060,45 @@  2. cloning publishing repository
   |/
   o  1 public a-B - 548a3d25dbf0
   |
   o  0 public a-A - 054250a37db4
   
+
+Pushing From an unlockable repo
+--------------------------------
+(issue3684)
+
+Unability to lock the source repo should not prevent the push. It will prevent
+the retrieval of remote phase during push. For example, pushing to a publishing
+server won't turn changeset public.
+
+1. Test that push is not prevented
+
+  $ hg init Phi
+  $ cd Upsilon
+  $ chmod -R -w .hg
+  $ hg push ../Phi
+  pushing to ../Phi
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 14 changesets with 14 changes to 14 files (+3 heads)
+  $ chmod -R +w .hg
+
+2. Test that failed phases movement are reported
+
+  $ hg phase --force --draft 3
+  $ chmod -R -w .hg
+  $ hg push ../Phi
+  pushing to ../Phi
+  searching for changes
+  no changes found
+  public phase unchanged locally, cannot lock repo
+  [1]
+  $ chmod -R +w .hg
+  $ hgph Upsilon
+
+  $ cd ..
+
   $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS