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

[Phillip Cohen]

> 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.

We debated this approach internally actually. There were some light
concerns over the extra filesystem calls and the usefulness of being able
to inspect non-mutating processes. However I think I still lean a bit
towards it, because it seems more widely useful as a platform for building
things. Plus, `hg procs` would be cool. But not strongly opinionated.

> 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.

OK, cool. This was actually my first version but I figured JSON was not yet
widely accepted as the go-to serialization format.

My only real disagreement is I think `ui.fullargs` or `ui.argv` is still
useful and we should add it (perhaps elsewhere).

On Mon, Mar 20, 2017 at 5:39 AM, Ryan McElroy <rm at fb.com> wrote:

> 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>')
>>
>>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170320/74e3b6c2/attachment.html>