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

Ryan McElroy rm at fb.com
Mon Mar 20 08:39:11 EDT 2017


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?

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>')
>



More information about the Mercurial-devel mailing list