| Submitter | Jun Wu |
|---|---|
| Date | Feb. 16, 2016, 11:12 a.m. |
| Message ID | <0ce18cbc108990527291.1455621121@x1c> |
| Download | mbox | patch |
| Permalink | /patch/13224/ |
| State | Changes Requested |
| Delegated to: | Yuya Nishihara |
| Headers | show |
Comments
On Tue, 16 Feb 2016 11:12:01 +0000, Jun Wu wrote: > # HG changeset patch > # User Jun Wu <quark@fb.com> > # Date 1455619460 0 > # Tue Feb 16 10:44:20 2016 +0000 > # Node ID 0ce18cbc108990527291701bb0e857f1e05288ef > # Parent 3fc701a86323ae896371ec3df375a52bce06b748 > cmdutil: do not unlink lockpath if it's empty > > Sometimes we want the effect of --daemon-pipe-fds but do not need the server > to unlink a lock file, because we may use other lock such as flock and the > lock may be held longer. This patch makes it possible by ignoring empty path. > Otherwise, we have to create a useless dummy file. > > diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py > --- a/mercurial/cmdutil.py > +++ b/mercurial/cmdutil.py > @@ -804,7 +804,8 @@ > os.setsid() > except AttributeError: > pass > - os.unlink(lockpath) > + if lockpath: > + os.unlink(lockpath) If lockpath (= opts['daemon_pipefds']) is empty, the whole "if opts['daemon_pipefds']" block would be skipped. Also, as I said before, --daemon-pipe-fds is the internal option. It isn't supposed to be specified by external tools. I don't want to introduce an unclear change in the core just for supporting chg.
On 02/17/2016 03:52 PM, Yuya Nishihara wrote: > If lockpath (= opts['daemon_pipefds']) is empty, the whole "if > opts['daemon_pipefds']" block would be skipped. Didn't notice how opts work. Then the patch is totally wrong. > Also, as I said before, --daemon-pipe-fds is the internal option. It > isn't supposed to be specified by external tools. I don't want to > introduce an unclear change in the core just for supporting chg. Since it's internal option, I guess we can rename it freely. The name "pipe-fd" does not reflect what it is doing now. I do want to split it into "--daemon-mode" and "--daemon-unlink".
On Wed, 17 Feb 2016 16:23:13 +0000, Jun Wu wrote: > On 02/17/2016 03:52 PM, Yuya Nishihara wrote: > > Also, as I said before, --daemon-pipe-fds is the internal option. It > > isn't supposed to be specified by external tools. I don't want to > > introduce an unclear change in the core just for supporting chg. > > Since it's internal option, I guess we can rename it freely. The name > "pipe-fd" does not reflect what it is doing now. I do want to split > it into "--daemon-mode" and "--daemon-unlink". or "--daemon-postexec", etc. which look like an internal option. I'm not super excited about splitting options, but it would be doable.
Patch
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py --- a/mercurial/cmdutil.py +++ b/mercurial/cmdutil.py @@ -804,7 +804,8 @@ os.setsid() except AttributeError: pass - os.unlink(lockpath) + if lockpath: + os.unlink(lockpath) util.hidewindow() sys.stdout.flush() sys.stderr.flush()