[PATCH 3 of 3] localrepo: pass args and command running as store/write lock metadata

[Jun Wu]

Excerpts from Ryan McElroy's message of 2017-03-20 12:39:11 +0000:
> Overall, I like the functionality this series adds, but I'm less 
> convinced on the specific implementation. Adding more stuff to the 
> junkyard that is the "ui" class doesn't feel awesome, and I'm concerned 
> with how it will interact with chg and a future direction of ui 
> immutability.
> 
> I have an alternate suggestion for how to achieve this visibility into 
> what is going on in a repo:
> 
> Instead of having this information inside of a lock, what if every 
> process that started up in a repo tried to create a file in 
> .hg/processes/ (or something) of the form "info.<pid>". Then you 
> wouldn't need to pass this data through to the lock command, wouldn't 
> need to plumb it through the ui class, and combining the lock info with 
> the processes info would still be straightforward.
>
> This would allow you to touch only the dispatch logic and achieve 
> something even cooler: you'd be able to see all of the processes running 
> in a repo (even read processes, as long as the reader has write access 
> to the repo's .hg/).
> 
> Thoughts on this alternate approach?

Two questions:

1. Who is responsible for garbage collecting those pid files, in case of
   crashes?
2. Linux has pid namespace, pid could conflict.

I think it's better to just rely on whatever the operating system provides
to convert pid to args.

> 
> Some nitpicks inline as well.
> 
> On 3/20/17 7:49 AM, Phil Cohen wrote:
> > # HG changeset patch
> > # User Phil Cohen <phillco at fb.com>
> > # Date 1489996027 25200
> > #      Mon Mar 20 00:47:07 2017 -0700
> > # Node ID f37121209a2bbcde572e986f2b038bf2da7f954a
> > # Parent  1ca023fb02cbe4747e2b5b625866cfa538cbebd3
> > localrepo: pass args and command running as store/write lock metadata
> >
> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > --- a/mercurial/localrepo.py
> > +++ b/mercurial/localrepo.py
> > @@ -10,6 +10,7 @@
> >   import errno
> >   import hashlib
> >   import inspect
> > +import json
> >   import os
> >   import random
> >   import time
> > @@ -1355,10 +1356,15 @@
> >           if parentenvvar is not None:
> >               parentlock = encoding.environ.get(parentenvvar)
> >           try:
> > +            metadata = json.dumps({
> > +                'cmd': self.ui.commandname,
> > +                'args': self.ui.args
> > +            })
> >               l = lockmod.lock(vfs, lockname, 0, releasefn=releasefn,
> >                                acquirefn=acquirefn, desc=desc,
> >                                inheritchecker=inheritchecker,
> > -                             parentlock=parentlock)
> > +                             parentlock=parentlock,
> > +                             metadata=metadata)
> 
> It wasn't clear to me that metadata should be a string and not arbitrary 
> data that would be "somehow" serialized. It might be good to name (in 
> the previous patch) the variable "infostring" rather than "metadata" so 
> that this is more self-documenting.
> 
> Alternatively, have the lock code do the JSON serialization, and allow 
> metadata to be a list of dict of data that the lock will do the 
> serialization for, if present. That help's guarantee that lock.info will 
> always contain JSON serialized data.
> 
> >           except error.LockHeld as inst:
> >               if not wait:
> >                   raise
> > diff --git a/tests/test-repo-lock-writes-metadata.t b/tests/test-repo-lock-writes-metadata.t
> > new file mode 100644
> > --- /dev/null
> > +++ b/tests/test-repo-lock-writes-metadata.t
> > @@ -0,0 +1,38 @@
> > +Make a repo
> > +
> > +  $ hg init test
> > +  $ cd test
> > +  $ echo file > file
> > +  $ hg commit -Aqm initial
> 
> Nit-pick: I'd love some whitespace after the previous test instead of 
> before the next test.
> 
> > +Make an extension
> > +
> > +  $ cat << EOT > lockinfo.py
> > +  > from contextlib import nested
> > +  > from mercurial import (cmdutil, extensions)
> > +  > cmdtable = {}
> > +  > command = cmdutil.command(cmdtable)
> > +  > @command('locktest')
> > +  > def locktest(ui, repo, *args, **opts):
> > +  >   def readf(path):
> > +  >      try:
> > +  >        with open(path, "r") as f:
> > +  >           return f.read()
> > +  >      except IOError as e:
> > +  >         return "<not found>"
> > +  >   def status(str):
> > +  >     print(str, readf(".hg/wlock.info"), readf(".hg/store/lock.info"))
> > +  >   status("No lock, shouldn't have either file:")
> > +  >   with repo.wlock():
> > +  >       status("Have the write lock:")
> > +  >       with repo.lock():
> > +  >         status("Also have the store lock:")
> > +  >       status("Just the write lock:")
> > +  >   status("Neither:")
> > +  > EOT
> > +Test store and write lock
> > +  $ hg locktest -v --config extensions.x=lockinfo.py
> > +  ("No lock, shouldn't have either file:", '<not found>', '<not found>')
> > +  ('Have the write lock:', '{"cmd": "locktest", "args": ["locktest", "-v"]}', '<not found>')
> > +  ('Also have the store lock:', '{"cmd": "locktest", "args": ["locktest", "-v"]}', '{"cmd": "locktest", "args": ["locktest", "-v"]}')
> > +  ('Just the write lock:', '{"cmd": "locktest", "args": ["locktest", "-v"]}', '<not found>')
> > +  ('Neither:', '<not found>', '<not found>')
> >
>