GNU bug report logs - #30280
async-shell-command-display-buffer doesn't work anymore

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 30280 in the body.
You can then email your comments to 30280 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: Juri Linkov <juri <at> linkov.net>
To: bug-gnu-emacs <at> gnu.org
Subject: async-shell-command-display-buffer doesn't work anymore
Date: Mon, 29 Jan 2018 00:20:30 +0200

0. emacs -Q
1. Eval: (setq async-shell-command-buffer 'new-buffer
               async-shell-command-display-buffer nil)
2. M-& sleep 3600
3. M-& sleep 3; echo "xyzzy"

At the step 2 “*Async Shell Command*” is not shown because of no output,
this is ok.

But at the step 3 “*Async Shell Command*<2>” is not shown after 3 seconds
when the output arrives, this is a regression.

It seems it is caused by commit#9f4f130 from bug#28997

PS: Also it doesn't work with less official configuration:
0. emacs -Q
1. Eval: (setq async-shell-command-display-buffer nil)
         (add-hook 'shell-mode-hook 'rename-uniquely)
2. M-& sleep 3600
3. M-& sleep 3; echo "xyzzy"

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>, "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 30280 <at> debbugs.gnu.org
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Mon, 29 Jan 2018 19:24:38 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Date: Mon, 29 Jan 2018 00:20:30 +0200
> 
> 0. emacs -Q
> 1. Eval: (setq async-shell-command-buffer 'new-buffer
>                async-shell-command-display-buffer nil)
> 2. M-& sleep 3600
> 3. M-& sleep 3; echo "xyzzy"
> 
> At the step 2 “*Async Shell Command*” is not shown because of no output,
> this is ok.
> 
> But at the step 3 “*Async Shell Command*<2>” is not shown after 3 seconds
> when the output arrives, this is a regression.
> 
> It seems it is caused by commit#9f4f130 from bug#28997
> 
> PS: Also it doesn't work with less official configuration:
> 0. emacs -Q
> 1. Eval: (setq async-shell-command-display-buffer nil)
>          (add-hook 'shell-mode-hook 'rename-uniquely)
> 2. M-& sleep 3600
> 3. M-& sleep 3; echo "xyzzy"

Thanks for reporting this.  Basil, can you take a look, please?

P.S. Juri, in the future, could you please make sure the version of
Emacs where you discovered this appears in the bug report, either in
the Subject or in the body?

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

From: Juri Linkov <juri <at> linkov.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: "Basil L. Contovounesios" <contovob <at> tcd.ie>, 30280 <at> debbugs.gnu.org
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Mon, 29 Jan 2018 23:40:52 +0200

> P.S. Juri, in the future, could you please make sure the version of
> Emacs where you discovered this appears in the bug report, either in
> the Subject or in the body?

The problem is that I don't know what version to report:
I discover these bugs in 27.0 in master, but I believe the same
bugs exist also in 26.0, and should be fixed in the release branch.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: contovob <at> tcd.ie, 30280 <at> debbugs.gnu.org
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Tue, 30 Jan 2018 05:24:54 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Cc: "Basil L. Contovounesios" <contovob <at> tcd.ie>,  30280 <at> debbugs.gnu.org
> Date: Mon, 29 Jan 2018 23:40:52 +0200
> 
> > P.S. Juri, in the future, could you please make sure the version of
> > Emacs where you discovered this appears in the bug report, either in
> > the Subject or in the body?
> 
> The problem is that I don't know what version to report:
> I discover these bugs in 27.0 in master, but I believe the same
> bugs exist also in 26.0, and should be fixed in the release branch.

To avoid the dilemma, how about if you run the release branch from now
until it is released?

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Juri Linkov <juri <at> linkov.net>, 30280 <at> debbugs.gnu.org,
 Reuben Thomas <rrt <at> sc3d.org>
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Tue, 30 Jan 2018 18:53:09 +0000

[0001-Fix-deferred-display-of-async-shell-command-buffers.patch (text/x-diff, attachment)]

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

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Juri Linkov <juri <at> linkov.net>
>> Date: Mon, 29 Jan 2018 00:20:30 +0200
>> 
>> 0. emacs -Q
>> 1. Eval: (setq async-shell-command-buffer 'new-buffer
>>                async-shell-command-display-buffer nil)
>> 2. M-& sleep 3600
>> 3. M-& sleep 3; echo "xyzzy"
>> 
>> At the step 2 “*Async Shell Command*” is not shown because of no output,
>> this is ok.
>> 
>> But at the step 3 “*Async Shell Command*<2>” is not shown after 3 seconds
>> when the output arrives, this is a regression.
>> 
>> It seems it is caused by commit#9f4f130 from bug#28997

Commit 9f4f130 indeed touches the relevant code, but the bug was
introduced along with the async-shell-command-display-buffer feature.

>> PS: Also it doesn't work with less official configuration:
>> 0. emacs -Q
>> 1. Eval: (setq async-shell-command-display-buffer nil)
>>          (add-hook 'shell-mode-hook 'rename-uniquely)
>> 2. M-& sleep 3600
>> 3. M-& sleep 3; echo "xyzzy"
>
> Thanks for reporting this.  Basil, can you take a look, please?

I am taking the liberty of CCing Reuben because I believe this report
can be merged with bug#30213.  As justification, a recap and correction
of my diagnosis and solution from that report follows.

The async-shell-command-display-buffer toggle determines whether the
output buffer is displayed immediately or in the process filter, as the
latter should only be executed on process output.  The guard in the
process filter, however, has always required that the buffer (a) be
empty; and (b) have the same name as the original output buffer.

I assume the reasoning behind (a) is that we only want the output buffer
to be displayed when the process first outputs something, and not every
time the process filter is called.  As mentioned in bug#30213, though,
the empty buffer check is The Wrong Thing when
shell-command-dont-erase-buffer is non-nil, i.e. when we want to reuse a
non-empty output buffer left behind by an old shell-command invocation.
AIUI, the process filter advice needs a way to distinguish between the
first time it receives process output and all its subsequent triggers,
irrespective of buffer contents.

I'm not entirely sure what the case for (b) is.  Usually process filters
need only check that their process buffer is live before acting on it;
surely that is also The Right Thing in this case?  The main issue with
(b) is that it does not take into account the various flavours of
async-shell-command-buffer under which the output buffer is renamed, as
demonstrated in this bug report.  I think the simplest fix for (b) would
be something like

[foo.diff (text/x-diff, inline)]

diff --git a/lisp/simple.el b/lisp/simple.el
index 3ac6b86381..4f6708dbc1 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3552,8 +3552,7 @@ shell-command
                                   (lambda (process _string)
                                     (let ((buf (process-buffer process)))
                                       (when (and (zerop (buffer-size buf))
-                                                 (string= (buffer-name buf)
-                                                          bname))
+                                                 (eq buf buffer))
                                         (display-buffer buf))))))))
 	    ;; Otherwise, command is executed synchronously.
 	    (shell-command-on-region (point) (point) command

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

but I still don't see why we are forgoing a liveness check in favour of
this.

I attach a patch which addresses both bugs.  Its solution for (a) is to
make the advice disposable, i.e. it removes itself from the process
filter after it has fulfilled its purpose of displaying the output
buffer.  A syntactically simpler implementation of this could use a
plain boolean switch instead of removing the advice, but IMO the latter
is semantically more sound (and possibly more performant in subsequent
invocations of the process filter, though this should be irrelevant).

WDYT?

P.S. Would patch(es) for aesthetic changes to the rest of shell-command
(such as removing redundant variables, inverting the condition of the
massive if-then-else to reduce indentation, etc.) be welcome?  If so,
where should I send them?

-- 
Basil

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

From: Reuben Thomas <rrt <at> sc3d.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 30280 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Tue, 30 Jan 2018 19:06:19 +0000

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

2018-01-30 18:53 GMT+00:00 Basil L. Contovounesios <contovob <at> tcd.ie>:

>
> I am taking the liberty of CCing Reuben because I believe this report
> can be merged with bug#30213.  As justification, a recap and correction
> of my diagnosis and solution from that report follows.
>
> The async-shell-command-display-buffer toggle determines whether the
> output buffer is displayed immediately or in the process filter, as the
> latter should only be executed on process output.  The guard in the
> process filter, however, has always required that the buffer (a) be
> empty; and (b) have the same name as the original output buffer.
>

​I think you're right that (b) is not necessary. I think I did it that way
originally because I was doing everything with the buffer name, so really
it was just an oversight.​

-- 
https://rrt.sc3d.org

[Message part 2 (text/html, inline)]

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

From: Juri Linkov <juri <at> linkov.net>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 30280 <at> debbugs.gnu.org,
 Reuben Thomas <rrt <at> sc3d.org>
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Wed, 31 Jan 2018 23:44:43 +0200

> I attach a patch which addresses both bugs.  Its solution for (a) is to
> make the advice disposable, i.e. it removes itself from the process
> filter after it has fulfilled its purpose of displaying the output
> buffer.  A syntactically simpler implementation of this could use a
> plain boolean switch instead of removing the advice, but IMO the latter
> is semantically more sound (and possibly more performant in subsequent
> invocations of the process filter, though this should be irrelevant).
>
> WDYT?

Thanks, I confirm this is the right thing to do and your patch fixes
the reported issue.

> P.S. Would patch(es) for aesthetic changes to the rest of shell-command
> (such as removing redundant variables, inverting the condition of the
> massive if-then-else to reduce indentation, etc.) be welcome?  If so,
> where should I send them?

Such changes are always welcome as patches separate from the actual fixes,
i.e. when not amalgamating bug fixes and code beautification/refactoring
in the same patch.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: contovob <at> tcd.ie, 30280 <at> debbugs.gnu.org, rrt <at> sc3d.org
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Fri, 02 Feb 2018 12:42:14 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  Reuben Thomas <rrt <at> sc3d.org>,  <30280 <at> debbugs.gnu.org>
> Date: Wed, 31 Jan 2018 23:44:43 +0200
> 
> > I attach a patch which addresses both bugs.  Its solution for (a) is to
> > make the advice disposable, i.e. it removes itself from the process
> > filter after it has fulfilled its purpose of displaying the output
> > buffer.  A syntactically simpler implementation of this could use a
> > plain boolean switch instead of removing the advice, but IMO the latter
> > is semantically more sound (and possibly more performant in subsequent
> > invocations of the process filter, though this should be irrelevant).
> >
> > WDYT?
> 
> Thanks, I confirm this is the right thing to do and your patch fixes
> the reported issue.

Then please push them to the emacs-26 branch, and thanks.

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: rrt <at> sc3d.org, 30280 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Sat, 03 Feb 2018 14:13:15 +0000

[0001-lisp-simple.el-async-shell-command-shell-command-Fix.patch (text/x-diff, attachment)]

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

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Thanks, I confirm this is the right thing to do and your patch fixes
>> the reported issue.
>
> Then please push them to the emacs-26 branch, and thanks.

Perhaps I can sneak in the attached doc fixes as well.

-- 
Basil

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

From: Juri Linkov <juri <at> linkov.net>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 30280-done <at> debbugs.gnu.org, rrt <at> sc3d.org
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Sat, 03 Feb 2018 23:27:51 +0200

>>> Thanks, I confirm this is the right thing to do and your patch fixes
>>> the reported issue.
>>
>> Then please push them to the emacs-26 branch, and thanks.

Done.

> Perhaps I can sneak in the attached doc fixes as well.

Thanks, this is pushed as well.

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Juri Linkov <juri <at> linkov.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, tino calancha <tino.calancha <at> gmail.com>,
 30280 <at> debbugs.gnu.org, rrt <at> sc3d.org
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Sun, 06 May 2018 17:18:32 +0100

[0001-Fix-failing-test-for-bug-30280.patch (text/x-diff, attachment)]

[0002-Minor-shell-command-simplifications.patch (text/x-diff, attachment)]

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

I recently noticed that a test for an expected failure was added around
the time of this bug report[1: ea8c0e1b9e].

[1: ea8c0e1b9e]: 2018-01-29 22:31:50 +0900
  * test/lisp/simple-tests.el (simple-tests-async-shell-command-30280): Add test
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=ea8c0e1b9eaa6651919fb4e039e3fcb5a1fa73db

I attach two patches.  The first tries to make this test succeed in
accordance with the resulting bugfix.  The second suggests some
simplifications to the logic in shell-command.  WDYT?

Thanks,

-- 
Basil

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Basil L. Contovounesios <contovob <at> tcd.ie>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Juri Linkov <juri <at> linkov.net>,
 tino.calancha <at> gmail.com, 30280 <at> debbugs.gnu.org, rrt <at> sc3d.org
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Mon, 07 May 2018 16:35:03 +0900

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> I attach two patches.  The first tries to make this test succeed in
> accordance with the resulting bugfix.  The second suggests some
> simplifications to the logic in shell-command.  WDYT?

Basil, thank you for fixing that test!

Concerning the 2nd patch; it adds a bug when
`async-shell-command-buffer' is
confirm-rename-buffer
;; or
rename-buffer

Following is a recipe:

emacs -Q -eval "(setq async-shell-command-buffer 'rename-buffer)" \
-eval '(async-shell-command "while true; do echo foo; sleep 3;done")' \
-eval '(async-shell-command "while true; do echo bar; sleep 3;done")'

;; Now execute the following form:
(cdr 
  (delq nil
     (mapcar (lambda (b) (if (string-match "\*Async Shell Command\*" (buffer-name b)) (buffer-name b)))(buffer-list))))
=> nil
;; It shouldn't be nil: there must be 2 buffers matching '*Async Shell Command*'.
;; Here we are using 1 buffer for 2 shell processes.

;; The original code binds bname below so that we can create the new
;; buffer with the proper name:
(let* ((buffer (get-buffer-create
                (or output-buffer "*Async Shell Command*")))
       (bname (buffer-name buffer))
       (directory default-directory)
       proc)

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Juri Linkov <juri <at> linkov.net>,
 30280 <at> debbugs.gnu.org, rrt <at> sc3d.org
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Wed, 09 May 2018 12:54:51 +0100

[0001-Fix-failing-test-for-bug-30280.patch (text/x-diff, attachment)]

[0002-Minor-shell-command-simplifications.patch (text/x-diff, attachment)]

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

Tino Calancha <tino.calancha <at> gmail.com> writes:

> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>
>> I attach two patches.  The first tries to make this test succeed in
>> accordance with the resulting bugfix.  The second suggests some
>> simplifications to the logic in shell-command.  WDYT?
>
> Basil, thank you for fixing that test!
>
> Concerning the 2nd patch; it adds a bug when
> `async-shell-command-buffer' is
> confirm-rename-buffer
> ;; or
> rename-buffer

Right, that was silly of me; thanks for catching it.
I reattach the patches with the buffer name changes removed.

Thanks again,

-- 
Basil

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: <contovob <at> tcd.ie>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Juri Linkov <juri <at> linkov.net>,
 30280 <at> debbugs.gnu.org, rrt <at> sc3d.org
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Wed, 09 May 2018 22:57:30 +0900

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> I reattach the patches with the buffer name changes removed.
Thank you Basil!

IMHO this patch looks OK.
I have two minor comments.

I)
> +(declare-function comint-output-filter "comint" (process string))
> +
What is the purpose of this? AFICT no warning is shown when compiling
the file.
* We require `shell.el' inside `shell-coomand'.
* `shell.el' requires `comint.el'.
Is the purpose to serve as documentation? In that case I don't think we
need it (the prefix 'comint-' already makes obvious where this function
belongs to).

II)
It's better to keep consistent with the indentation of the function you
are modifying:  here, `shell-command' is indenting with TAB.

Tip:
You can see the tabs searching them with:
C-s C-q C-I
or you can persistenly highlight them with:
M-s h r C-I RET RET

For instance, here you are changing:
1) ' ---> #'
;; and
2) \t\t\s\s 000> \s\s\s\s...\s (18 white spaces)

Please, do not change 2).
>  		  ;; Use the comint filter for proper handling of
>  		  ;; carriage motion (see comint-inhibit-carriage-motion).
> -		  (set-process-filter proc 'comint-output-filter)
> +                  (set-process-filter proc #'comint-output-filter)

>                    (if async-shell-command-display-buffer
>                        ;; Display buffer immediately.
>                        (display-buffer buffer '(nil (allow-no-window . t)))

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

From: Noam Postavsky <npostavs <at> gmail.com>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: "Basil L. Contovounesios" <contovob <at> tcd.ie>, Juri Linkov <juri <at> linkov.net>,
 30280 <at> debbugs.gnu.org, Reuben Thomas <rrt <at> sc3d.org>
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Wed, 9 May 2018 10:10:21 -0400

On 9 May 2018 at 09:57, Tino Calancha <tino.calancha <at> gmail.com> wrote:

> II)
> It's better to keep consistent with the indentation of the function you
> are modifying

> For instance, here you are changing:
> 1) ' ---> #'
> ;; and
> 2) \t\t\s\s 000> \s\s\s\s...\s (18 white spaces)

AFAIK, current policy is that it's fine to change tabs to spaces when
the line is being modified anyway.

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: "Basil L. Contovounesios" <contovob <at> tcd.ie>, Juri Linkov <juri <at> linkov.net>,
 Reuben Thomas <rrt <at> sc3d.org>, 30280 <at> debbugs.gnu.org,
 Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Wed, 9 May 2018 23:24:24 +0900 (JST)

On Wed, 9 May 2018, Noam Postavsky wrote:

> On 9 May 2018 at 09:57, Tino Calancha <tino.calancha <at> gmail.com> wrote:
>
>> II)
>> It's better to keep consistent with the indentation of the function you
>> are modifying
>
>> For instance, here you are changing:
>> 1) ' ---> #'
>> ;; and
>> 2) \t\t\s\s 000> \s\s\s\s...\s (18 white spaces)
>
> AFAIK, current policy is that it's fine to change tabs to spaces when
> the line is being modified anyway.
Thank you.  I didn't know it.
IMO, not very nice to make a 'Frankestein' indented function, but ... OK.

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

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Juri Linkov <juri <at> linkov.net>,
 Noam Postavsky <npostavs <at> gmail.com>, 30280 <at> debbugs.gnu.org, rrt <at> sc3d.org
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Wed, 09 May 2018 19:29:25 +0100

Tino Calancha <tino.calancha <at> gmail.com> writes:

> I have two minor comments.
>
> I)
>> +(declare-function comint-output-filter "comint" (process string))
>> +
> What is the purpose of this? AFICT no warning is shown when compiling
> the file.

On my end, removing the function declaration and invoking 'make' results
in the following output:

...
make[2]: Entering directory '/home/blc/.local/src/emacs/lisp'
  ELC      ../lisp/simple.elc
Reloading stale loaddefs.el
Loading /home/blc/.local/src/emacs/lisp/loaddefs.el (source)...

In end of data:
simple.el:9030:1:Warning: the function ‘comint-output-filter’ is not known to
    be defined.
...

Note that this warning is only emitted when comint-output-filter is
#'-quoted.  This is in line with the usual behaviour of the
byte-compiler that I am accustomed to, i.e. I don't see anything out of
the ordinary here.

> * We require `shell.el' inside `shell-coomand'.
> * `shell.el' requires `comint.el'.

Yes, I understand that using comint-output-filter at this point in the
program is kosher, but the byte-compiler evidently does not.

> Is the purpose to serve as documentation? In that case I don't think we
> need it (the prefix 'comint-' already makes obvious where this function
> belongs to).

No, the only intention is to pacify the byte-compiler.

> II)
> It's better to keep consistent with the indentation of the function you
> are modifying:  here, `shell-command' is indenting with TAB.
>
> Tip:
> You can see the tabs searching them with:
> C-s C-q C-I
> or you can persistenly highlight them with:
> M-s h r C-I RET RET

Thanks for the tip.  I personally prefer to use [global-]whitespace-mode
with a whitespace-style setting which includes (face tab-mark).
I additionally avoid accidentally committing tabs by configuring the git
option core.whitespace to include tab-in-indent.

> For instance, here you are changing:
> 1) ' ---> #'
> ;; and
> 2) \t\t\s\s 000> \s\s\s\s...\s (18 white spaces)
>
> Please, do not change 2).

I have no strong feeling on this; I was merely going along with the
(emacs-lisp-mode . ((indent-tabs-mode . nil))) setting in the project's
toplevel dir-locals-file, as well what I had inferred to be accepted
policy (as Noam mentions in a separate email) from following Emacs
development for the last couple of years.

If it weren't for the above and the fact that most everything I have
come across in the Emacs tree has Frankindentation (including the target
function shell-command), I would be more inclined to remain consistent
with the surrounding source.

Let me know if it's still a problem and I'll gladly resend the patches
with indent-tabs-mode enabled.

Thanks again for your help and feedback,

-- 
Basil

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: contovob <at> tcd.ie
Cc: 30280 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>,
 rrt <at> sc3d.org, Noam Postavsky <npostavs <at> gmail.com>,
 Juri Linkov <juri <at> linkov.net>, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#30280: async-shell-command-display-buffer doesn't work anymore
Date: Thu, 10 May 2018 11:13:44 +0900 (JST)

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

Thank you Basil for your prompt response!

Your patches LGTM.
Let's wait few more days if some people want to make further comments.
Otherwise, I will push them in your name into the master branch.

On Wed, 9 May 2018, Basil L. Contovounesios wrote:

>> I)
>>> +(declare-function comint-output-filter "comint" (process string))
>>> +
>> What is the purpose of this? AFICT no warning is shown when compiling
>> the file.
>
> On my end, removing the function declaration and invoking 'make' results
> in the following output:
>
> ...
> make[2]: Entering directory '/home/blc/.local/src/emacs/lisp'
>  ELC      ../lisp/simple.elc
> Reloading stale loaddefs.el
> Loading /home/blc/.local/src/emacs/lisp/loaddefs.el (source)...
>
> In end of data:
> simple.el:9030:1:Warning: the function ‘comint-output-filter’ is not known to
>    be defined.
> ...
>
> Note that this warning is only emitted when comint-output-filter is
> #'-quoted.  This is in line with the usual behaviour of the
> byte-compiler that I am accustomed to, i.e. I don't see anything out of
> the ordinary here.
Oh, I see.  I overlooked the fact that we changed `quote' with `function'.
With:
make bootstrap
there is no warning.
I am OK with clearing the warning in all cases.


>> * We require `shell.el' inside `shell-coomand'.
>> * `shell.el' requires `comint.el'.
>
> Yes, I understand that using comint-output-filter at this point in the
> program is kosher, but the byte-compiler evidently does not.
>
>> Is the purpose to serve as documentation? In that case I don't think we
>> need it (the prefix 'comint-' already makes obvious where this function
>> belongs to).
>
> No, the only intention is to pacify the byte-compiler.
>
>> II)
>> It's better to keep consistent with the indentation of the function you
>> are modifying:  here, `shell-command' is indenting with TAB.
>>
>> Tip:
>> You can see the tabs searching them with:
>> C-s C-q C-I
>> or you can persistenly highlight them with:
>> M-s h r C-I RET RET
>
> Thanks for the tip.  I personally prefer to use [global-]whitespace-mode
> with a whitespace-style setting which includes (face tab-mark).
> I additionally avoid accidentally committing tabs by configuring the git
> option core.whitespace to include tab-in-indent.
>
>> For instance, here you are changing:
>> 1) ' ---> #'
>> ;; and
>> 2) \t\t\s\s 000> \s\s\s\s...\s (18 white spaces)
>>
>> Please, do not change 2).
>
> I have no strong feeling on this; I was merely going along with the
> (emacs-lisp-mode . ((indent-tabs-mode . nil))) setting in the project's
> toplevel dir-locals-file, as well what I had inferred to be accepted
> policy (as Noam mentions in a separate email) from following Emacs
> development for the last couple of years.
>
> If it weren't for the above and the fact that most everything I have
> come across in the Emacs tree has Frankindentation (including the target
> function shell-command), I would be more inclined to remain consistent
> with the surrounding source.
>
> Let me know if it's still a problem and I'll gladly resend the patches
> with indent-tabs-mode enabled.
It's OK, Frankenstein is a glorious novel after all; I recommend anyone to 
revisit such classical masterpiece :-)

Previous Next

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