GNU bug report logs - #53517
29.0.50; [PATCH] `eshell-eval-using-options' :preserve-args breaks :external handling in some cases

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 53517 in the body.
You can then email your comments to 53517 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: 29.0.50; [PATCH] `eshell-eval-using-options' :preserve-args breaks
 :external handling in some cases
Date: Mon, 24 Jan 2022 17:11:27 -0800

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

To see this in action:

  $ touch foo
  $ emacs -Q
  M-x eshell
  cp foo bar -s
  ls -l bar

`bar' should be a symlink pointing to `foo', but it's just a regular file.

This happens because, when :preserve-args is true, 
`eshell-eval-using-options' doesn't flatten and stringify the args to be 
parsed. That results in the various helper functions modifying the list 
of raw args in-place. When :external is set, if `eshell--process-option' 
finds an option it doesn't recognize, it throws, telling 
`eshell--do-opts' to run the external command instead. That requires 
having the original args, un-tampered with, in order to forward on to 
the external command.

Attached is a patch that fixes this by copying the raw args list for the 
:preserve-args case before manipulating it. I considered only adding 
this when :external is also set, but I think it's useful for callers to 
be able to inspect the raw args afterwards. This is used by `eshell/su' 
to check for the presence of a "-" argument, since 
`eshell-eval-using-options' doesn't handle that. (It might be nice to 
fix that too, but I think it's still useful to keep the original raw 
args around unchanged. Improving "-" support could be done later.)

As part of this, I also added several unit tests for `eshell/su' and 
`eshell/sudo' to make sure I didn't break anything there; these should 
pass both before and after the rest of the patch. I also added tests to 
make sure the original arguments are preserved for calling the external 
command; these should fail before the patch, and pass after.

There's just one remaining issue that I'm not sure how to fix: since I 
updated the `eshell-eval-using-options' macro, any file that uses it 
needs to be recompiled to get the new version. What's the right way to 
tell the build system about this? I just ran `touch lisp/eshell/*.el' 
locally, but I'm sure there's a better way so that people can just run 
`make' after pulling this patch.

[0001-Don-t-manipulate-args-in-place-for-eshell-eval-using.patch (text/plain, attachment)]

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: 53517 <at> debbugs.gnu.org
Subject: Re: bug#53517: 29.0.50; [PATCH] `eshell-eval-using-options'
 :preserve-args breaks :external handling in some cases
Date: Mon, 24 Jan 2022 21:19:52 -0800

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

On 1/24/2022 5:11 PM, Jim Porter wrote:
> (It might be nice to fix that too, but I think it's still useful to
> keep the original raw args around unchanged. Improving "-" support
> could be done later.)

Actually, I'll do that now. Doing so fixes an issue I found in the 
implementation of `eshell/cat'. "cat -" should read from stdin (and the 
Eshell implementation should use the external /bin/cat to do this), but 
it was getting parsed as "cat", so it erroneously thought there were 
*no* input files. Previously, `eshell--process-args' saw the "-" and 
started to look for short options, found none, and then just shrugged 
and ignored that token entirely.

I also fixed an issue with the first patch where I had some misplaced 
parens in test/lisp/eshell/esh-opt-tests.el.

[0001-Don-t-manipulate-args-in-place-for-eshell-eval-using.patch (text/plain, attachment)]

[0002-Treat-as-a-positional-arg-in-eshell-eval-using-optio.patch (text/plain, attachment)]

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: 53517 <at> debbugs.gnu.org
Subject: Re: bug#53517: 29.0.50; [PATCH] `eshell-eval-using-options'
 :preserve-args breaks :external handling in some cases
Date: Mon, 24 Jan 2022 22:29:20 -0800

On 1/24/2022 9:19 PM, Jim Porter wrote:
> On 1/24/2022 5:11 PM, Jim Porter wrote:
>> (It might be nice to fix that too, but I think it's still useful to
>> keep the original raw args around unchanged. Improving "-" support
>> could be done later.)
> 
> Actually, I'll do that now. Doing so fixes an issue I found in the 
> implementation of `eshell/cat'. "cat -" should read from stdin (and the 
> Eshell implementation should use the external /bin/cat to do this), but 
> it was getting parsed as "cat", so it erroneously thought there were 
> *no* input files.

Just a note: `cat' with no args *should* read from stdin, but this 
doesn't actually work in Eshell yet; to read from stdin, you need to 
explicitly say "cat -" or "*cat". I plan to fix these bits in a separate 
bug, since I have a WIP patch series to allow piping to Lisp functions 
in Eshell. That involves rewriting most of `eshell/cat', so I figured I 
may as well just fix it once (later) instead of twice.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 53517 <at> debbugs.gnu.org
Subject: Re: bug#53517: 29.0.50; [PATCH] `eshell-eval-using-options'
 :preserve-args breaks :external handling in some cases
Date: Tue, 25 Jan 2022 13:30:50 +0100

Jim Porter <jporterbugs <at> gmail.com> writes:

> Actually, I'll do that now. Doing so fixes an issue I found in the
> implementation of `eshell/cat'. "cat -" should read from stdin (and
> the Eshell implementation should use the external /bin/cat to do
> this), but it was getting parsed as "cat", so it erroneously thought
> there were *no* input files. Previously, `eshell--process-args' saw
> the "-" and started to look for short options, found none, and then
> just shrugged and ignored that token entirely.
>
> I also fixed an issue with the first patch where I had some misplaced
> parens in test/lisp/eshell/esh-opt-tests.el.

Thanks; pushed to Emacs 28.

Jim Porter <jporterbugs <at> gmail.com> writes:

> There's just one remaining issue that I'm not sure how to fix: since I
> updated the `eshell-eval-using-options' macro, any file that uses it
> needs to be recompiled to get the new version. What's the right way to
> tell the build system about this? I just ran `touch lisp/eshell/*.el'
> locally, but I'm sure there's a better way so that people can just run
> `make' after pulling this patch.

We unfortunately don't have any system for handling build issues like
this, so people just have to say "make bootstrap" (or touch .el files or
delete .elc files) in these cases.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

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