Patchwork [4,of,4] lock message: provide feedback how to handle "waiting for lock on repository" in error message

login
register
mail settings
Submitter Tuli Uchitel
Date March 8, 2016, 8:50 p.m.
Message ID <068e106bb16cfe91d510.1457470217@yous-iphone.dhcp.thefacebook.com>
Download mbox | patch
Permalink /patch/13695/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Tuli Uchitel - March 8, 2016, 8:50 p.m.
# HG changeset patch
# User Tuli Uchitel <tuli@fb.com>
# Date 1457464102 0
#      Tue Mar 08 19:08:22 2016 +0000
# Branch stable
# Node ID 068e106bb16cfe91d510518aa86e05229fe6d2ed
# Parent  d3da9ce4c07018109275c91e41b2c33231449104
lock message: provide feedback how to handle "waiting for lock on repository" in error message

(issue4752) currently, whenever there is an attempt to acquire the lock to a repository, which is already occupied, we have a laconic message of the form 'waiting for lock on <description> held by <hostname:pid>'. This message doesn't provide the user any feedback that can help to resolve the issue. This series of commits, intends to provide a mechanism that provides the means, to record description of operations being carried out after a lock is acquired, that can later be retrieved in order to give information for the reason of the lock's acquisition. In particular we shall be using this mechanism, to record information about an editor being opened, as in the case of commiting a change. We also improve the formulation of the original message.
The new message will be of the form: 'waiting for lock on <description> help by process <pid> on host <hostname>(user <username> requested to open a <editor executable path> session on host <hostname>)'
timeless - March 8, 2016, 9:27 p.m.
Tuli Uchitel wrote:
> +            basicwarningmessage = 'waiting for lock on %s held by process %s on host %s' % (desc, pid, host)
> +        except ValueError:
> +            basicwarningmessage = 'waiting for lock on %s held by %r\n' % (desc, inst.locker)

In general, your _()s should be here, not later.

But, who's writing this stuff, and who is reading it?

Because if I'm creating the lock and my LANG= Russian, and you're
hitting my lock and your LANG= Japanese, we don't want to show you a
message my hg generated in Russian (you speak Japanese and are
unlikely to speak Russian).
Pierre-Yves David - March 10, 2016, 3:16 p.m.
On 03/08/2016 08:50 PM, Tuli Uchitel wrote:
> # HG changeset patch
> # User Tuli Uchitel <tuli@fb.com>
> # Date 1457464102 0
> #      Tue Mar 08 19:08:22 2016 +0000
> # Branch stable
> # Node ID 068e106bb16cfe91d510518aa86e05229fe6d2ed
> # Parent  d3da9ce4c07018109275c91e41b2c33231449104
> lock message: provide feedback how to handle "waiting for lock on repository" in error message
>
> (issue4752)

this should go in the first line

> currently, whenever there is an attempt to acquire the lock to a repository, which is already occupied, we have a laconic message of the form 'waiting for lock on <description> held by <hostname:pid>'. This message doesn't provide the user any feedback that can help to resolve the issue. This series of commits, intends to provide a mechanism that provides the means, to record description of operations being carried out after a lock is acquired, that can later be retrieved in order to give information for the reason of the lock's acquisition. In particular we shall be using this mechanism, to record information about an editor being opened, as in the case of commiting a change. We also improve the formulation of the original message.
> The new message will be of the form: 'waiting for lock on <description> help by process <pid> on host <hostname>(user <username> requested to open a <editor executable path> session on host <hostname>)'

That message should probably be on two lines:

waiting for lock on <desc> help by process <pid> on host <hostname>
(user <username> requested to open a <editor executable path> session on 
host <hostname>)

Also the change to the core message should go in a different changeset 
that the hint (and probably and the begining of your stack.

Thanks a lot for working on this.

> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py

Don't add new rarely called method on localrepo. find somewhere else to 
put them.


> @@ -1244,6 +1244,18 @@
>               lockdescriptionfilename = "%s.description" % lockfilename
>           return lockdescriptionfilename
>
> +    def _createwarningmessages(self, desc, inst):
> +        try:
> +            host, pid = inst.locker.split(":", 1)
> +            basicwarningmessage = 'waiting for lock on %s held by process %s on host %s' % (desc, pid, host)
> +        except ValueError:
> +            basicwarningmessage = 'waiting for lock on %s held by %r\n' % (desc, inst.locker)
> +        detailedwarningmessage = None
> +        lockloggerfilename = self._getlockdescriptionfilename(inst.filename)
> +        if lockloggerfilename is not None and self.vfs.exists(lockloggerfilename):
> +            detailedwarningmessage = self.vfs.read(lockloggerfilename)
> +        return basicwarningmessage, detailedwarningmessage

I would aim for a bit more compact variable name to help visibility eg: 
basicmsg, detailedmsg. Even maybe just `msg, hint`.

> +
>       def _lock(self, vfs, lockname, wait, releasefn, acquirefn, desc,
>                 inheritchecker=None, parentenvvar=None):
>           parentlock = None
> @@ -1259,8 +1271,15 @@
>           except error.LockHeld as inst:
>               if not wait:
>                   raise
> -            self.ui.warn(_("waiting for lock on %s held by %r\n") %
> -                         (desc, inst.locker))
> +
> +            # Print warning messages for user about the lock
> +            basicwarningmessage, detailedwarningmessage = self._createwarningmessages(desc, inst)
> +            if detailedwarningmessage is not None:
> +                warningmessage = _("{} ({})\n").format(basicwarningmessage, detailedwarningmessage)

That translation is not going to give anything, the translating can't do 
anything useful with "{} {}" and the translating code won't have a key 
for the content after transformation (because pid, user and editor are 
already inserted)

> +            else:
> +                warningmessage = _("{}\n").format(basicwarningmessage)
> +            self.ui.warn(warningmessage)

You should issue two call to ui.warn.

self.ui.warn("%s"  % msg)
if hint:
    self.ui.warn("%s\n" % hint)

> +
>               # default to 600 seconds timeout
>               l = lockmod.lock(vfs, lockname,
>                                int(self.ui.config("ui", "timeout", "600")),
Pierre-Yves David - March 10, 2016, 3:43 p.m.
I forgot to mention that at that point, we will want some test using 
that new behavior. To make sure it actually keep working in the future.

On 03/10/2016 03:16 PM, Pierre-Yves David wrote:
>
>
> On 03/08/2016 08:50 PM, Tuli Uchitel wrote:
>> # HG changeset patch
>> # User Tuli Uchitel <tuli@fb.com>
>> # Date 1457464102 0
>> #      Tue Mar 08 19:08:22 2016 +0000
>> # Branch stable
>> # Node ID 068e106bb16cfe91d510518aa86e05229fe6d2ed
>> # Parent  d3da9ce4c07018109275c91e41b2c33231449104
>> lock message: provide feedback how to handle "waiting for lock on
>> repository" in error message
>>
>> (issue4752)
>
> this should go in the first line
>
>> currently, whenever there is an attempt to acquire the lock to a
>> repository, which is already occupied, we have a laconic message of
>> the form 'waiting for lock on <description> held by <hostname:pid>'.
>> This message doesn't provide the user any feedback that can help to
>> resolve the issue. This series of commits, intends to provide a
>> mechanism that provides the means, to record description of operations
>> being carried out after a lock is acquired, that can later be
>> retrieved in order to give information for the reason of the lock's
>> acquisition. In particular we shall be using this mechanism, to record
>> information about an editor being opened, as in the case of commiting
>> a change. We also improve the formulation of the original message.
>> The new message will be of the form: 'waiting for lock on
>> <description> help by process <pid> on host <hostname>(user <username>
>> requested to open a <editor executable path> session on host <hostname>)'
>
> That message should probably be on two lines:
>
> waiting for lock on <desc> help by process <pid> on host <hostname>
> (user <username> requested to open a <editor executable path> session on
> host <hostname>)
>
> Also the change to the core message should go in a different changeset
> that the hint (and probably and the begining of your stack.
>
> Thanks a lot for working on this.
>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>
> Don't add new rarely called method on localrepo. find somewhere else to
> put them.
>
>
>> @@ -1244,6 +1244,18 @@
>>               lockdescriptionfilename = "%s.description" % lockfilename
>>           return lockdescriptionfilename
>>
>> +    def _createwarningmessages(self, desc, inst):
>> +        try:
>> +            host, pid = inst.locker.split(":", 1)
>> +            basicwarningmessage = 'waiting for lock on %s held by
>> process %s on host %s' % (desc, pid, host)
>> +        except ValueError:
>> +            basicwarningmessage = 'waiting for lock on %s held by
>> %r\n' % (desc, inst.locker)
>> +        detailedwarningmessage = None
>> +        lockloggerfilename =
>> self._getlockdescriptionfilename(inst.filename)
>> +        if lockloggerfilename is not None and
>> self.vfs.exists(lockloggerfilename):
>> +            detailedwarningmessage = self.vfs.read(lockloggerfilename)
>> +        return basicwarningmessage, detailedwarningmessage
>
> I would aim for a bit more compact variable name to help visibility eg:
> basicmsg, detailedmsg. Even maybe just `msg, hint`.
>
>> +
>>       def _lock(self, vfs, lockname, wait, releasefn, acquirefn, desc,
>>                 inheritchecker=None, parentenvvar=None):
>>           parentlock = None
>> @@ -1259,8 +1271,15 @@
>>           except error.LockHeld as inst:
>>               if not wait:
>>                   raise
>> -            self.ui.warn(_("waiting for lock on %s held by %r\n") %
>> -                         (desc, inst.locker))
>> +
>> +            # Print warning messages for user about the lock
>> +            basicwarningmessage, detailedwarningmessage =
>> self._createwarningmessages(desc, inst)
>> +            if detailedwarningmessage is not None:
>> +                warningmessage = _("{}
>> ({})\n").format(basicwarningmessage, detailedwarningmessage)
>
> That translation is not going to give anything, the translating can't do
> anything useful with "{} {}" and the translating code won't have a key
> for the content after transformation (because pid, user and editor are
> already inserted)
>
>> +            else:
>> +                warningmessage = _("{}\n").format(basicwarningmessage)
>> +            self.ui.warn(warningmessage)
>
> You should issue two call to ui.warn.
>
> self.ui.warn("%s"  % msg)
> if hint:
>     self.ui.warn("%s\n" % hint)
>
>> +
>>               # default to 600 seconds timeout
>>               l = lockmod.lock(vfs, lockname,
>>                                int(self.ui.config("ui", "timeout",
>> "600")),
>
>

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1244,6 +1244,18 @@ 
             lockdescriptionfilename = "%s.description" % lockfilename
         return lockdescriptionfilename
 
+    def _createwarningmessages(self, desc, inst):
+        try:
+            host, pid = inst.locker.split(":", 1)
+            basicwarningmessage = 'waiting for lock on %s held by process %s on host %s' % (desc, pid, host)
+        except ValueError:
+            basicwarningmessage = 'waiting for lock on %s held by %r\n' % (desc, inst.locker)
+        detailedwarningmessage = None
+        lockloggerfilename = self._getlockdescriptionfilename(inst.filename)
+        if lockloggerfilename is not None and self.vfs.exists(lockloggerfilename):
+            detailedwarningmessage = self.vfs.read(lockloggerfilename)
+        return basicwarningmessage, detailedwarningmessage
+
     def _lock(self, vfs, lockname, wait, releasefn, acquirefn, desc,
               inheritchecker=None, parentenvvar=None):
         parentlock = None
@@ -1259,8 +1271,15 @@ 
         except error.LockHeld as inst:
             if not wait:
                 raise
-            self.ui.warn(_("waiting for lock on %s held by %r\n") %
-                         (desc, inst.locker))
+
+            # Print warning messages for user about the lock
+            basicwarningmessage, detailedwarningmessage = self._createwarningmessages(desc, inst)
+            if detailedwarningmessage is not None:
+                warningmessage = _("{} ({})\n").format(basicwarningmessage, detailedwarningmessage)
+            else:
+                warningmessage = _("{}\n").format(basicwarningmessage)
+            self.ui.warn(warningmessage)
+
             # default to 600 seconds timeout
             l = lockmod.lock(vfs, lockname,
                              int(self.ui.config("ui", "timeout", "600")),