GNU bug report logs - #36034
[PATCH] Zsh extended_history shows up in comint input ring

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 36034 in the body.
You can then email your comments to 36034 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: Matthew Bauer <mjbauer95 <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Zsh extended_history shows up in comint input ring
Date: Fri, 31 May 2019 16:30:44 -0400

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

Currently, Zsh’s extended_history option is not handled well in Emacs.
The comint buffer does not know to skip it when running
comint-read-input-ring. The attached patch handles this.

This behavior is described in the Zsh manual available at:

http://zsh.sourceforge.net/Doc/Release/Options.html#History

The format of this line looks like this:

: <beginning time>:<elapsed seconds>;<command>

This patch just skips those timestamp to get the <command> part.

[0001-Add-zsh-extended_history-handling-for-comint.el-inpu.patch (text/x-patch, inline)]

From b8a8857cd686fae1ebbeca79f4469ce878837b90 Mon Sep 17 00:00:00 2001
From: Matthew Bauer <mjbauer95 <at> gmail.com>
Date: Fri, 31 May 2019 16:27:24 -0400
Subject: [PATCH] Add zsh extended_history handling for comint.el input ring
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Adds handling of the Zsh extended_history to comint.el input
ring. This means that the timestamp doesn’t show up when reading
through history from other shells. The lines look like this:

: <beginning time>:<elapsed seconds>;<command>

This patch skips the part before <command>.

Zsh documents it here:

http://zsh.sourceforge.net/Doc/Release/Options.html#History
---
 lisp/comint.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 3dce1c9c8d..c5c0ad0f7b 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -976,7 +976,11 @@ See also `comint-input-ignoredups' and `comint-write-input-ring'."
                  (setq start
                        (if (re-search-backward comint-input-ring-separator
                                                nil t)
-                           (match-end 0)
+                           (progn
+                             ;; Skip zsh extended_history stamps
+                             (re-search-forward ": [[:digit:]]+:[[:digit:]]+;" nil t)
+
+                             (match-end 0))
                          (point-min)))
                  (setq history (buffer-substring start end))
                  (goto-char start)
-- 
2.21.0

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Matthew Bauer <mjbauer95 <at> gmail.com>
Cc: 36034 <at> debbugs.gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint input
 ring
Date: Sun, 23 Jun 2019 18:53:54 +0200

Matthew Bauer <mjbauer95 <at> gmail.com> writes:

> The format of this line looks like this:
>
> : <beginning time>:<elapsed seconds>;<command>
>
> This patch just skips those timestamp to get the <command> part.

[...]

> --- a/lisp/comint.el
> +++ b/lisp/comint.el
> @@ -976,7 +976,11 @@ See also `comint-input-ignoredups' and `comint-write-input-ring'."
>                   (setq start
>                         (if (re-search-backward comint-input-ring-separator
>                                                 nil t)
> -                           (match-end 0)
> +                           (progn
> +                             ;; Skip zsh extended_history stamps
> +                             (re-search-forward ": [[:digit:]]+:[[:digit:]]+;" nil t)
> +
> +                             (match-end 0))
>                           (point-min)))
>                   (setq history (buffer-substring start end))
>                   (goto-char start)

I'm not that familiar with the comint/shell code...  but this is done
in the central comint code, so it would do this for all the modes that
use comint?  Couldn't that lead to problems in these other modes that
aren't doing this timestamp thing?

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

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

From: Matthew Bauer <mjbauer95 <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint input
 ring
Date: Sun, 23 Jun 2019 18:27:59 -0400

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

> On Jun 23, 2019, at 12:53 PM, Lars Ingebrigtsen <larsi <at> gnus.org> wrote:
> 
> I'm not that familiar with the comint/shell code...  but this is done
> in the central comint code, so it would do this for all the modes that
> use comint?  Couldn't that lead to problems in these other modes that
> aren't doing this timestamp thing?

Yes, it does apply for any comint buffer that uses comint-read-input-ring. i had originally thought this was only used by shell-mode, but you are correct that it is used by others. I’ve added defcustom handling to the patch to properly account for this. In addition, I have made some revisions to better account for the extended history prefix. Any additional help on this would be appreciate!

[0001-Add-zsh-extended_history-handling-for-comint.el-inpu.patch (application/octet-stream, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Matthew Bauer <mjbauer95 <at> gmail.com>
Cc: 36034 <at> debbugs.gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint input
 ring
Date: Mon, 24 Jun 2019 12:51:08 +0200

Matthew Bauer <mjbauer95 <at> gmail.com> writes:

> Yes, it does apply for any comint buffer that uses
> comint-read-input-ring. i had originally thought this was only used by
> shell-mode, but you are correct that it is used by others. Ive added
> defcustom handling to the patch to properly account for this.

Looks less dangerous now.  :-)

Some comments:

> +(defcustom comint-input-ring-file-prefix nil
> +  "If non-nil, the prefix to skip when parsing the input ring file.
> +This is useful in Zsh when the extended_history option is on."
> +  :type 'boolean
> +  :group 'comint)

This doesn't really seem like a user-level variable, so it should just
be a defvar, I think.

>                   (setq start
>                         (if (re-search-backward comint-input-ring-separator
>                                                 nil t)
> -                           (match-end 0)
> -                         (point-min)))
> +                           (progn
> +                             (when (and
> +                                    comint-input-ring-file-prefix
> +                                    (looking-at (concat comint-input-ring-separator
> +                                                        comint-input-ring-file-prefix)))
> +                               ;; Skip zsh extended_history stamps
> +                               (re-search-forward comint-input-ring-file-prefix
> +                                                  nil t))
> +                             (match-end 0))

The re-search-forward here doesn't seem necessary -- can't you just go
to (match-end 0) here instead?

> +                         (progn
> +                           (goto-char (point-min))
> +                           (if (and comint-input-ring-file-prefix
> +                                    (looking-at comint-input-ring-file-prefix))
> +                               (progn
> +                                 (re-search-forward comint-input-ring-file-prefix
> +                                                    nil t)
> +                                 (match-end 0))
> +                             (point-min)))))

And I don't understand this bit.  This is when we didn't find
comint-input-ring-separator, right?  But you still want to skip
comint-input-ring-file-prefix?

If you want to skip it anyway, then you can just have the check (and the
skip) after the if statement...

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

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Matthew Bauer <mjbauer95 <at> gmail.com>
Cc: 36034 <at> debbugs.gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint input
 ring
Date: Mon, 24 Jun 2019 12:51:08 +0200

Matthew Bauer <mjbauer95 <at> gmail.com> writes:

> Yes, it does apply for any comint buffer that uses
> comint-read-input-ring. i had originally thought this was only used by
> shell-mode, but you are correct that it is used by others. Ive added
> defcustom handling to the patch to properly account for this.

Looks less dangerous now.  :-)

Some comments:

> +(defcustom comint-input-ring-file-prefix nil
> +  "If non-nil, the prefix to skip when parsing the input ring file.
> +This is useful in Zsh when the extended_history option is on."
> +  :type 'boolean
> +  :group 'comint)

This doesn't really seem like a user-level variable, so it should just
be a defvar, I think.

>                   (setq start
>                         (if (re-search-backward comint-input-ring-separator
>                                                 nil t)
> -                           (match-end 0)
> -                         (point-min)))
> +                           (progn
> +                             (when (and
> +                                    comint-input-ring-file-prefix
> +                                    (looking-at (concat comint-input-ring-separator
> +                                                        comint-input-ring-file-prefix)))
> +                               ;; Skip zsh extended_history stamps
> +                               (re-search-forward comint-input-ring-file-prefix
> +                                                  nil t))
> +                             (match-end 0))

The re-search-forward here doesn't seem necessary -- can't you just go
to (match-end 0) here instead?

> +                         (progn
> +                           (goto-char (point-min))
> +                           (if (and comint-input-ring-file-prefix
> +                                    (looking-at comint-input-ring-file-prefix))
> +                               (progn
> +                                 (re-search-forward comint-input-ring-file-prefix
> +                                                    nil t)
> +                                 (match-end 0))
> +                             (point-min)))))

And I don't understand this bit.  This is when we didn't find
comint-input-ring-separator, right?  But you still want to skip
comint-input-ring-file-prefix?

If you want to skip it anyway, then you can just have the check (and the
skip) after the if statement...

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

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

From: Matthew Bauer <mjbauer95 <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 36034 <at> debbugs.gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint input
 ring
Date: Mon, 24 Jun 2019 18:45:51 -0400

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

> On Jun 24, 2019, at 6:51 AM, Lars Ingebrigtsen <larsi <at> gnus.org> wrote:
> 
> Matthew Bauer <mjbauer95 <at> gmail.com> writes:
> 
>> +(defcustom comint-input-ring-file-prefix nil
>> +  "If non-nil, the prefix to skip when parsing the input ring file.
>> +This is useful in Zsh when the extended_history option is on."
>> +  :type 'boolean
>> +  :group 'comint)
> 
> This doesn't really seem like a user-level variable, so it should just
> be a defvar, I think.

Seems reasonable, I updated the patch.

> 
>>                  (setq start
>>                        (if (re-search-backward comint-input-ring-separator
>>                                                nil t)
>> -                           (match-end 0)
>> -                         (point-min)))
>> +                           (progn
>> +                             (when (and
>> +                                    comint-input-ring-file-prefix
>> +                                    (looking-at (concat comint-input-ring-separator
>> +                                                        comint-input-ring-file-prefix)))
>> +                               ;; Skip zsh extended_history stamps
>> +                               (re-search-forward comint-input-ring-file-prefix
>> +                                                  nil t))
>> +                             (match-end 0))
> 
> The re-search-forward here doesn't seem necessary -- can't you just go
> to (match-end 0) here instead?

Without re-search-forward, the “start” integer would just be the character right after the newline. re-search-forward skips that prefix.

>> +                         (progn
>> +                           (goto-char (point-min))
>> +                           (if (and comint-input-ring-file-prefix
>> +                                    (looking-at comint-input-ring-file-prefix))
>> +                               (progn
>> +                                 (re-search-forward comint-input-ring-file-prefix
>> +                                                    nil t)
>> +                                 (match-end 0))
>> +                             (point-min)))))
> 
> And I don't understand this bit.  This is when we didn't find
> comint-input-ring-separator, right?  But you still want to skip
> comint-input-ring-file-prefix?
> 
> If you want to skip it anyway, then you can just have the check (and the
> skip) after the if statement…
> 

This is for the very first line which will not have a newline before it. In this case, we want to skip the prefix as well. This is part of why we need special handling for “prefix" and tricks like this don’t work:

https://emacs.stackexchange.com/a/22295 <https://emacs.stackexchange.com/a/22295>

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

[0001-Add-zsh-extended_history-handling-for-comint.el-inpu.patch (application/octet-stream, attachment)]

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

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Matthew Bauer <mjbauer95 <at> gmail.com>
Cc: 36034 <at> debbugs.gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint input
 ring
Date: Tue, 25 Jun 2019 13:05:22 +0200

Matthew Bauer <mjbauer95 <at> gmail.com> writes:

>  -                           (match-end 0)
>  -                         (point-min)))
>  +                           (progn
>  +                             (when (and
>  +                                    comint-input-ring-file-prefix
>  +                                    (looking-at (concat
>  comint-input-ring-separator
>  +                                                        comint-input-ring-file-prefix)))
>  +                               ;; Skip zsh extended_history stamps
>  +                               (re-search-forward comint-input-ring-file-prefix
>  +                                                  nil t))
>  +                             (match-end 0))
>
>  The re-search-forward here doesn't seem necessary -- can't you just go
>  to (match-end 0) here instead?
>
> Without re-search-forward, the “start” integer would just be the character
> right after the newline. re-search-forward skips that prefix.

You first do a

(looking-at (concat comint-input-ring-separator comint-input-ring-file-prefix)))

and then

(re-search-forward comint-input-ring-file-prefix)

This will make point end up at where (match-end 0) would be after the
looking-at, surely?

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

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

From: Matthew Bauer <mjbauer95 <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 36034 <at> debbugs.gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint input
 ring
Date: Sat, 29 Jun 2019 17:35:19 -0400

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

> On Jun 25, 2019, at 7:05 AM, Lars Ingebrigtsen <larsi <at> gnus.org> wrote:

> This will make point end up at where (match-end 0) would be after the
> looking-at, surely?
> 

Ok yeah that simplified things a bit. Attached an updated version.

[0001-Add-zsh-extended_history-handling-for-comint.el-inpu.patch (application/octet-stream, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Matthew Bauer <mjbauer95 <at> gmail.com>
Cc: 36034 <at> debbugs.gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint input
 ring
Date: Thu, 04 Jul 2019 15:37:29 +0200

Matthew Bauer <mjbauer95 <at> gmail.com> writes:

>                   (setq start
>                         (if (re-search-backward comint-input-ring-separator
>                                                 nil t)
> -                           (match-end 0)
> -                         (point-min)))
> +                           (progn
> +                             (when comint-input-ring-file-prefix
> +                               ;; Skip zsh extended_history stamps
> +                               (re-search-forward comint-input-ring-file-prefix
> +                                                  nil t))
> +                             (match-end 0))

Hm...  I don't think this is right, either.  If the re-search-forward
fails, then (match-end 0) will fail, too.  And since (if I understood
correctly), the prefix will follow on directly from where point it,
using looking-at would be better, anyway...

>        ;; Bypass a bug in certain versions of bash.
>        (when (string-equal shell "bash")
>          (add-hook 'comint-preoutput-filter-functions
> -                  'shell-filter-ctrl-a-ctrl-b nil t)))
> +                  'shell-filter-ctrl-a-ctrl-b nil t))
> +
> +      ;; Skip extended history for zsh.
> +      (when (string-equal shell "zsh")
> +        (setq-local comint-input-ring-file-prefix ": [[:digit:]]+:[[:digit:]]+;")))
>      (comint-read-input-ring t)))

And this bit didn't apply.

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

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Matthew Bauer <mjbauer95 <at> gmail.com>, 36034 <at> debbugs.gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint
 input ring
Date: Mon, 20 Jan 2020 20:43:51 +0100

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Matthew Bauer <mjbauer95 <at> gmail.com> writes:
>
>>                   (setq start
>>                         (if (re-search-backward comint-input-ring-separator
>>                                                 nil t)
>> -                           (match-end 0)
>> -                         (point-min)))
>> +                           (progn
>> +                             (when comint-input-ring-file-prefix
>> +                               ;; Skip zsh extended_history stamps
>> +                               (re-search-forward comint-input-ring-file-prefix
>> +                                                  nil t))
>> +                             (match-end 0))
>
> Hm...  I don't think this is right, either.  If the re-search-forward
> fails, then (match-end 0) will fail, too.  And since (if I understood
> correctly), the prefix will follow on directly from where point it,
> using looking-at would be better, anyway...
>
>>        ;; Bypass a bug in certain versions of bash.
>>        (when (string-equal shell "bash")
>>          (add-hook 'comint-preoutput-filter-functions
>> -                  'shell-filter-ctrl-a-ctrl-b nil t)))
>> +                  'shell-filter-ctrl-a-ctrl-b nil t))
>> +
>> +      ;; Skip extended history for zsh.
>> +      (when (string-equal shell "zsh")
>> +        (setq-local comint-input-ring-file-prefix ": [[:digit:]]+:[[:digit:]]+;")))
>>      (comint-read-input-ring t)))
>
> And this bit didn't apply.

That was 7 months ago.  Did you have any time to look into the
comments by Lars above?  Thanks.

Best regards,
Stefan Kangas

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

From: Matthew Bauer <mjbauer95 <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Stefan Kangas <stefan <at> marxist.se>,
 36034 <at> debbugs.gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint input
 ring
Date: Wed, 18 Mar 2020 11:05:09 -0400

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

Sorry for the late reply. Attached is the updated patch, using looking-at
and on top of latest Emacs master.

On Thu, Jul 4, 2019 at 9:37 AM Lars Ingebrigtsen <larsi <at> gnus.org> wrote:

> Matthew Bauer <mjbauer95 <at> gmail.com> writes:
>
> >                   (setq start
> >                         (if (re-search-backward
> comint-input-ring-separator
> >                                                 nil t)
> > -                           (match-end 0)
> > -                         (point-min)))
> > +                           (progn
> > +                             (when comint-input-ring-file-prefix
> > +                               ;; Skip zsh extended_history stamps
> > +                               (re-search-forward
> comint-input-ring-file-prefix
> > +                                                  nil t))
> > +                             (match-end 0))
>
> Hm...  I don't think this is right, either.  If the re-search-forward
> fails, then (match-end 0) will fail, too.  And since (if I understood
> correctly), the prefix will follow on directly from where point it,
> using looking-at would be better, anyway...
>
> >        ;; Bypass a bug in certain versions of bash.
> >        (when (string-equal shell "bash")
> >          (add-hook 'comint-preoutput-filter-functions
> > -                  'shell-filter-ctrl-a-ctrl-b nil t)))
> > +                  'shell-filter-ctrl-a-ctrl-b nil t))
> > +
> > +      ;; Skip extended history for zsh.
> > +      (when (string-equal shell "zsh")
> > +        (setq-local comint-input-ring-file-prefix ":
> [[:digit:]]+:[[:digit:]]+;")))
> >      (comint-read-input-ring t)))
>
> And this bit didn't apply.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>

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

[0001-Add-zsh-extended_history-handling-for-comint.el-inpu.patch (text/x-patch, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Matthew Bauer <mjbauer95 <at> gmail.com>
Cc: Stefan Kangas <stefan <at> marxist.se>, 36034 <at> debbugs.gnu.org
Subject: Re: bug#36034: [PATCH] Zsh extended_history shows up in comint
 input ring
Date: Mon, 10 Aug 2020 13:11:32 +0200

Matthew Bauer <mjbauer95 <at> gmail.com> writes:

> Adds handling of the Zsh extended_history to comint.el input
> ring. This means that the timestamp doesn’t show up when reading
> through history from other shells. The lines look like this:
>
> : <beginning time>:<elapsed seconds>;<command>

Thanks; I applied your patch (with some changes -- I replaced a couple
of re-search-forwards that didn't seem to do much more than (goto
(match-end 0))).

Please check that I didn't mess something up when doing that fix.  :-)

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