Subject: bug#63221: 30.0.50; [PATCH] Eshell should get user (and group) IDs in a connection-aware fashion

From: Eli Zaretskii <[email-masked]>
Date: Tue, 02 May 2023 14:58:39 +0300

> Date: Mon, 1 May 2023 22:42:53 -0700
> From: Jim Porter <[email-masked]>
> 
> -(defcustom eshell-rm-interactive-query (= (user-uid) 0)
> -  "If non-nil, `rm' will query before removing anything."
> -  :type 'boolean
> +(define-widget 'eshell-interactive-query 'lazy
> +  "When to interatively query the user about a particular operation."

This terse sentence needs to be explained in the rest of the doc
string, because, unlike "If non-nil", "When" does not explain itself.
The doc string should explain how to specify "when".  It should also
explain the different supported values.

> +  :tag "Query"
> +  :type '(choice (const :tag "Never" nil)
> +                 (const :tag "Always" t)
> +                 (const :tag "When root" root)))

Also, the default value is not one of the possible optional values.

Same comment to the other similar defcustoms where you changed a
boolean option to something else: their doc strings are now
obfuscated.

> +(defun eshell-interactive-query-p (value)
> +  "Return non-nil if a command should query the user according to VALUE.
> +If VALUE is `root', return non-nil when evaluated as root (see
> +`file-user-uid').  Otherwise, simply return VALUE."

You assume here that "evaluated as root" explains itself?  I wouldn't
rely on that.

Thanks.