Patchwork [2,of,3] cmdutil: do not unlink lockpath if it's empty

login
register
mail settings
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

Jun Wu - Feb. 16, 2016, 11:12 a.m.
# 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.
Yuya Nishihara - Feb. 17, 2016, 3:52 p.m.
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.
Jun Wu - Feb. 17, 2016, 4:23 p.m.
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".
Yuya Nishihara - Feb. 18, 2016, 2:57 p.m.
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()