Re: error in server-running-p on M$

[Stefan Monnier]

>> I usually prefer it if the command just fails and lets the user run some
>> other command to do what she wants.  Sometimes asking the question is
>> a better option, but here I don't thinkg that its worth it.  The main
>> problem with asking a question is that it's modal.

> I've implemented `server-force-delete', as you suggested.

>> Most/all Unix locks based on process-ids (like the ones used by Emacs,
>> for example) don't pay attention to the process name.  So experience
>> shows it's usually good enough.

> OK. Now `server-running-p' will return t for a matching PID process,
> and does not check the name. It is the safer behavior anyway.

>> We can also reduce the likelihood of leaving behind some obsolete
>> socket/file using kill-emacs-hook.

> When the server is running, `kill-emacs-hook' already contains code to
> turn the server off; (with the patch) that deletes the connection
> file.

> Please, take a look at the attached patch.

Looks good.  I've added some more nitpicks, with which you can install it.

>    ;; Delete the associated connection file, if applicable.
>    ;; This is actually problematic: the file may have been overwritten by
>    ;; another Emacs server in the mean time, so it's not ours any more.
> -  ;; (and (process-contact proc :server)
> -  ;;      (eq (process-status proc) 'closed)
> -  ;;      (ignore-errors (delete-file (process-get proc :server-file))))
> +  (and (process-contact proc :server)
> +       (eq (process-status proc) 'closed)
> +       (ignore-errors (delete-file (process-get proc :server-file))))

I didn't mean to leave the comment as is.  Please update the comment to
say that the file *should* be ours thanks to the server-wunning-p check
in server-start.

> +If `server-running-p' returns t, the server is not started.

I'd just write it as "If a server is already running, the server is not
started".

> +      (if (memq (server-running-p server-name) '(nil :other))

Better check (not (eq t (server-running-p server-name))).

> -     ;; Remove any leftover socket or authentication file.

You've moved the subsequent line but left out this comment that goes
with it.

> +;;;###autoload
> +(defun server-force-delete (&optional name)
> +  "Unconditionally delete connection file for server NAME.
> +NAME defaults to `server-name'.  With argument, ask for NAME."
> +  (interactive
> +   (list (if current-prefix-arg
> +          (read-string "Server name: " nil nil server-name))))
> +  (let ((file (expand-file-name (or name server-name)
> +                             (if server-use-tcp
> +                                 server-auth-dir
> +                               server-socket-dir))))
> +    (condition-case nil
> +     (progn
> +       (delete-file file)
> +       (message "Connection file %S deleted" file))
> +      (file-error
> +       (message "Connection file %S not found or not deleted" file)))))

This should first stop our own server.  The user will usually not run
it when our server is running, but she might do it occasionally.

>  (defun server-running-p (&optional name)
> -  "Test whether server NAME is running."
> +  "Test whether server NAME is running.
> +
> +NOTE: This function is designed to return immediately, rather than
> +risking non-termination.  In some cases it returns `:other' when it
> +cannot completely determine whether there's a server running or not."

The docstring should say:
nil => the server is definitely not running.
t   => the server seems to be running.
something else => we cannot determine whether it's running without
    using commands which may have to wait for a long time.

No need to document the :other value.

> +         (or (and (looking-at "127\.0\.0\.1:[0-9]+ \\([0-9]+\\)")
> +                  (assq 'comm
> +                        (system-process-attributes
> +                         (string-to-number (match-string 1))))

                     t)

> +             :other))

-- Stefan