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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 63221 in the body.
You can then email your comments to 63221 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Eshell should get user (and group) IDs in a
 connection-aware fashion
Date: Mon, 1 May 2023 22:42:53 -0700

[Message part 1 (text/plain, inline)]

This is sort of a continuation from bug#60722. Eshell gets the user's 
UID/GID in a few places, e.g. to decide when to prompt before deleting a 
file with "rm" (if you're root, it should prompt). However, these aren't 
connection-aware. That means that if you start Emacs as a normal user 
and then run "cd /sudo::", it doesn't recognize that you're now root, so 
it doesn't prompt.

Similarly, Eshell's "ls" implementation doesn't check for the 
connection-aware UID/username for ownership. And likewise for the "U" 
and "G" Eshell predicate operators.

Attached is a patch to fix this, which also adds a new utility function 
'file-group-gid'. Similar to 'file-user-uid' (added in bug#60722), it's 
just a connection-aware version of 'group-gid'.

[0001-Use-connection-aware-functions-when-getting-the-UID-.patch (text/plain, attachment)]

Message #8 received at 63221 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 63221 <at> debbugs.gnu.org
Subject: Re: bug#63221: 30.0.50;
 [PATCH] Eshell should get user (and group) IDs in a connection-aware
 fashion
Date: Tue, 02 May 2023 14:58:39 +0300

> Date: Mon, 1 May 2023 22:42:53 -0700
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> -(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.

Message #11 received at 63221 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 63221 <at> debbugs.gnu.org
Subject: Re: bug#63221: 30.0.50; [PATCH] Eshell should get user (and group)
 IDs in a connection-aware fashion
Date: Tue, 2 May 2023 11:36:16 -0700

[Message part 1 (text/plain, inline)]

On 5/2/2023 4:58 AM, Eli Zaretskii wrote:
>> Date: Mon, 1 May 2023 22:42:53 -0700
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> -(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.

Thanks, fixed.

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

I changed how this works so now the widget inherits from 'radio' instead 
of 'lazy', and I think it should work better overall now.

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

Fixed.

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

Also fixed.

[0001-Use-connection-aware-functions-when-getting-the-UID-.patch (text/plain, attachment)]

Message #14 received at 63221 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 63221 <at> debbugs.gnu.org
Subject: Re: bug#63221: 30.0.50; [PATCH] Eshell should get user (and group)
 IDs in a connection-aware fashion
Date: Tue, 02 May 2023 21:44:25 +0300

> Date: Tue, 2 May 2023 11:36:16 -0700
> Cc: 63221 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> > 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.
> 
> Thanks, fixed.
> 
> >> +  :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.
> 
> I changed how this works so now the widget inherits from 'radio' instead 
> of 'lazy', and I think it should work better overall now.
> 
> > Same comment to the other similar defcustoms where you changed a
> > boolean option to something else: their doc strings are now
> > obfuscated.
> 
> Fixed.
> 
> >> +(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.
> 
> Also fixed.

Thanks, LGTM.

Message #19 received at 63221-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 63221-done <at> debbugs.gnu.org
Subject: Re: bug#63221: 30.0.50; [PATCH] Eshell should get user (and group)
 IDs in a connection-aware fashion
Date: Tue, 2 May 2023 21:30:14 -0700

On 5/2/2023 11:44 AM, Eli Zaretskii wrote:
> Thanks, LGTM.

Thanks. Merged as 40d66095635; closing this bug now.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.