[PATCH 4 of 4 STABLE] lock: show about possibility of lock corruption for empty locker

[FUJIWARA Katsunori]

At Sun, 30 Apr 2017 22:13:57 +0900,
Yuya Nishihara wrote:
> 
> On Sat, 29 Apr 2017 17:38:10 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
> > # Date 1493454424 -32400
> > #      Sat Apr 29 17:27:04 2017 +0900
> > # Branch stable
> > # Node ID 650d9f6bf1e7c1f6dee9d82616af2a56aaf56a2e
> > # Parent  b64713f57a22301b6f481cd8e401542f46356c94
> > lock: show about possibility of lock corruption for empty locker
> 
> The change looks good to me.
> 
> > --- a/mercurial/lock.py
> > +++ b/mercurial/lock.py
> > @@ -161,7 +161,8 @@ class lock(object):
> >          for new-style locks.
> >          """
> >          try:
> > -            return self.vfs.readlock(self.f)
> > +            # normalize white space by strip() to detect corruption easily
> > +            return self.vfs.readlock(self.f).strip()
> 
> I'm not sure if stripping whitespace makes sense. Do we write '\n' to a
> lockfile on Windows?

I apply strip() on locker, because we can't assume that corrupted file
always causes empty string. It might consist of white space, and
showing such string as locker is meaningless for users.

But, I'm OK to omit strip() on non-None locker string.


> > --- a/mercurial/scmutil.py
> > +++ b/mercurial/scmutil.py
> > @@ -151,10 +151,20 @@ def callcatch(ui, func):
> >      # Mercurial-specific first, followed by built-in and library exceptions
> >      except error.LockHeld as inst:
> >          if inst.errno == errno.ETIMEDOUT:
> > -            reason = _('timed out waiting for lock held by %s') % inst.locker
> > +            if inst.locker:
> > +                reason = _('timed out waiting for lock held by %s'
> > +                           ) % inst.locker
> > +            else:
> > +                reason = _('timed out waiting for lock, but might be corrupted')
> >          else:
> > -            reason = _('lock held by %s') % inst.locker
> > +            if inst.locker:
> > +                reason = _('lock held by %s') % inst.locker
> > +            else:
> > +                reason = _('lock held, but might be corrupted')
> 
> I would simply say "lock held by (unknown)" since I don't think a corrupted
> lock file is more likely to be empty.

Are you OK for a hint message itself like below for an empty lock
file, because an empty lock file is more likely to be corrupted,
especially after timeout ?

  abort: loock held by unknown
  (it might be corrupted, use 'hg debuglocks' to check/unlock)


> >          ui.warn(_("abort: %s: %s\n") % (inst.desc or inst.filename, reason))
> > +        if not inst.locker:
> > +            ui.warn('(%s)\n' %
> > +                    _("use 'hg debuglocks' to check/unlock corrupted lock"))
> 

-- 
----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp