should search ring contain duplicates?

should search ring contain duplicates?

[Drew Adams]

Each successful incremental search adds the search string to the search ring
(as the most recent entry), even if it is already in the ring past the first
entry. Nothing prevents the ring from containing multiple copies of the same
string, which seems wasteful (and useless). At the limit, the search ring,
regardless of its length, might contain as few as two different search
strings.

Shouldn't `isearch-update-ring' (in isearch.el) add the latest search string
at the front of the ring but remove it elsewhere in the ring? The result
would be a more useful ring.

Re: should search ring contain duplicates?

[Dan Nicolaescu]

"Drew Adams" <drew.adams@oracle.com> writes:

  > Each successful incremental search adds the search string to the search ring
  > (as the most recent entry), even if it is already in the ring past the first
  > entry. Nothing prevents the ring from containing multiple copies of the same
  > string, which seems wasteful (and useless). At the limit, the search ring,
  > regardless of its length, might contain as few as two different search
  > strings.
  >
  > Shouldn't `isearch-update-ring' (in isearch.el) add the latest search string
  > at the front of the ring but remove it elsewhere in the ring? The result
  > would be a more useful ring.

`isearch-update-ring' could at least do that when
history-delete-duplicates is t.

Something like this could help: [completely untested]

(defun isearch-update-ring (string &optional regexp)
  "Add STRING to the beginning of the search ring.
REGEXP says which ring to use."
  (if regexp
      (if (or (null regexp-search-ring)
	      (not (string= string (car regexp-search-ring))))
	  (progn
	    (when history-delete-duplicates
	      (setq regexp-search-ring (delete string regexp-search-ring)))
	    (push string regexp-search-ring)
	    (if (> (length regexp-search-ring) regexp-search-ring-max)
		(setcdr (nthcdr (1- search-ring-max) regexp-search-ring)
			nil))))
    (if (or (null search-ring)
	    (not (string= string (car search-ring))))
	(progn
	  (when history-delete-duplicates
	    (setq search-ring (delete string search-ring)))
	  (push string search-ring)
	  (if (> (length search-ring) search-ring-max)
	      (setcdr (nthcdr (1- search-ring-max) search-ring) nil))))))

Re: should search ring contain duplicates?

[Kim F. Storm]

Dan Nicolaescu <dann@ics.uci.edu> writes:

> 	    (when history-delete-duplicates
> 	      (setq regexp-search-ring (delete string regexp-search-ring)))
> 	    (push string regexp-search-ring)
> 	    (if (> (length regexp-search-ring) regexp-search-ring-max)
> 		(setcdr (nthcdr (1- search-ring-max) regexp-search-ring)
> 			nil))))

IMO, that would be a very good candidate for a standard macro
in subr.el.  

There are several places which does this (or should do it -- but
actually doesn't get this right!):

   (history-push ELT HIST &optional LENGTH KEEPDUPS)

Default for LENGTH would be history-length, but it should also look at
the history-length property of HIST.

If KEEPDUPS is t, ignore value of history-delete-duplicates.

This would make it easy for Lisp code to do the same history
manipulation as C code:

      if (NILP (histval)
	  || (CONSP (histval)
	      /* Don't duplicate the most recent entry in the history.  */
	      && (keep_all
		  || NILP (Fequal (histstring, Fcar (histval))))))
	{
	  Lisp_Object length;

	  if (history_delete_duplicates) Fdelete (histstring, histval);
	  histval = Fcons (histstring, histval);
	  Fset (Vminibuffer_history_variable, histval);

	  /* Truncate if requested.  */
	  length = Fget (Vminibuffer_history_variable, Qhistory_length);
	  if (NILP (length)) length = Vhistory_length;
	  if (INTEGERP (length))
	    {
	      if (XINT (length) <= 0)
		Fset (Vminibuffer_history_variable, Qnil);
	      else
		{
		  Lisp_Object temp;

		  temp = Fnthcdr (Fsub1 (length), histval);
		  if (CONSP (temp)) Fsetcdr (temp, Qnil);
		}
	    }
	}


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[Juri Linkov]

> IMO, that would be a very good candidate for a standard macro
> in subr.el.
>
> There are several places which does this (or should do it -- but
> actually doesn't get this right!):
>
>    (history-push ELT HIST &optional LENGTH KEEPDUPS)
>
> Default for LENGTH would be history-length, but it should also look at
> the history-length property of HIST.
>
> If KEEPDUPS is t, ignore value of history-delete-duplicates.
>
> This would make it easy for Lisp code to do the same history
> manipulation as C code.

I think this is a good idea.  This would help to get rid of a new
useless argument `keep-all' added a few months ago to the function
`read-from-minibuffer' only for the sake of query-replace.
Since query-replace is the only place in Emacs that uses this new argument,
it is better to remove it now before the release, and to use a new
macro `history-push' in `query-replace-read-to' to treat the query-replace
history specially.

Below is a tested patch that removes `keep-all' from `read-from-minibuffer',
adds duplicate replacement strings to the query-replace history explicitly,
and fixes more places where the value of `history-delete-duplicates'
is not taken into account yet (namely, `repeat-complex-command',
`call-interactively', and `isearch-update-ring').  This patch doesn't use
a new macro `history-push', but it would be easy to add it later to two
places in `isearch-update-ring' and `query-replace-read-to'.

Index: emacs/etc/NEWS
===================================================================
RCS file: /sources/emacs/emacs/etc/NEWS,v
retrieving revision 1.1336
diff -u -r1.1336 NEWS
*** emacs/etc/NEWS	29 Apr 2006 14:14:53 -0000	1.1336
--- emacs/etc/NEWS	3 May 2006 12:47:00 -0000
***************
*** 4271,4280 ****
  was selected when entering the minibuffer.
  
  +++
- *** `read-from-minibuffer' now accepts an additional argument KEEP-ALL
- saying to put all inputs in the history list, even empty ones.
- 
- +++
  *** The `read-file-name' function now takes an additional argument which
  specifies a predicate which the file name read must satisfy.  The
  new variable `read-file-name-predicate' contains the predicate argument
--- 4276,4281 ----
Index: emacs/lispref/minibuf.texi
===================================================================
RCS file: /sources/emacs/emacs/lispref/minibuf.texi,v
retrieving revision 1.77
diff -u -r1.77 minibuf.texi
*** emacs/lispref/minibuf.texi	19 Feb 2006 23:40:25 -0000	1.77
--- emacs/lispref/minibuf.texi	3 May 2006 12:47:18 -0000
***************
*** 110,116 ****
  reading the arguments for a command, in the @code{interactive}
  specification.  @xref{Defining Commands}.
  
! @defun read-from-minibuffer prompt-string &optional initial-contents keymap read hist default inherit-input-method keep-all
  This function is the most general way to get input through the
  minibuffer.  By default, it accepts arbitrary text and returns it as a
  string; however, if @var{read} is non-@code{nil}, then it uses
--- 110,116 ----
  reading the arguments for a command, in the @code{interactive}
  specification.  @xref{Defining Commands}.
  
! @defun read-from-minibuffer prompt-string &optional initial-contents keymap read hist default inherit-input-method
  This function is the most general way to get input through the
  minibuffer.  By default, it accepts arbitrary text and returns it as a
  string; however, if @var{read} is non-@code{nil}, then it uses
***************
*** 162,170 ****
  Representations}) from whichever buffer was current before entering the
  minibuffer.
  
- If @var{keep-all} is non-@code{nil}, even empty and duplicate inputs
- are added to the history list.
- 
  Use of @var{initial-contents} is mostly deprecated; we recommend using
  a non-@code{nil} value only in conjunction with specifying a cons cell
  for @var{hist}.  @xref{Initial Input}.
--- 162,167 ----
Index: emacs/src/lisp.h
===================================================================
RCS file: /sources/emacs/emacs/src/lisp.h,v
retrieving revision 1.562
diff -u -r1.562 lisp.h
*** emacs/src/lisp.h	12 Apr 2006 08:06:25 -0000	1.562
--- emacs/src/lisp.h	3 May 2006 12:47:46 -0000
***************
*** 2899,2905 ****
  extern Lisp_Object last_minibuf_string;
  extern void choose_minibuf_frame P_ ((void));
  EXFUN (Fcompleting_read, 8);
! EXFUN (Fread_from_minibuffer, 8);
  EXFUN (Fread_variable, 2);
  EXFUN (Fread_buffer, 3);
  EXFUN (Fread_minibuffer, 2);
--- 2900,2906 ----
  extern Lisp_Object last_minibuf_string;
  extern void choose_minibuf_frame P_ ((void));
  EXFUN (Fcompleting_read, 8);
! EXFUN (Fread_from_minibuffer, 7);
  EXFUN (Fread_variable, 2);
  EXFUN (Fread_buffer, 3);
  EXFUN (Fread_minibuffer, 2);
Index: emacs/src/minibuf.c
===================================================================
RCS file: /sources/emacs/emacs/src/minibuf.c,v
retrieving revision 1.301
diff -u -r1.301 minibuf.c
*** emacs/src/minibuf.c	6 Feb 2006 15:23:21 -0000	1.301
--- emacs/src/minibuf.c	3 May 2006 12:48:12 -0000
***************
*** 219,225 ****
  				     Lisp_Object, Lisp_Object,
  				     int, Lisp_Object,
  				     Lisp_Object, Lisp_Object,
! 				     int, int, int));
  static Lisp_Object read_minibuf_noninteractive P_ ((Lisp_Object, Lisp_Object,
  						    Lisp_Object, Lisp_Object,
  						    int, Lisp_Object,
--- 219,225 ----
  				     Lisp_Object, Lisp_Object,
  				     int, Lisp_Object,
  				     Lisp_Object, Lisp_Object,
! 				     int, int));
  static Lisp_Object read_minibuf_noninteractive P_ ((Lisp_Object, Lisp_Object,
  						    Lisp_Object, Lisp_Object,
  						    int, Lisp_Object,
***************
*** 440,447 ****
  
  static Lisp_Object
  read_minibuf (map, initial, prompt, backup_n, expflag,
! 	      histvar, histpos, defalt, allow_props, inherit_input_method,
! 	      keep_all)
       Lisp_Object map;
       Lisp_Object initial;
       Lisp_Object prompt;
--- 440,446 ----
  
  static Lisp_Object
  read_minibuf (map, initial, prompt, backup_n, expflag,
! 	      histvar, histpos, defalt, allow_props, inherit_input_method)
       Lisp_Object map;
       Lisp_Object initial;
       Lisp_Object prompt;
***************
*** 452,458 ****
       Lisp_Object defalt;
       int allow_props;
       int inherit_input_method;
-      int keep_all;
  {
    Lisp_Object val;
    int count = SPECPDL_INDEX ();
--- 451,456 ----
***************
*** 747,756 ****
    last_minibuf_string = val;
  
    /* Choose the string to add to the history.  */
!   if (SCHARS (val) != 0 || keep_all)
      histstring = val;
    else if (STRINGP (defalt))
      histstring = defalt;
    else
      histstring = Qnil;
  
--- 745,756 ----
    last_minibuf_string = val;
  
    /* Choose the string to add to the history.  */
!   if (SCHARS (val) != 0)
      histstring = val;
    else if (STRINGP (defalt))
      histstring = defalt;
    else
      histstring = Qnil;
  
***************
*** 774,781 ****
        if (NILP (histval)
  	  || (CONSP (histval)
  	      /* Don't duplicate the most recent entry in the history.  */
! 	      && (keep_all
! 		  || NILP (Fequal (histstring, Fcar (histval))))))
  	{
  	  Lisp_Object length;
  
--- 774,781 ----
        if (NILP (histval)
  	  || (CONSP (histval)
  	      /* Don't duplicate the most recent entry in the history.  */
! 	      && (NILP (Fequal (histstring, Fcar (histval))))))
  	{
  	  Lisp_Object length;
  
***************
*** 937,943 ****
  }
  \f
  
! DEFUN ("read-from-minibuffer", Fread_from_minibuffer, Sread_from_minibuffer, 1, 8, 0,
         doc: /* Read a string from the minibuffer, prompting with string PROMPT.
  The optional second arg INITIAL-CONTENTS is an obsolete alternative to
    DEFAULT-VALUE.  It normally should be nil in new code, except when
--- 937,943 ----
  }
  \f
  
! DEFUN ("read-from-minibuffer", Fread_from_minibuffer, Sread_from_minibuffer, 1, 7, 0,
         doc: /* Read a string from the minibuffer, prompting with string PROMPT.
  The optional second arg INITIAL-CONTENTS is an obsolete alternative to
    DEFAULT-VALUE.  It normally should be nil in new code, except when
***************
*** 961,968 ****
    the empty string.
  Seventh arg INHERIT-INPUT-METHOD, if non-nil, means the minibuffer inherits
   the current input method and the setting of `enable-multibyte-characters'.
- Eight arg KEEP-ALL, if non-nil, says to put all inputs in the history list,
-  even empty or duplicate inputs.
  If the variable `minibuffer-allow-text-properties' is non-nil,
   then the string which is returned includes whatever text properties
   were present in the minibuffer.  Otherwise the value has no text properties.
--- 961,966 ----
***************
*** 978,986 ****
  one puts point at the beginning of the string.  *Note* that this
  behavior differs from the way such arguments are used in `completing-read'
  and some related functions, which use zero-indexing for POSITION.  */)
!   (prompt, initial_contents, keymap, read, hist, default_value, inherit_input_method, keep_all)
       Lisp_Object prompt, initial_contents, keymap, read, hist, default_value;
!      Lisp_Object inherit_input_method, keep_all;
  {
    Lisp_Object histvar, histpos, val;
    struct gcpro gcpro1;
--- 976,984 ----
  one puts point at the beginning of the string.  *Note* that this
  behavior differs from the way such arguments are used in `completing-read'
  and some related functions, which use zero-indexing for POSITION.  */)
!   (prompt, initial_contents, keymap, read, hist, default_value, inherit_input_method)
       Lisp_Object prompt, initial_contents, keymap, read, hist, default_value;
!      Lisp_Object inherit_input_method;
  {
    Lisp_Object histvar, histpos, val;
    struct gcpro gcpro1;
***************
*** 1011,1018 ****
  		      Qnil, !NILP (read),
  		      histvar, histpos, default_value,
  		      minibuffer_allow_text_properties,
! 		      !NILP (inherit_input_method),
! 		      !NILP (keep_all));
    UNGCPRO;
    return val;
  }
--- 1009,1015 ----
  		      Qnil, !NILP (read),
  		      histvar, histpos, default_value,
  		      minibuffer_allow_text_properties,
! 		      !NILP (inherit_input_method));
    UNGCPRO;
    return val;
  }
***************
*** 1029,1035 ****
    CHECK_STRING (prompt);
    return read_minibuf (Vminibuffer_local_map, initial_contents,
  		       prompt, Qnil, 1, Qminibuffer_history,
! 		       make_number (0), Qnil, 0, 0, 0);
  }
  
  DEFUN ("eval-minibuffer", Feval_minibuffer, Seval_minibuffer, 1, 2, 0,
--- 1026,1032 ----
    CHECK_STRING (prompt);
    return read_minibuf (Vminibuffer_local_map, initial_contents,
  		       prompt, Qnil, 1, Qminibuffer_history,
! 		       make_number (0), Qnil, 0, 0);
  }
  
  DEFUN ("eval-minibuffer", Feval_minibuffer, Seval_minibuffer, 1, 2, 0,
***************
*** 1067,1075 ****
    Lisp_Object val;
    val = Fread_from_minibuffer (prompt, initial_input, Qnil,
  			       Qnil, history, default_value,
! 			       inherit_input_method, Qnil);
    if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (default_value))
!     val = default_value;
    return val;
  }
  
--- 1064,1072 ----
    Lisp_Object val;
    val = Fread_from_minibuffer (prompt, initial_input, Qnil,
  			       Qnil, history, default_value,
! 			       inherit_input_method);
    if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (default_value))
!     val = default_value;
    return val;
  }
  
***************
*** 1089,1095 ****
    CHECK_STRING (prompt);
    return read_minibuf (Vminibuffer_local_ns_map, initial, prompt, Qnil,
  		       0, Qminibuffer_history, make_number (0), Qnil, 0,
! 		       !NILP (inherit_input_method), 0);
  }
  
  DEFUN ("read-command", Fread_command, Sread_command, 1, 2, 0,
--- 1086,1092 ----
    CHECK_STRING (prompt);
    return read_minibuf (Vminibuffer_local_ns_map, initial, prompt, Qnil,
  		       0, Qminibuffer_history, make_number (0), Qnil, 0,
! 		       !NILP (inherit_input_method));
  }
  
  DEFUN ("read-command", Fread_command, Sread_command, 1, 2, 0,
***************
*** 1778,1787 ****
  			 : Vminibuffer_local_must_match_filename_map),
  		      init, prompt, make_number (pos), 0,
  		      histvar, histpos, def, 0,
! 		      !NILP (inherit_input_method), 0);
  
    if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (def))
!     val = def;
  
    RETURN_UNGCPRO (unbind_to (count, val));
  }
--- 1775,1784 ----
  			 : Vminibuffer_local_must_match_filename_map),
  		      init, prompt, make_number (pos), 0,
  		      histvar, histpos, def, 0,
! 		      !NILP (inherit_input_method));
  
    if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (def))
!     val = CONSP (def) ? XCAR (def) : def;
  
    RETURN_UNGCPRO (unbind_to (count, val));
  }
Index: emacs/lisp/replace.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/replace.el,v
retrieving revision 1.238
diff -u -r1.238 replace.el
*** emacs/lisp/replace.el	6 Feb 2006 14:33:35 -0000	1.238
--- emacs/lisp/replace.el	3 May 2006 14:45:00 -0000
***************
*** 101,107 ****
  	    ;; a region in order to specify the minibuffer input.
  	    ;; That should not clobber the region for the query-replace itself.
  	    (save-excursion
! 	      (when (equal lastfrom lastto)
  		;; Typically, this is because the two histlists are shared.
  		(setq lastfrom (cadr (symbol-value
  				      query-replace-from-history-variable))))
--- 101,109 ----
  	    ;; a region in order to specify the minibuffer input.
  	    ;; That should not clobber the region for the query-replace itself.
  	    (save-excursion
! 	      (when (and (equal lastfrom lastto)
! 			 (eq query-replace-from-history-variable
! 			     query-replace-to-history-variable))
  		;; Typically, this is because the two histlists are shared.
  		(setq lastfrom (cadr (symbol-value
  				      query-replace-from-history-variable))))
***************
*** 113,125 ****
  		 (format "%s: " prompt))
  	       nil nil nil
  	       query-replace-from-history-variable
! 	       nil t t))))
        (if (and (zerop (length from)) lastto lastfrom)
! 	  (progn
! 	    (set query-replace-from-history-variable
! 		 (cdr (symbol-value query-replace-from-history-variable)))
! 	    (cons lastfrom
! 		  (query-replace-compile-replacement lastto regexp-flag)))
  	;; Warn if user types \n or \t, but don't reject the input.
  	(and regexp-flag
  	     (string-match "\\(\\`\\|[^\\]\\)\\(\\\\\\\\\\)*\\(\\\\[nt]\\)" from)
--- 115,124 ----
  		 (format "%s: " prompt))
  	       nil nil nil
  	       query-replace-from-history-variable
! 	       nil t))))
        (if (and (zerop (length from)) lastto lastfrom)
! 	  (cons lastfrom
! 		(query-replace-compile-replacement lastto regexp-flag))
  	;; Warn if user types \n or \t, but don't reject the input.
  	(and regexp-flag
  	     (string-match "\\(\\`\\|[^\\]\\)\\(\\\\\\\\\\)*\\(\\\\[nt]\\)" from)
***************
*** 175,187 ****
  
  (defun query-replace-read-to (from prompt regexp-flag)
    "Query and return the `to' argument of a query-replace operation."
!   (query-replace-compile-replacement
!    (save-excursion
!      (read-from-minibuffer
!       (format "%s %s with: " prompt (query-replace-descr from))
!       nil nil nil
!       query-replace-to-history-variable from t t))
!    regexp-flag))
  
  (defun query-replace-read-args (prompt regexp-flag &optional noerror)
    (unless noerror
--- 174,187 ----
  
  (defun query-replace-read-to (from prompt regexp-flag)
    "Query and return the `to' argument of a query-replace operation."
!   (let* ((to-history (symbol-value query-replace-to-history-variable))
! 	 (to (save-excursion
! 	       (read-from-minibuffer
! 		(format "%s %s with: " prompt (query-replace-descr from))
! 		nil nil nil
! 		query-replace-to-history-variable from t))))
!     (set query-replace-to-history-variable (cons to to-history))
!     (query-replace-compile-replacement to regexp-flag)))
  
  (defun query-replace-read-args (prompt regexp-flag &optional noerror)
    (unless noerror
Index: emacs/lisp/simple.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/simple.el,v
retrieving revision 1.797
diff -u -r1.797 simple.el
*** emacs/lisp/simple.el	9 Apr 2006 23:03:48 -0000	1.797
--- emacs/lisp/simple.el	3 May 2006 12:45:28 -0000
***************
*** 1135,1141 ****
  		    ;; evaluable expressions there.
  		    (if (stringp (car command-history))
  			(setq command-history (cdr command-history))))))
! 
  	  ;; If command to be redone does not match front of history,
  	  ;; add it to the history.
  	  (or (equal newcmd (car command-history))
--- 1135,1142 ----
  		    ;; evaluable expressions there.
  		    (if (stringp (car command-history))
  			(setq command-history (cdr command-history))))))
! 	  (if history-delete-duplicates
! 	      (setq command-history (delete newcmd command-history)))
  	  ;; If command to be redone does not match front of history,
  	  ;; add it to the history.
  	  (or (equal newcmd (car command-history))
Index: emacs/src/callint.c
===================================================================
RCS file: /sources/emacs/emacs/src/callint.c,v
retrieving revision 1.141
diff -u -r1.141 callint.c
*** emacs/src/callint.c	6 Feb 2006 15:23:20 -0000	1.141
--- emacs/src/callint.c	3 May 2006 12:45:18 -0000
***************
*** 40,45 ****
--- 40,46 ----
  Lisp_Object Qcall_interactively;
  Lisp_Object Vcommand_history;
  
+ extern int history_delete_duplicates;
  extern Lisp_Object Vhistory_length;
  extern Lisp_Object Vthis_original_command, real_this_command;
  
***************
*** 386,398 ****
        if (i != num_input_events || !NILP (record_flag))
  	{
  	  /* We should record this command on the command history.  */
  	  Lisp_Object values;
  	  /* Make a copy of the list of values, for the command history,
  	     and turn them into things we can eval.  */
  	  values = quotify_args (Fcopy_sequence (specs));
  	  fix_command (input, values);
! 	  Vcommand_history
! 	    = Fcons (Fcons (function, values), Vcommand_history);
  
  	  /* Don't keep command history around forever.  */
  	  if (INTEGERP (Vhistory_length) && XINT (Vhistory_length) > 0)
--- 386,402 ----
        if (i != num_input_events || !NILP (record_flag))
  	{
  	  /* We should record this command on the command history.  */
+ 	  Lisp_Object histelem;
  	  Lisp_Object values;
  	  /* Make a copy of the list of values, for the command history,
  	     and turn them into things we can eval.  */
  	  values = quotify_args (Fcopy_sequence (specs));
  	  fix_command (input, values);
! 
! 	  histelem = Fcons (function, values);
! 	  if (history_delete_duplicates)
! 	    Vcommand_history = Fdelete (histelem, Vcommand_history);
! 	  Vcommand_history = Fcons (histelem, Vcommand_history);
  
  	  /* Don't keep command history around forever.  */
  	  if (INTEGERP (Vhistory_length) && XINT (Vhistory_length) > 0)
***************
*** 730,736 ****
  
  		tem = Fread_from_minibuffer (build_string (callint_message),
  					     Qnil, Qnil, Qnil, Qnil, Qnil,
! 					     Qnil, Qnil);
  		if (! STRINGP (tem) || SCHARS (tem) == 0)
  		  args[i] = Qnil;
  		else
--- 737,743 ----
  
  		tem = Fread_from_minibuffer (build_string (callint_message),
  					     Qnil, Qnil, Qnil, Qnil, Qnil,
! 					     Qnil);
  		if (! STRINGP (tem) || SCHARS (tem) == 0)
  		  args[i] = Qnil;
  		else
***************
*** 842,847 ****
--- 849,855 ----
  
    if (arg_from_tty || !NILP (record_flag))
      {
+       Lisp_Object histelem;
        visargs[0] = function;
        for (i = 1; i < count + 1; i++)
  	{
***************
*** 850,857 ****
  	  else
  	    visargs[i] = quotify_arg (args[i]);
  	}
!       Vcommand_history = Fcons (Flist (count + 1, visargs),
! 				Vcommand_history);
        /* Don't keep command history around forever.  */
        if (INTEGERP (Vhistory_length) && XINT (Vhistory_length) > 0)
  	{
--- 858,868 ----
  	  else
  	    visargs[i] = quotify_arg (args[i]);
  	}
! 
!       histelem = Flist (count + 1, visargs);
!       if (history_delete_duplicates)
! 	Vcommand_history = Fdelete (histelem, Vcommand_history);
!       Vcommand_history = Fcons (histelem, Vcommand_history);
        /* Don't keep command history around forever.  */
        if (INTEGERP (Vhistory_length) && XINT (Vhistory_length) > 0)
  	{
Index: emacs/lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.285
diff -u -r1.285 isearch.el
*** emacs/lisp/isearch.el	18 Mar 2006 15:11:48 -0000	1.285
--- emacs/lisp/isearch.el	3 May 2006 12:44:36 -0000
***************
*** 835,840 ****
--- 842,849 ----
        (if (or (null regexp-search-ring)
  	      (not (string= string (car regexp-search-ring))))
  	  (progn
+ 	    (if history-delete-duplicates
+ 		(setq regexp-search-ring (delete string regexp-search-ring)))
  	    (push string regexp-search-ring)
  	    (if (> (length regexp-search-ring) regexp-search-ring-max)
  		(setcdr (nthcdr (1- search-ring-max) regexp-search-ring)
***************
*** 842,847 ****
--- 851,858 ----
      (if (or (null search-ring)
  	    (not (string= string (car search-ring))))
  	(progn
+ 	  (if history-delete-duplicates
+ 	      (setq search-ring (delete string search-ring)))
  	  (push string search-ring)
  	  (if (> (length search-ring) search-ring-max)
  	      (setcdr (nthcdr (1- search-ring-max) search-ring) nil))))))

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: should search ring contain duplicates?

[Kim F. Storm]

Juri Linkov <juri@jurta.org> writes:

> Since query-replace is the only place in Emacs that uses this new argument,
> it is better to remove it now before the release, and to use a new
> macro `history-push' in `query-replace-read-to' to treat the query-replace
> history specially.

Agree.

> Below is a tested patch that removes `keep-all' from `read-from-minibuffer',
> adds duplicate replacement strings to the query-replace history explicitly,
> and fixes more places where the value of `history-delete-duplicates'
> is not taken into account yet 

Looks good to me!

>                               (namely, `repeat-complex-command',
> `call-interactively', and `isearch-update-ring').  This patch doesn't use
> a new macro `history-push', but it would be easy to add it later to two
> places in `isearch-update-ring' and `query-replace-read-to'.

I definitely think we need the history-push macro!
Here is something which works for me:


(defmacro history-push (newelt history &optional maxelt)
  "Add NEWELT to the history list stored in the symbol HISTORY.
Truncate the history to max MAXELT elements, if specified, or
to the value of the `history-length' property on symbol HISTORY,
if set, or to the value of the `history-length' variable.
Remove duplicates of NEWELT unless `history-delete-duplicates' is nil."
  (declare (debug (form sexp)))
  `(let ((len ,maxelt))
     (if history-delete-duplicates
	 (setq ,history (delete ,newelt ,history)))
     (setq ,history (cons ,newelt ,history))
     (if (null len)
	 (setq len (or (get ',history 'history-length)
		       history-length)))
     (if (integerp len)
	 (if (= 0 len)
	     (setq ,history nil)
	   (if (> (length ,history) len)
	       (setcdr (nthcdr (1- len) ,history) nil))))))


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[Kim F. Storm]

storm@cua.dk (Kim F. Storm) writes:

> I definitely think we need the history-push macro!
> Here is something which works for me:

.. almost :-)  Here is a better version:

(defmacro history-push (newelt history &optional maxelt)
  "Add NEWELT to the history list stored in the symbol HISTORY.
If symbol MAXELT is specified, the maximum length of the history is
specified by the value of that symbol.  Otherwise, the maximum history
length is  to the value of the `history-length' property on symbol
HISTORY, if set, or to the value of the `history-length' variable.
Remove duplicates of NEWELT unless `history-delete-duplicates' is nil."
  (declare (debug (form sexp)))
  (unless maxelt
    (setq maxelt `(or (get ',history 'history-length)
		       history-length)))
  `(let ((len ,maxelt))
     (if history-delete-duplicates
	 (setq ,history (delete ,newelt ,history)))
     (setq ,history (cons ,newelt ,history))
     (when (integerp len)
       (if (= 0 len)
	   (setq ,history nil)
	 (if (> (length ,history) len)
	     (setcdr (nthcdr (1- len) ,history) nil))))))


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> storm@cua.dk (Kim F. Storm) writes:
>
>> I definitely think we need the history-push macro!
>> Here is something which works for me:
>
> .. almost :-)  Here is a better version:
>
> (defmacro history-push (newelt history &optional maxelt)
>   "Add NEWELT to the history list stored in the symbol HISTORY.
> If symbol MAXELT is specified, the maximum length of the history is
> specified by the value of that symbol.  Otherwise, the maximum history
> length is  to the value of the `history-length' property on symbol
> HISTORY, if set, or to the value of the `history-length' variable.
> Remove duplicates of NEWELT unless `history-delete-duplicates' is nil."
>   (declare (debug (form sexp)))
>   (unless maxelt
>     (setq maxelt `(or (get ',history 'history-length)
> 		       history-length)))
>   `(let ((len ,maxelt))
>      (if history-delete-duplicates
> 	 (setq ,history (delete ,newelt ,history)))
>      (setq ,history (cons ,newelt ,history))
>      (when (integerp len)
>        (if (= 0 len)
> 	   (setq ,history nil)
> 	 (if (> (length ,history) len)
> 	     (setcdr (nthcdr (1- len) ,history) nil))))))

What is the rationale for making this a macro?  It's not performance
relevant, and not short.  Seems like something

(defun history-push (newelt history &optional maxelt)
  "Add NEWELT to the history list stored in the symbol HISTORY.
If symbol MAXELT is specified, the maximum length of the history is
specified by the value of that symbol.  Otherwise, the maximum history
length is  to the value of the `history-length' property on symbol
HISTORY, if set, or to the value of the `history-length' variable.
Remove duplicates of NEWELT unless `history-delete-duplicates' is nil."
  (unless maxelt
    (setq maxelt (or (get history 'history-length)
		       history-length)))
  (if history-delete-duplicates
     (set history (delete newelt (symbol-value history))))
  (set history (cons newelt (symbol-value history)))
  (when (integerp maxelt)
      (if (= 0 maxelt)
	 (set history nil)
       (if (> (length (symbol-value history)) maxelt)
          (setcdr (nthcdr (1- maxelt) (symbol-value history)) nil)))))

Sure, you need to quote history in the call then, but it seems like
the trouble for a macro is not really warranted.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: should search ring contain duplicates?

[Stefan Monnier]

> What is the rationale for making this a macro?  It's not performance
> relevant, and not short.

100% agreement.

> Sure, you need to quote history in the call then,

Big deal!  Especially given that history variables are already quoted
everywhere else they're used (i.e. in calls to read-*).


        Stefan

Re: should search ring contain duplicates?

[Kim F. Storm]

David Kastrup <dak@gnu.org> writes:

> What is the rationale for making this a macro?

It's named ...-push :-)

> Sure, you need to quote history in the call then, but it seems like
> the trouble for a macro is not really warranted.

You're right, so let's change it to a defun.

But then, to reduce confusion, the name should be `add-to-history',
and the sequence of args changed to match the interface of add-to-list.


(defun add-to-history (history-var newelt &optional maxelt)
  "Add NEWELT to the history list stored in the variable HISTORY-VAR.
If MAXELT is non-nil, it specifies the maximum length of the history.
Otherwise, the maximum history length is the value of the `history-length'
property on symbol HISTORY-VAR, if set, or the value of the `history-length'
variable.
Remove duplicates of NEWELT unless `history-delete-duplicates' is nil."
  (unless maxelt
    (setq maxelt (or (get history-var 'history-length)
		     history-length)))
  (let ((history (symbol-value history-var)))
    (if history-delete-duplicates
	(setq history (delete newelt history)))
    (setq history (cons newelt history))
    (when (integerp maxelt)
      (if (= 0 maxelt)
	  (setq history nil)
	(if (> (length history) maxelt)
	    (setcdr (nthcdr (1- maxelt) history) nil))))
    (set history-var history)))

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> David Kastrup <dak@gnu.org> writes:
>
>> What is the rationale for making this a macro?
>
> It's named ...-push :-)
>
>> Sure, you need to quote history in the call then, but it seems like
>> the trouble for a macro is not really warranted.
>
> You're right, so let's change it to a defun.
>
> But then, to reduce confusion, the name should be `add-to-history',
> and the sequence of args changed to match the interface of add-to-list.

Sounds reasonable.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: should search ring contain duplicates?

[Richard Stallman]

Please install add-to-history;
you can change places to use it, too.

Re: should search ring contain duplicates?

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

> Please install add-to-history;
> you can change places to use it, too.

Done.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[Eli Zaretskii]

> From: storm@cua.dk (Kim F. Storm)
> Date: Sat, 06 May 2006 01:55:43 +0200
> Cc: juri@jurta.org, dann@ics.uci.edu, emacs-devel@gnu.org
> 
> Richard Stallman <rms@gnu.org> writes:
> 
> > Please install add-to-history;
> > you can change places to use it, too.
> 
> Done.

I'm not sure I see the utility of the second optional argument
KEEP-DUPS.  It doesn't give you full control of whether the duplicates
are removed, unless you also bind history-delete-duplicates, so it's
not more convenient than simply binding history-delete-duplicates.
Its disadvantage, OTOH, albeit a minor one, is that it makes the doc
string harder to understand, due to multiple negations (``unless FOO
is nil or BAR is non-nil'').

Am I missing something?

(If you do change the function, please update the NEWS entry and the
Lisp manual where I added its description.)

Re: should search ring contain duplicates?

[Richard Stallman]

    I'm not sure I see the utility of the second optional argument
    KEEP-DUPS.

I don't see a need for it either, but does anyone else?

Re: should search ring contain duplicates?

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     I'm not sure I see the utility of the second optional argument
>     KEEP-DUPS.
>
> I don't see a need for it either, but does anyone else?

I'll remove it.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[Stuart D. Herring]

> (defun history-push (newelt history &optional maxelt)
>   "Add NEWELT to the history list stored in the symbol HISTORY.
> If symbol MAXELT is specified, the maximum length of the history is
> specified by the value of that symbol.  Otherwise, the maximum history
> length is  to the value of the `history-length' property on symbol
> HISTORY, if set, or to the value of the `history-length' variable.
> Remove duplicates of NEWELT unless `history-delete-duplicates' is nil."
>   (unless maxelt
>     (setq maxelt (or (get history 'history-length)
> 		       history-length)))
>   (if history-delete-duplicates
>      (set history (delete newelt (symbol-value history))))
>   (set history (cons newelt (symbol-value history)))
>   (when (integerp maxelt)
>       (if (= 0 maxelt)
> 	 (set history nil)
>        (if (> (length (symbol-value history)) maxelt)
>           (setcdr (nthcdr (1- maxelt) (symbol-value history)) nil)))))
>
> Sure, you need to quote history in the call then, but it seems like
> the trouble for a macro is not really warranted.

Evaluating the history argument is a feature, not a bug.  It lets you do
something like this:

(defun isearch-update-ring (string &optional regexp)
  "Add STRING to the beginning of the search ring.
REGEXP says which ring to use."
  (history-push string
                (if regexp 'regexp-search-ring 'search-ring)
                (if regexp regexp-search-ring-max search-ring-max)))

(With, of course, modifications if Kim's `add-to-history' is used instead.)

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: should search ring contain duplicates?

[David Kastrup]

"Stuart D. Herring" <herring@lanl.gov> writes:

>> (defun history-push (newelt history &optional maxelt)
>>   "Add NEWELT to the history list stored in the symbol HISTORY.
>> If symbol MAXELT is specified, the maximum length of the history is
>> specified by the value of that symbol.  Otherwise, the maximum history
>> length is  to the value of the `history-length' property on symbol
>> HISTORY, if set, or to the value of the `history-length' variable.
>> Remove duplicates of NEWELT unless `history-delete-duplicates' is nil."
>>   (unless maxelt
>>     (setq maxelt (or (get history 'history-length)
>> 		       history-length)))
>>   (if history-delete-duplicates
>>      (set history (delete newelt (symbol-value history))))
>>   (set history (cons newelt (symbol-value history)))
>>   (when (integerp maxelt)
>>       (if (= 0 maxelt)
>> 	 (set history nil)
>>        (if (> (length (symbol-value history)) maxelt)
>>           (setcdr (nthcdr (1- maxelt) (symbol-value history)) nil)))))
>>
>> Sure, you need to quote history in the call then, but it seems like
>> the trouble for a macro is not really warranted.
>
> Evaluating the history argument is a feature, not a bug.  It lets you do
> something like this:
>
> (defun isearch-update-ring (string &optional regexp)
>   "Add STRING to the beginning of the search ring.
> REGEXP says which ring to use."
>   (history-push string
>                 (if regexp 'regexp-search-ring 'search-ring)
>                 (if regexp regexp-search-ring-max search-ring-max)))

No, it doesn't.  Thanks for making my point that "the trouble for a
macro is not really warranted".

(get '(if regexp 'regexp-search-ring 'search-ring) 'history-length)

is going to throw an error at execution time.

Are you sure you know what a macro does?  It compiles the form
resulting from executing the macro on the quoted arguments at compile
time.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: should search ring contain duplicates?

[Stuart D. Herring]

>>> Sure, you need to quote history in the call then, but it seems like
>>> the trouble for a macro is not really warranted.
>>
>> Evaluating the history argument is a feature, not a bug.  It lets you do
>> something like this:
>>
>> (defun isearch-update-ring (string &optional regexp)
>>   "Add STRING to the beginning of the search ring.
>> REGEXP says which ring to use."
>>   (history-push string
>>                 (if regexp 'regexp-search-ring 'search-ring)
>>                 (if regexp regexp-search-ring-max search-ring-max)))
>
> No, it doesn't.  Thanks for making my point that "the trouble for a
> macro is not really warranted".

No, what doesn't do what?  If you're saying that using the macro does not
let you do what I suggested that `isearch-update-ring' could do, you're of
course right.  However, that's not what I was saying.

I was agreeing with your suggestion that it should be a function.  I was
discussing your noted drawback that "you need to quote history in the call
then".  I was pointing out a case where the evaluation of the history
argument (which often necessitates the quoting you noted) is a good thing,
by using that evaluation to combine the code for the two search rings in
`isearch-update-ring'.  Of course, with the macro one could do

(if regexp (history-push string regexp-search-ring regexp-search-ring-max)
           (history-push string search-ring search-ring-max))

...which isn't any longer.  But for the case of using the default for
maxlen, being able to stick an (if) in the ring argument would be the
convenient option.

Hopefully my previous message is clearer now.

> (get '(if regexp 'regexp-search-ring 'search-ring) 'history-length)
>
> is going to throw an error at execution time.
>
> Are you sure you know what a macro does?  It compiles the form
> resulting from executing the macro on the quoted arguments at compile
> time.

I do believe I understand them, but if my previous mail exposes some
misunderstanding even given that it was discussing the use of the function
and not the macro, please do enlighten me.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: should search ring contain duplicates?

[Kim F. Storm]

"Stuart D. Herring" <herring@lanl.gov> writes:

> (defun isearch-update-ring (string &optional regexp)
>   "Add STRING to the beginning of the search ring.
> REGEXP says which ring to use."
>   (history-push string
>                 (if regexp 'regexp-search-ring 'search-ring)
>                 (if regexp regexp-search-ring-max search-ring-max)))

What a piece of art!  Thanks!

It shows exactly why add-to-history is the rigth way to go.

And fixes the bug in isearch-update-ring too !

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

query-replace in isearch (was Re: should search ring contain duplicates?)

[Dan Nicolaescu]

[snip]
  > I think this is a good idea.  This would help to get rid of a new
  > useless argument `keep-all' added a few months ago to the function
  > `read-from-minibuffer' only for the sake of query-replace.
  > Since query-replace is the only place in Emacs that uses this new argument,
  > it is better to remove it now before the release, and to use a new
  > macro `history-push' in `query-replace-read-to' to treat the query-replace
  > history specially.

This reminds me of the fact that doing `query-replace' from isearch
does not put `query-replace' in the complex command history. IMHO it
should. i.e.: C-s FOO M-% BAR RET C-x ESC ESC should show the same
thing as M-% FOO RET BAR RET C-x ESC ESC

Re: query-replace in isearch

[Juri Linkov]

> This reminds me of the fact that doing `query-replace' from isearch
> does not put `query-replace' in the complex command history. IMHO it
> should. i.e.: C-s FOO M-% BAR RET C-x ESC ESC should show the same
> thing as M-% FOO RET BAR RET C-x ESC ESC

Calling `query-replace' from isearch with using `call-interactive' would
add it with its arguments to the complex command history, but using
`call-interactive' is not feasible here, since isearch-query-replace calls
a low-level replacement function `perform-replace'.  So one way to
implement this I see is to use a new macro `history-push' that Kim
just proposed.  This is the third place where this macro would be useful.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Richard Stallman]

    This reminds me of the fact that doing `query-replace' from isearch
    does not put `query-replace' in the complex command history. IMHO it
    should. i.e.: C-s FOO M-% BAR RET C-x ESC ESC should show the same
    thing as M-% FOO RET BAR RET C-x ESC ESC

I agree.  Could you fix it?

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Richard Stallman]

    This reminds me of the fact that doing `query-replace' from isearch
    does not put `query-replace' in the complex command history. IMHO it
    should. i.e.: C-s FOO M-% BAR RET C-x ESC ESC should show the same
    thing as M-% FOO RET BAR RET C-x ESC ESC

Would someone please fix this and ack?

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Juri Linkov]

>     This reminds me of the fact that doing `query-replace' from isearch
>     does not put `query-replace' in the complex command history. IMHO it
>     should. i.e.: C-s FOO M-% BAR RET C-x ESC ESC should show the same
>     thing as M-% FOO RET BAR RET C-x ESC ESC
>
> Would someone please fix this and ack?

The following patch should do that.  It also uses `add-to-history'
to add the current search string to `query-replace-from-history-variable'
to improve code in the same function.  This requires adding a new
argument KEEP-ALL to `add-to-history'. Its meaning is the same as the
argument KEEP-ALL of `read-from-minibuffer'.

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.288
diff -c -r1.288 isearch.el
*** lisp/isearch.el	5 May 2006 23:37:31 -0000	1.288
--- lisp/isearch.el	11 May 2006 11:55:45 -0000
***************
*** 1214,1220 ****
    (interactive)
    (barf-if-buffer-read-only)
    (if regexp-flag (setq isearch-regexp t))
!   (let ((case-fold-search isearch-case-fold-search))
      (isearch-done)
      (isearch-clean-overlays)
      (if (and isearch-other-end
--- 1229,1235 ----
    (interactive)
    (barf-if-buffer-read-only)
    (if regexp-flag (setq isearch-regexp t))
!   (let ((case-fold-search isearch-case-fold-search) to)
      (isearch-done)
      (isearch-clean-overlays)
      (if (and isearch-other-end
***************
*** 1222,1239 ****
               (not (and transient-mark-mode mark-active
                         (< (mark) (point)))))
          (goto-char isearch-other-end))
!     (set query-replace-from-history-variable
!          (cons isearch-string
!                (symbol-value query-replace-from-history-variable)))
      (perform-replace
       isearch-string
!      (query-replace-read-to
!       isearch-string
!       (if isearch-regexp "Query replace regexp" "Query replace")
!       isearch-regexp)
       t isearch-regexp isearch-word nil nil
       (if (and transient-mark-mode mark-active) (region-beginning))
!      (if (and transient-mark-mode mark-active) (region-end)))))
  
  (defun isearch-query-replace-regexp ()
    "Start query-replace-regexp with string to replace from last search string."
--- 1237,1258 ----
               (not (and transient-mark-mode mark-active
                         (< (mark) (point)))))
          (goto-char isearch-other-end))
!     (add-to-history query-replace-from-history-variable isearch-string nil t)
      (perform-replace
       isearch-string
!      (setq to (query-replace-read-to
! 	       isearch-string
! 	       (if isearch-regexp "Query replace regexp" "Query replace")
! 	       isearch-regexp))
       t isearch-regexp isearch-word nil nil
       (if (and transient-mark-mode mark-active) (region-beginning))
!      (if (and transient-mark-mode mark-active) (region-end)))
!     (add-to-history
!      'command-history
!      (list (if isearch-regexp 'query-replace-regexp 'query-replace)
! 	   isearch-string to isearch-word
! 	   '(if (and transient-mark-mode mark-active) (region-beginning))
! 	   '(if (and transient-mark-mode mark-active) (region-end))))))
  
  (defun isearch-query-replace-regexp ()
    "Start query-replace-regexp with string to replace from last search string."

Index: lisp/subr.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/subr.el,v
retrieving revision 1.507
diff -c -r1.507 subr.el
*** lisp/subr.el	7 May 2006 20:49:01 -0000	1.507
--- lisp/subr.el	11 May 2006 11:55:55 -0000
***************
*** 1123,1151 ****
  				(< oa ob)
  			      oa)))))))
  
! (defun add-to-history (history-var newelt &optional maxelt)
    "Add NEWELT to the history list stored in the variable HISTORY-VAR.
  Return the new history list.
  If MAXELT is non-nil, it specifies the maximum length of the history.
  Otherwise, the maximum history length is the value of the `history-length'
  property on symbol HISTORY-VAR, if set, or the value of the `history-length'
  variable.
! Remove duplicates of NEWELT unless `history-delete-duplicates' is nil."
    (unless maxelt
      (setq maxelt (or (get history-var 'history-length)
  		     history-length)))
    (let ((history (symbol-value history-var))
  	tail)
!     (if history-delete-duplicates
  	(setq history (delete newelt history)))
!     (setq history (cons newelt history))
!     (when (integerp maxelt)
!       (if (= 0 maxelt)
! 	  (setq history nil)
! 	(setq tail (nthcdr (1- maxelt) history))
! 	(when (consp tail)
! 	  (setcdr tail nil))))
!     (set history-var history)))
  
  \f
  ;;;; Mode hooks.
--- 1123,1154 ----
  				(< oa ob)
  			      oa)))))))
  
! (defun add-to-history (history-var newelt &optional maxelt keep-all)
    "Add NEWELT to the history list stored in the variable HISTORY-VAR.
  Return the new history list.
  If MAXELT is non-nil, it specifies the maximum length of the history.
  Otherwise, the maximum history length is the value of the `history-length'
  property on symbol HISTORY-VAR, if set, or the value of the `history-length'
  variable.
! Remove duplicates of NEWELT unless `history-delete-duplicates' is nil.
! KEEP-ALL, if non-nil, says to put all inputs in the history list,
! even empty or duplicate inputs."
    (unless maxelt
      (setq maxelt (or (get history-var 'history-length)
  		     history-length)))
    (let ((history (symbol-value history-var))
  	tail)
!     (if (and history-delete-duplicates (null keep-all))
  	(setq history (delete newelt history)))
!     (unless (and (null keep-all) history (equal (car history) newelt))
!       (setq history (cons newelt history))
!       (when (integerp maxelt)
! 	(if (= 0 maxelt)
! 	    (setq history nil)
! 	  (setq tail (nthcdr (1- maxelt) history))
! 	  (when (consp tail)
! 	    (setcdr tail nil))))
!       (set history-var history))))
  
  \f
  ;;;; Mode hooks.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Kim F. Storm]

Juri Linkov <juri@jurta.org> writes:

>  This requires adding a new
> argument KEEP-ALL to `add-to-history'. Its meaning is the same as the
> argument KEEP-ALL of `read-from-minibuffer'.

That makes sense for the same reason as it makes sense
in read-from-minibuffer.

But didn't you just argue that we could remove keep-all from that
function?

Just a little confused :-)

Pls. remember to update the lispref entry for add-to-history.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Juri Linkov]

>>  This requires adding a new
>> argument KEEP-ALL to `add-to-history'. Its meaning is the same as the
>> argument KEEP-ALL of `read-from-minibuffer'.
>
> That makes sense for the same reason as it makes sense
> in read-from-minibuffer.
>
> But didn't you just argue that we could remove keep-all from that
> function?

`read-from-minibuffer' already has too many arguments, and I think adding
a new very specific argument (used for a special history handling just in
one specific place in Emacs) to this general function was not a good thing.

I'm trying to find a better replacement for KEEP-ALL.  There are currently
two alternative variants of keeping empty and duplicate elements
in the history:

1. Instead of using the KEEP-ALL argument bind `history-delete-duplicates'
to a special value `keep-all' before calling `read-from-minibuffer'.

2. Tell `read-from-minibuffer' not to add a new element to the history
list, and add it later by calling `add-to-history' with the KEEP-ALL
argument.

I like the latter.  In my latest patch I preserve the original history
list before calling `read-from-minibuffer', and overwrite the history list
modified by `read-from-minibuffer' with the old original value.  After that,
`add-to-history' adds a new element to the original history list.

However, I admit such preserving is not a good solution, because
in the minibuffer the user can use own commands to modify the history
list, and this modified history list gets overwritten with the old value.

What about a special value of `history-length' (e.g. nil) to tell
`read-from-minibuffer' not to add a new element to the history list
(with the intention to add it later by `add-to-history')?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Juri Linkov]

> I'm trying to find a better replacement for KEEP-ALL.

I want to remind that for add-to-history to be usable in several places
there should be a way for read-from-minibuffer not to add a new
element to the history list, thus allowing add-to-history to perform
this task itself afterwards.

I see two ways to do this:

1. before calling read-from-minibuffer to change a global variable
   (either `history-delete-duplicates' or `history-length') to a special
   value.

2. to change the argument KEEP-ALL to DONT-KEEP so that if DONT-KEEP
   is t, then read-from-minibuffer doesn't add a new element to the
   history list, and the calling code can add it by the explicit call
   of add-to-history (with any necessary arguments including the value t
   for KEEP-ALL argument of add-to-history).

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Kim F. Storm]

Juri Linkov <juri@jurta.org> writes:

>> I'm trying to find a better replacement for KEEP-ALL.
>
> I want to remind that for add-to-history to be usable in several places
> there should be a way for read-from-minibuffer not to add a new
> element to the history list, thus allowing add-to-history to perform
> this task itself afterwards.
>
> I see two ways to do this:
>
> 1. before calling read-from-minibuffer to change a global variable
>    (either `history-delete-duplicates' or `history-length') to a special
>    value.
>
> 2. to change the argument KEEP-ALL to DONT-KEEP so that if DONT-KEEP
>    is t, then read-from-minibuffer doesn't add a new element to the
>    history list, and the calling code can add it by the explicit call
>    of add-to-history (with any necessary arguments including the value t
>    for KEEP-ALL argument of add-to-history).

Can't you let-bind the history variable around the call to
read-from-minibuffer, i.e.

  (let ((history-var history-var))
    (read-from-minibuffer .... 'history-var ...))
  (add-to-history 'history-var ...)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Juri Linkov]

> Can't you let-bind the history variable around the call to
> read-from-minibuffer, i.e.
>
>   (let ((history-var history-var))
>     (read-from-minibuffer .... 'history-var ...))
>   (add-to-history 'history-var ...)

This was the first solution I tried, but after using it I noticed
that when I change the history list while the minibuffer is active
(e.g. I use M-k in the minibuffer to purge the history list from
unnecessary old elements, see
http://lists.gnu.org/archive/html/emacs-devel/2004-06/msg00937.html)
then after exiting from the minibuffer I lose all my changes in the
history list.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Richard Stallman]

    2. to change the argument KEEP-ALL to DONT-KEEP so that if DONT-KEEP
       is t, then read-from-minibuffer doesn't add a new element to the
       history list, and the calling code can add it by the explicit call
       of add-to-history (with any necessary arguments including the value t
       for KEEP-ALL argument of add-to-history).

Since we already decided to get rid of the KEEP-ALL argument,
I think that is the cleanest way.  It would require changing
the Lisp manual and etc/NEWS.

Would you like to do it?

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Juri Linkov]

>     2. to change the argument KEEP-ALL to DONT-KEEP so that if DONT-KEEP
>        is t, then read-from-minibuffer doesn't add a new element to the
>        history list, and the calling code can add it by the explicit call
>        of add-to-history (with any necessary arguments including the value t
>        for KEEP-ALL argument of add-to-history).
>
> Since we already decided to get rid of the KEEP-ALL argument,
> I think that is the cleanest way.  It would require changing
> the Lisp manual and etc/NEWS.

This solution has a significant drawback: this argument is available only
to one reading function `read-from-minibuffer' and is not available to
all other minibuffer reading functions (like `read-file-name' etc.).
Adding this argument to every reading function would be a clumsy solution.

I'm really inclined to bind a global variable `history-length' to
a special value before calling a reading function (and to access this
value in `read_minibuf' where all reading functions eventually go).

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Richard Stallman]

    This solution has a significant drawback: this argument is available only
    to one reading function `read-from-minibuffer' and is not available to
    all other minibuffer reading functions (like `read-file-name' etc.).
    Adding this argument to every reading function would be a clumsy solution.

That is a good point.

    I'm really inclined to bind a global variable `history-length' to
    a special value before calling a reading function (and to access this
    value in `read_minibuf' where all reading functions eventually go).

I agree that a global variable seems like a good solution,
but calling it `history-length' seems counterintuitive.
I suggest calling it `history-dont-add'.

Would you like to implement this?  Meanwhile, would you please
install the previous patch to delete KEEP_ALL, aside from the
parts that change the behavior of command-history, as I said before?

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Juri Linkov]

> I agree that a global variable seems like a good solution,
> but calling it `history-length' seems counterintuitive.
> I suggest calling it `history-dont-add'.

This name is too limited and has no room for extensions.
What do you think about `history-keep' with values `none', `all',
and possibly other values like `empty', `duplicate', `last' (to not compare
with the last element before adding a new one) or combinations of them?

> Meanwhile, would you please install the previous patch to delete
> KEEP_ALL, aside from the parts that change the behavior of command-history,
> as I said before?

Done.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Richard Stallman]

    > I agree that a global variable seems like a good solution,
    > but calling it `history-length' seems counterintuitive.
    > I suggest calling it `history-dont-add'.

    This name is too limited and has no room for extensions.
    What do you think about `history-keep' with values `none', `all',
    and possibly other values like `empty', `duplicate', `last' (to not compare
    with the last element before adding a new one) or combinations of them?

I don't mind using a variable name that opens the room for other extensions,
but we don't need those other extensions now.  So please call it
`history-add-new-input', and initially just define the values t and nil.

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Juri Linkov]

> I don't mind using a variable name that opens the room for other extensions,
> but we don't need those other extensions now.  So please call it
> `history-add-new-input', and initially just define the values t and nil.

Please see the patch below:

Index: src/minibuf.c
===================================================================
RCS file: /sources/emacs/emacs/src/minibuf.c,v
retrieving revision 1.304
diff -c -r1.304 minibuf.c
*** src/minibuf.c	25 May 2006 21:16:22 -0000	1.304
--- src/minibuf.c	30 May 2006 09:27:04 -0000
***************
*** 66,71 ****
--- 66,75 ----
  
  int history_delete_duplicates;
  
+ /* Non-nil means add new input to history.  */
+ 
+ Lisp_Object Vhistory_add_new_input;
+ 
  /* Fread_minibuffer leaves the input here as a string. */
  
  Lisp_Object last_minibuf_string;
***************
*** 749,759 ****
    else
      histstring = Qnil;
  
    /* Add the value to the appropriate history list, if any.  */
!   if (SYMBOLP (Vminibuffer_history_variable)
        && !NILP (histstring))
      {
        /* If the caller wanted to save the value read on a history list,
--- 753,766 ----
    else
      histstring = Qnil;
  
    /* Add the value to the appropriate history list, if any.  */
!   if (!NILP (Vhistory_add_new_input)
!       && SYMBOLP (Vminibuffer_history_variable)
        && !NILP (histstring))
      {
        /* If the caller wanted to save the value read on a history list,
***************
*** 2800,2808 ****
  If set to t when adding a new history element, all previous identical
  elements are deleted.  */);
    history_delete_duplicates = 0;
  
+   DEFVAR_LISP ("history-add-new-input", &Vhistory_add_new_input,
+ 	       doc: /* *Non-nil means to add new elements in history.
+ If set to nil, minibuffer reading functions don't add new elements to the
+ history list, so it is possible to do this afterwards by calling
+ `add-to-history' explicitly.  */);
+   Vhistory_add_new_input = Qt;
+ 
    DEFVAR_LISP ("completion-auto-help", &Vcompletion_auto_help,
  	       doc: /* *Non-nil means automatically provide help for invalid completion input.
  Under Partial Completion mode, a non-nil, non-t value has a special meaning;


Index: lispref/minibuf.texi
===================================================================
RCS file: /sources/emacs/emacs/lispref/minibuf.texi,v
retrieving revision 1.83
diff -c -r1.83 minibuf.texi
*** lispref/minibuf.texi	25 May 2006 22:57:50 -0000	1.83
--- lispref/minibuf.texi	30 May 2006 09:27:03 -0000
***************
*** 460,465 ****
--- 460,471 ----
  duplicates, and to add @var{newelt} to the list even if it is empty.
  @end defun
  
+ @defvar history-add-new-input
+ The value of this variable @code{nil} means that standard functions
+ that read from the minibuffer don't add new elements to the history
+ list, so it is possible to do this explicitly by using @code{add-to-history}.
+ @end defvar
+ 
  @defvar history-length
  The value of this variable specifies the maximum length for all
  history lists that don't specify their own maximum lengths.  If the

Index: etc/NEWS
===================================================================
RCS file: /sources/emacs/emacs/etc/NEWS,v
retrieving revision 1.1349
diff -c -r1.1349 NEWS
*** etc/NEWS	25 May 2006 21:53:31 -0000	1.1349
--- etc/NEWS	30 May 2006 09:27:33 -0000
***************
*** 4371,4376 ****
--- 4375,4386 ----
  It is like `read-file-name' except that the defaulting works better
  for directories, and completion inside it shows only directories.
  
+ +++
+ *** The new variable `history-add-new-input' specifies whether to add new
+ elements in history.  If set to nil, minibuffer reading functions don't
+ add new elements to the history list, so it is possible to do this
+ afterwards by calling `add-to-history' explicitly.
+ 
  ** Completion changes:
  
  +++

Index: lisp/replace.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/replace.el,v
retrieving revision 1.242
diff -c -r1.242 replace.el
*** lisp/replace.el	28 May 2006 17:05:38 -0000	1.242
--- lisp/replace.el	30 May 2006 09:27:28 -0000
***************
*** 99,105 ****
  wants to replace FROM with TO."
    (if query-replace-interactive
        (car (if regexp-flag regexp-search-ring search-ring))
!     (let  ((from
  	    ;; The save-excursion here is in case the user marks and copies
  	    ;; a region in order to specify the minibuffer input.
  	    ;; That should not clobber the region for the query-replace itself.
--- 99,106 ----
  wants to replace FROM with TO."
    (if query-replace-interactive
        (car (if regexp-flag regexp-search-ring search-ring))
!     (let* ((history-add-new-input nil)
! 	   (from
  	    ;; The save-excursion here is in case the user marks and copies
  	    ;; a region in order to specify the minibuffer input.
  	    ;; That should not clobber the region for the query-replace itself.
***************
*** 114,125 ****
 	  (cons (car query-replace-defaults)
  		(query-replace-compile-replacement
  		 (cdr query-replace-defaults) regexp-flag))
+ 	(add-to-history query-replace-from-history-variable from nil t)
   	;; Warn if user types \n or \t, but don't reject the input.
   	(and regexp-flag
   	     (string-match "\\(\\`\\|[^\\]\\)\\(\\\\\\\\\\)*\\(\\\\[nt]\\)" from)
***************
*** 174,183 ****
    "Query and return the `to' argument of a query-replace operation."
    (query-replace-compile-replacement
     (save-excursion
!      (let ((to (read-from-minibuffer
! 		(format "%s %s with: " prompt (query-replace-descr from))
! 		nil nil nil
! 		query-replace-to-history-variable from t)))
         (setq query-replace-defaults (cons from to))
         to))
     regexp-flag))
--- 176,187 ----
    "Query and return the `to' argument of a query-replace operation."
    (query-replace-compile-replacement
     (save-excursion
!      (let* ((history-add-new-input nil)
! 	    (to (read-from-minibuffer
! 		 (format "%s %s with: " prompt (query-replace-descr from))
! 		 nil nil nil
! 		 query-replace-to-history-variable from t)))
!        (add-to-history query-replace-to-history-variable to nil t)
         (setq query-replace-defaults (cons from to))
         to))
     regexp-flag))

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Richard Stallman]

Please install your change.

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Richard Stallman]

    The following patch should do that.  It also uses `add-to-history'
    to add the current search string to `query-replace-from-history-variable'
    to improve code in the same function.  This requires adding a new
    argument KEEP-ALL to `add-to-history'.

Why do you think it requires that?

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Juri Linkov]

>     The following patch should do that.  It also uses `add-to-history'
>     to add the current search string to `query-replace-from-history-variable'
>     to improve code in the same function.  This requires adding a new
>     argument KEEP-ALL to `add-to-history'.
>
> Why do you think it requires that?

The old code unconditionally adds a new element to the history list as:

!     (set query-replace-from-history-variable
!          (cons isearch-string
!                (symbol-value query-replace-from-history-variable)))

A new argument KEEP-ALL allows to do the same for the new code that uses
`add-to-history':

!     (add-to-history query-replace-from-history-variable isearch-string nil t)

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Richard Stallman]

    The old code unconditionally adds a new element to the history list as:

    !     (set query-replace-from-history-variable
    !          (cons isearch-string
    !                (symbol-value query-replace-from-history-variable)))

    A new argument KEEP-ALL allows to do the same for the new code that uses
    `add-to-history':

    !     (add-to-history query-replace-from-history-variable isearch-string nil t)

If I understand right, add-to-history unconditionally adds a new last
element.  Not so?

It also deletes older elements that match the new one.  Using
add-to-history will treat this new element just like other new
elements on the same history list.  That seems right to me.  Do you
think it is wrong in this case?

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     The old code unconditionally adds a new element to the history list as:
>
>     !     (set query-replace-from-history-variable
>     !          (cons isearch-string
>     !                (symbol-value query-replace-from-history-variable)))
>
>     A new argument KEEP-ALL allows to do the same for the new code that uses
>     `add-to-history':
>
>     !     (add-to-history query-replace-from-history-variable isearch-string nil t)
>
> If I understand right, add-to-history unconditionally adds a new last
> element.  Not so?
>
> It also deletes older elements that match the new one.  

Only if history-delete-duplicates is non-nil ... but it is nil by default.

>                                                         Using
> add-to-history will treat this new element just like other new
> elements on the same history list.  That seems right to me.  Do you
> think it is wrong in this case?

It is wrong if history-delete-duplicates is nil; then it unconditionally
adds an element even if it is equal to the head of the history.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Richard Stallman]

    >                                                         Using
    > add-to-history will treat this new element just like other new
    > elements on the same history list.  That seems right to me.  Do you
    > think it is wrong in this case?

    It is wrong if history-delete-duplicates is nil; then it unconditionally
    adds an element even if it is equal to the head of the history.

If history-delete-duplicates is nil, why is that behavior wrong?
Isn't it normal, in that case, to add the new element unconditionally?

Re: query-replace in isearch (was Re: should search ring contain duplicates?)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     >                                                         Using
>     > add-to-history will treat this new element just like other new
>     > elements on the same history list.  That seems right to me.  Do you
>     > think it is wrong in this case?
>
>     It is wrong if history-delete-duplicates is nil; then it unconditionally
>     adds an element even if it is equal to the head of the history.
>
> If history-delete-duplicates is nil, why is that behavior wrong?
> Isn't it normal, in that case, to add the new element unconditionally?

read-from-minibuffer doesn't add it unless KEEP-ALL is non-nil.

so for consistency, neither should add-to-history IMO.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[Juri Linkov]

> Below is a tested patch that removes `keep-all' from `read-from-minibuffer',
> adds duplicate replacement strings to the query-replace history explicitly,
> and fixes more places where the value of `history-delete-duplicates'
> is not taken into account yet (namely, `repeat-complex-command',
> `call-interactively').

What is the decision about my patch?  Is it OK to remove the argument
`keep-all' and to fix other history handling places?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: should search ring contain duplicates?

[Kim F. Storm]

Juri Linkov <juri@jurta.org> writes:

>> Below is a tested patch that removes `keep-all' from `read-from-minibuffer',

Did you test that with your changes, it still fixes the original problem
which was the reason for the original change? 

http://lists.gnu.org/archive/html/emacs-pretest-bug/2004-11/msg00287.html

>> adds duplicate replacement strings to the query-replace history explicitly,
>> and fixes more places where the value of `history-delete-duplicates'
>> is not taken into account yet (namely, `repeat-complex-command',
>> `call-interactively').
>
> What is the decision about my patch?  Is it OK to remove the argument
> `keep-all' and to fix other history handling places?

Modulo fixing the original problem, then you should install your patch.

But pls. consider using add-to-history instead of explicitly testing
for history-delete-duplicates.

BTW, add-to-history should probably be fixed to _not_ add an element
which is already at the head of the history.

Index: subr.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/subr.el,v
retrieving revision 1.509
diff -c -b -r1.509 subr.el
*** subr.el	10 May 2006 01:58:37 -0000	1.509
--- subr.el	10 May 2006 09:35:21 -0000
***************
*** 1138,1143 ****
--- 1138,1145 ----
  	tail)
      (if history-delete-duplicates
  	(setq history (delete newelt history)))
+     (unless (and history
+ 		 (equal (car history) newlet))
        (setq history (cons newelt history))
        (when (integerp maxelt)
  	(if (= 0 maxelt)
***************
*** 1145,1151 ****
  	(setq tail (nthcdr (1- maxelt) history))
  	(when (consp tail)
  	  (setcdr tail nil))))
!     (set history-var history)))
  
  \f
  ;;;; Mode hooks.
--- 1147,1153 ----
  	  (setq tail (nthcdr (1- maxelt) history))
  	  (when (consp tail)
  	    (setcdr tail nil))))
!       (set history-var history))))
  

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[Juri Linkov]

>>> Below is a tested patch that removes `keep-all' from `read-from-minibuffer',
>
> Did you test that with your changes, it still fixes the original problem
> which was the reason for the original change?
>
> http://lists.gnu.org/archive/html/emacs-pretest-bug/2004-11/msg00287.html

I tested it with all test cases from this thread, but that was before
adding add-to-history to CVS.

> But pls. consider using add-to-history instead of explicitly testing
> for history-delete-duplicates.

To use add-to-history there should be a way to tell read-from-minibuffer
not to add new elements to the history list, so add-to-history could
undertake this task itself later.  nil as the `hist' argument of
read-from-minibuffer doesn't count because the history list should be
available for M-p.

> BTW, add-to-history should probably be fixed to _not_ add an element
> which is already at the head of the history.

Then it becomes a complete duplicate of C code.  Isn't then better
to call the Lisp function add-to-history from the C implementation of
read_minibuf?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: should search ring contain duplicates?

[Kim F. Storm]

Juri Linkov <juri@jurta.org> writes:

>>>> Below is a tested patch that removes `keep-all' from `read-from-minibuffer',
>>
>> Did you test that with your changes, it still fixes the original problem
>> which was the reason for the original change?
>>
>> http://lists.gnu.org/archive/html/emacs-pretest-bug/2004-11/msg00287.html
>
> I tested it with all test cases from this thread, but that was before
> adding add-to-history to CVS.
>
>> But pls. consider using add-to-history instead of explicitly testing
>> for history-delete-duplicates.
>
> To use add-to-history there should be a way to tell read-from-minibuffer
> not to add new elements to the history list, so add-to-history could
> undertake this task itself later.  nil as the `hist' argument of
> read-from-minibuffer doesn't count because the history list should be
> available for M-p.
>
>> BTW, add-to-history should probably be fixed to _not_ add an element
>> which is already at the head of the history.
>
> Then it becomes a complete duplicate of C code.  

The intention of add-to-history is to provide the same functionality as
the C code -- so that Lisp code can be sure to DTRT when they add 
stuff to a history list "manually".

>                                                  Isn't then better
> to call the Lisp function add-to-history from the C implementation of
> read_minibuf?

I have no opinion here.

Another approach would be to implement add-to-history in C (it's
practically already there), and export it to Lisp.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[Richard Stallman]

    > BTW, add-to-history should probably be fixed to _not_ add an element
    > which is already at the head of the history.

    Then it becomes a complete duplicate of C code.  Isn't then better
    to call the Lisp function add-to-history from the C implementation of
    read_minibuf?

Looking at the code of add-to-history, it appears to me that it
already behaves as requested here.  If the new element matches the
head of the history list, add-to-history does not add another copy.

More precisely, it does not preserve the old head.
Instead it deletes that and then adds a new element.
But the result is, all the same, to have just one, not two.
This is all that matters, as far as I can see.

Is there a real issue here?

Re: should search ring contain duplicates?

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     > BTW, add-to-history should probably be fixed to _not_ add an element
>     > which is already at the head of the history.
>
>     Then it becomes a complete duplicate of C code.  Isn't then better
>     to call the Lisp function add-to-history from the C implementation of
>     read_minibuf?
>
> Looking at the code of add-to-history, it appears to me that it
> already behaves as requested here.  If the new element matches the
> head of the history list, add-to-history does not add another copy.

By default, it does, since history-delete-duplicates is nil.

>
> More precisely, it does not preserve the old head.
> Instead it deletes that and then adds a new element.
> But the result is, all the same, to have just one, not two.
> This is all that matters, as far as I can see.

It depends on the setting of history-delete-duplicates, whereas
the code in read-from-minibuffer does not.

> Is there a real issue here?

Yes.

I looked closer at the code in read_minibuf and add-to-history, and
there are a couple of other inconsistencies, which should be fixed.

The following version of add-to-history which obeys the same rules
as read-from-minibuffer, including the keep-all arg.

(defun add-to-history (history-var newelt &optional maxelt keep-all)
  "Add NEWELT to the history list stored in the variable HISTORY-VAR.
Return the new history list.
If MAXELT is non-nil, it specifies the maximum length of the history.
Otherwise, the maximum history length is the value of the `history-length'
property on symbol HISTORY-VAR, if set, or the value of the `history-length'
variable.
Remove duplicates of NEWELT unless `history-delete-duplicates' is nil.
If optional fourth arg KEEP-ALL is non-nil, add NEWELT to history even
if it is empty or a duplicate."
  (unless maxelt
    (setq maxelt (or (get history-var 'history-length)
		     history-length)))
  (let ((history (symbol-value history-var))
	tail)
    (when (and (listp history)
	       (or keep-all
		   (not (stringp newelt))
		   (> (length newelt) 0))
	       (or keep-all
		   (not (equal (car history) newelt))))
      (if history-delete-duplicates
	  (delete newelt history))
      (setq history (cons newelt history))
      (when (integerp maxelt)
	(if (= 0 maxelt)
	    (setq history nil)
	  (setq tail (nthcdr (1- maxelt) history))
	  (when (consp tail)
	    (setcdr tail nil)))))
    (set history-var history)))


[It accepts a non-string element, so it can be used in more cases
 which are not directly related to a minibuffer history variable,
 such as the kmacro-kill-ring, which already uses it].


Can I install this, so we can get on with the release :-)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[Richard Stallman]

    > More precisely, it does not preserve the old head.
    > Instead it deletes that and then adds a new element.
    > But the result is, all the same, to have just one, not two.
    > This is all that matters, as far as I can see.

    It depends on the setting of history-delete-duplicates, whereas
    the code in read-from-minibuffer does not.

Ok, now I see the issue.

    The following version of add-to-history which obeys the same rules
    as read-from-minibuffer, including the keep-all arg.

Thanks, and please install it.
Please update the Lisp Manual and etc/NEWS, too.

Thanks.

Re: should search ring contain duplicates?

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     The following version of add-to-history which obeys the same rules
>     as read-from-minibuffer, including the keep-all arg.
>
> Thanks, and please install it.
> Please update the Lisp Manual and etc/NEWS, too.

Done (expect for NEWS which doesn't go into details).

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

C-f in isearch minibuffer (was: should search ring contain duplicates?)

[Juri Linkov]

> I think this is a good idea.  This would help to get rid of a new
> useless argument `keep-all' added a few months ago to the function
> `read-from-minibuffer' only for the sake of query-replace.
> Since query-replace is the only place in Emacs that uses this new argument,
> it is better to remove it now before the release, and to use a new
> macro `history-push' in `query-replace-read-to' to treat the query-replace
> history specially.

This reminds me of another misfeature.  C-f typed at the end of the
isearch's minibuffer that pulls characters from the parent buffer is
not user-friendly.  With recently proposed special commands like `M-.'
in the minibuffer to pull things from the parent buffer to be implemented
in the next release, I think it is better to get rid of this misfeature
before this release:

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.288
diff -c -r1.288 isearch.el
*** lisp/isearch.el	5 May 2006 23:37:31 -0000	1.288
--- lisp/isearch.el	11 May 2006 11:55:45 -0000
***************
*** 418,425 ****
      (define-key map "\M-\t" 'isearch-complete-edit)
      (define-key map "\C-s"  'isearch-forward-exit-minibuffer)
      (define-key map "\C-r"  'isearch-reverse-exit-minibuffer)
-     (define-key map "\C-f"  'isearch-yank-char-in-minibuffer)
-     (define-key map [right] 'isearch-yank-char-in-minibuffer)
      map)
    "Keymap for editing isearch strings in the minibuffer.")
  
--- 418,423 ----
***************
*** 1322,1338 ****
  	  (goto-char isearch-other-end))
       (buffer-substring-no-properties (point) (funcall jumpform)))))
  
- (defun isearch-yank-char-in-minibuffer (&optional arg)
-   "Pull next character from buffer into end of search string in minibuffer."
-   (interactive "p")
-   (if (eobp)
-       (insert
-        (save-excursion
-          (set-buffer (cadr (buffer-list)))
-          (buffer-substring-no-properties
-           (point) (progn (forward-char arg) (point)))))
-     (forward-char arg)))
- 
  (defun isearch-yank-char (&optional arg)
    "Pull next character from buffer into search string."
    (interactive "p")
--- 1341,1346 ----


-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: C-f in isearch minibuffer

[Miles Bader]

Juri Linkov <juri@jurta.org> writes:
> This reminds me of another misfeature.  C-f typed at the end of the
> isearch's minibuffer that pulls characters from the parent buffer is
> not user-friendly.  ...  I think it is better to get rid of this
> misfeature before this release:

Why is it a "misfeature"?  It seems harmless enough...

-Miles
-- 
P.S.  All information contained in the above letter is false,
      for reasons of military security.

Re: C-f in isearch minibuffer

[Juri Linkov]

>> This reminds me of another misfeature.  C-f typed at the end of the
>> isearch's minibuffer that pulls characters from the parent buffer is
>> not user-friendly.  ...  I think it is better to get rid of this
>> misfeature before this release:
>
> Why is it a "misfeature"?  It seems harmless enough...

I recall objections from Richard and Stefan about the unexpected behavior
of C-f at the end of the minibuffer:
http://lists.gnu.org/archive/html/emacs-devel/2004-07/msg00164.html
http://lists.gnu.org/archive/html/emacs-devel/2005-04/msg00578.html

With the recent idea from Drew I now think than adding a new general
keybinding `M-.' to every minibuffer (in the next release) to pull
different things (characters, words, file names) is a good general
replacement for this feature.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: C-f in isearch minibuffer (was: should search ring contain duplicates?)

[Richard Stallman]

    This reminds me of another misfeature.  C-f typed at the end of the
    isearch's minibuffer that pulls characters from the parent buffer is
    not user-friendly.  With recently proposed special commands like `M-.'
    in the minibuffer to pull things from the parent buffer to be implemented
    in the next release, I think it is better to get rid of this misfeature
    before this release:

I don't think it a is bad feature, and the proposal is just a proposal.
Please do not change this now.

Re: C-f in isearch minibuffer

[Juri Linkov]

>     This reminds me of another misfeature.  C-f typed at the end of the
>     isearch's minibuffer that pulls characters from the parent buffer is
>     not user-friendly.  With recently proposed special commands like `M-.'
>     in the minibuffer to pull things from the parent buffer to be implemented
>     in the next release, I think it is better to get rid of this misfeature
>     before this release:
>
> I don't think it a is bad feature, and the proposal is just a proposal.

In http://lists.gnu.org/archive/html/emacs-devel/2004-07/msg00164.html
you opposed to such feature as:

,----
|     > For what it's worth, I can also do
|     >
|     >    M-% C-w C-w C-w RET
|     >
|     > since I've added a C-w binding in minibuffer-local-map that mimics
|     > isearch's C-w.
| 
|     Why not add something like this to Emacs?  Or maybe more general
|     character-based command like typing C-f in the end of the minibuffer
|     to pull text from the source buffer character by character to the minibuffer.
| 
| No, no, no!  Let's not have more special casing.  People should be
| able to rely on standard Emacs commands to do the standard thing.
| 
| You can switch windows from the minibuffer to copy text into it.
`----

I now agree that it's better not to change the standard Emacs behavior,
but to add a new special keybinding (like `M-.') to pull characters/words
from the original buffer.  This feature could be implemented in the next
release, but C-f removed before the release to not expose non-standard C-f
at the end of the minibuffer to a large user base when there is a better
possible feature.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: C-f in isearch minibuffer

[Richard Stallman]

Looking at this now, I think that this added meaning of C-f is not as
bad, as a special case, as most of the special search commands,
because it is upward compatible with the usual meaning of C-f.  It is
only special in a case where otherwise it would get an error.

Meanwhile, I have doubts that the M-. proposal is any good.

Re: should search ring contain duplicates?

[Stefan Monnier]

> `isearch-update-ring' could at least do that when
> history-delete-duplicates is t.

BTW, is there any reason why history-delete-duplicates defaults to nil?


        Stefan

Re: should search ring contain duplicates?

[Kim F. Storm]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> `isearch-update-ring' could at least do that when
>> history-delete-duplicates is t.
>
> BTW, is there any reason why history-delete-duplicates defaults to nil?

Perhaps, so we don't get annoyed by the places which don't obey it?   :-)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

RE: should search ring contain duplicates?

[Drew Adams]

    BTW, is there any reason why history-delete-duplicates
    defaults to nil?

I agree it should be t.

I didn't even realize that variable existed when I started this thread,
which is why I asked - why not (always) remove duplicates?  I still wonder
what the nil use case is - why would anyone want to keep duplicates?

RE: should search ring contain duplicates?

[Drew Adams]

        BTW, is there any reason why history-delete-duplicates
        defaults to nil?

      I agree it should be t.

      I didn't even realize that variable existed when I started this
      thread, which is why I asked - why not (always) remove duplicates?
      I still wonder what the nil use case is - why would anyone want to
      keep duplicates?

    See the thread at
    http://lists.gnu.org/archive/html/emacs-devel/2004-09/msg00038.html

Thx. The only argument I see in that thread for ever allowing duplicates is
the point RMS raised about a command history. I can see that that might make
some sense in a shell interaction (although there are better ways to see the
shell history with duplicates). I don't see where it makes sense for, say,
the Emacs command history (and certainly not for histories like the
search-ring). But perhaps I'm missing something.

I think that for most uses of a history it makes no sense to keep
duplicates. I'd vote for making the default value be t and letting those few
modes where duplicates might make sense (e.g. shell-mode?) bind it to nil
unless the user has explicitly specified otherwise.

IOW the option values could be:

 nil            - means never remove duplicates
 t (default)    - means remove duplicates, but this can be
                  overridden by a mode (e.g. shell-mode)
 non-nil, non-t - means always remove duplicates (never override)

This would require code changes only for those few modes that want to
override the default (t). Plus a change to the defcustom.

Re: should search ring contain duplicates?

[Juri Linkov]

> I think that for most uses of a history it makes no sense to keep
> duplicates. I'd vote for making the default value be t and letting those few
> modes where duplicates might make sense (e.g. shell-mode?) bind it to nil
> unless the user has explicitly specified otherwise.
>
> IOW the option values could be:
>
>  nil            - means never remove duplicates
>  t (default)    - means remove duplicates, but this can be
>                   overridden by a mode (e.g. shell-mode)
>  non-nil, non-t - means always remove duplicates (never override)
>
> This would require code changes only for those few modes that want to
> override the default (t). Plus a change to the defcustom.

A related feature that specifies the maximum length of the history list
uses the `history-length' _property_ of the history list _symbol_
to override the default value of the _variable_ `history-length'
for a particular history list.  Exactly the same thing could be
implemented for `history-delete-duplicates', i.e. the property
`history-delete-duplicates' would override its default value
for a particular history list.

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: should search ring contain duplicates?

[Drew Adams]

    > I think that for most uses of a history it makes no sense to keep
    > duplicates. I'd vote for making the default value be t and
    > letting those few
    > modes where duplicates might make sense (e.g. shell-mode?)
    > bind it to nil
    > unless the user has explicitly specified otherwise.
    >
    > IOW the option values could be:
    >
    >  nil            - means never remove duplicates
    >  t (default)    - means remove duplicates, but this can be
    >                   overridden by a mode (e.g. shell-mode)
    >  non-nil, non-t - means always remove duplicates (never override)
    >
    > This would require code changes only for those few modes that want to
    > override the default (t). Plus a change to the defcustom.

    A related feature that specifies the maximum length of the history list
    uses the `history-length' _property_ of the history list _symbol_
    to override the default value of the _variable_ `history-length'
    for a particular history list.  Exactly the same thing could be
    implemented for `history-delete-duplicates', i.e. the property
    `history-delete-duplicates' would override its default value
    for a particular history list.

Clever, but maybe too clever. Non-lisper end users should be able to easily
change options, and they should be able to do so using Customize (and save
such preferences). Making users use property lists to override default
values is not a good way to go, IMO.

Re: should search ring contain duplicates?

[Richard Stallman]

I am not interested in considering such a complex proposal.
Please let's not spend time discussing it.

Re: should search ring contain duplicates?

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

> I am not interested in considering such a complex proposal.

There is nothing complex about it (with the latest add-to-history)
version.

> Please let's not spend time discussing it.

Is it better to spend the time on finding and fixing bugs like this, when
people copy/paste code ?? :

	(when (> (length regexp-search-ring) regexp-search-ring-max)
	  (setcdr (nthcdr (1- search-ring-max) regexp-search-ring) nil)))


There are many places where this would be useful!

:-(

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[Kim F. Storm]

storm@cua.dk (Kim F. Storm) writes:


> There are many places where this would be useful!

For example:

Index: lisp/subr.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/subr.el,v
retrieving revision 1.505
diff -c -r1.505 subr.el
*** lisp/subr.el	29 Apr 2006 13:56:19 -0000	1.505
--- lisp/subr.el	4 May 2006 22:44:11 -0000
***************
*** 1122,1127 ****
--- 1124,1155 ----
  			    (if (and oa ob)
  				(< oa ob)
  			      oa)))))))
+ 
+ (defun add-to-history (history-var newelt &optional maxelt keep-dups)
+   "Add NEWELT to the history list stored in the variable HISTORY-VAR.
+ Return the new history list.
+ If MAXELT is non-nil, it specifies the maximum length of the history.
+ Otherwise, the maximum history length is the value of the `history-length'
+ property on symbol HISTORY-VAR, if set, or the value of the `history-length'
+ variable.
+ Remove duplicates of NEWELT unless `history-delete-duplicates' is nil
+ or KEEP-DUPS is non-nil."
+   (unless maxelt
+     (setq maxelt (or (get history-var 'history-length)
+ 		     history-length)))
+   (let ((history (symbol-value history-var))
+ 	tail)
+     (if (and history-delete-duplicates (not keep-dups))
+ 	(setq history (delete newelt history)))
+     (setq history (cons newelt history))
+     (when (integerp maxelt)
+       (if (= 0 maxelt)
+ 	  (setq history nil)
+ 	(setq tail (nthcdr (1- maxelt) history))
+ 	(when (consp tail)
+ 	  (setcdr tail nil))))
+     (set history-var history)))
+ 
  \f
  ;;;; Mode hooks.

Index: lisp/ediff.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/ediff.el,v
retrieving revision 1.78
diff -c -r1.78 ediff.el
*** lisp/ediff.el	19 Feb 2006 03:16:44 -0000	1.78
--- lisp/ediff.el	4 May 2006 22:41:46 -0000
***************
*** 210,221 ****
  					   ediff-last-dir-B
  					 (file-name-directory f)))
  				 (progn
! 				   (setq file-name-history
! 					 (cons (ediff-abbreviate-file-name
! 						(expand-file-name
! 						 (file-name-nondirectory f)
! 						 dir-B))
! 					       file-name-history))
  				   (ediff-get-default-file-name f 1)))
  	   )))
    (ediff-files-internal file-A
--- 210,220 ----
  					   ediff-last-dir-B
  					 (file-name-directory f)))
  				 (progn
! 				   (add-to-history 'file-name-history
! 						   (ediff-abbreviate-file-name
! 						    (expand-file-name
! 						     (file-name-nondirectory f)
! 						     dir-B)))
  				   (ediff-get-default-file-name f 1)))
  	   )))
    (ediff-files-internal file-A
***************
*** 246,270 ****
  						    ediff-last-dir-B
  						  (file-name-directory f)))
  					  (progn
! 					    (setq file-name-history
! 						  (cons
! 						   (ediff-abbreviate-file-name
! 						    (expand-file-name
! 						     (file-name-nondirectory f)
! 						     dir-B))
! 						   file-name-history))
  					    (ediff-get-default-file-name f 1))))
  	   (ediff-read-file-name "File C to compare"
  				 (setq dir-C (if ediff-use-last-dir
  						 ediff-last-dir-C
  					       (file-name-directory ff)))
  				 (progn
! 				   (setq file-name-history
! 					 (cons (ediff-abbreviate-file-name
! 						(expand-file-name
! 						 (file-name-nondirectory ff)
! 						 dir-C))
! 					       file-name-history))
  				   (ediff-get-default-file-name ff 2)))
  	   )))
    (ediff-files-internal file-A
--- 245,266 ----
  						    ediff-last-dir-B
  						  (file-name-directory f)))
  					  (progn
! 					    (add-to-history 'file-name-history
! 							    (ediff-abbreviate-file-name
! 							     (expand-file-name
! 							      (file-name-nondirectory f)
! 							      dir-B)))
  					    (ediff-get-default-file-name f 1))))
  	   (ediff-read-file-name "File C to compare"
  				 (setq dir-C (if ediff-use-last-dir
  						 ediff-last-dir-C
  					       (file-name-directory ff)))
  				 (progn
! 				   (add-to-history 'file-name-history
! 						   (ediff-abbreviate-file-name
! 						    (expand-file-name
! 						     (file-name-nondirectory ff)
! 						     dir-C)))
  				   (ediff-get-default-file-name ff 2)))
  	   )))
    (ediff-files-internal file-A
***************
*** 1109,1120 ****
  					   ediff-last-dir-B
  					 (file-name-directory f)))
  				 (progn
! 				   (setq file-name-history
! 					 (cons (ediff-abbreviate-file-name
! 						(expand-file-name
! 						 (file-name-nondirectory f)
! 						 dir-B))
! 					       file-name-history))
  				   (ediff-get-default-file-name f 1)))
  	   )))
    (setq startup-hooks (cons 'ediff-merge-on-startup startup-hooks))
--- 1105,1115 ----
  					   ediff-last-dir-B
  					 (file-name-directory f)))
  				 (progn
! 				   (add-to-history 'file-name-history
! 						   (ediff-abbreviate-file-name
! 						    (expand-file-name
! 						     (file-name-nondirectory f)
! 						     dir-B)))
  				   (ediff-get-default-file-name f 1)))
  	   )))
    (setq startup-hooks (cons 'ediff-merge-on-startup startup-hooks))
***************
*** 1153,1165 ****
  						    ediff-last-dir-B
  						  (file-name-directory f)))
  					  (progn
! 					    (setq file-name-history
! 						  (cons
! 						   (ediff-abbreviate-file-name
! 						    (expand-file-name
! 						     (file-name-nondirectory f)
! 						     dir-B))
! 						   file-name-history))
  					    (ediff-get-default-file-name f 1))))
  	   (ediff-read-file-name "Ancestor file"
  				 (setq dir-ancestor
--- 1148,1158 ----
  						    ediff-last-dir-B
  						  (file-name-directory f)))
  					  (progn
! 					    (add-to-history 'file-name-history
! 							    (ediff-abbreviate-file-name
! 							     (expand-file-name
! 							      (file-name-nondirectory f)
! 							      dir-B)))
  					    (ediff-get-default-file-name f 1))))
  	   (ediff-read-file-name "Ancestor file"
  				 (setq dir-ancestor
***************
*** 1167,1178 ****
  					   ediff-last-dir-ancestor
  					 (file-name-directory ff)))
  				 (progn
! 				   (setq file-name-history
! 					 (cons (ediff-abbreviate-file-name
! 						(expand-file-name
! 						 (file-name-nondirectory ff)
! 						 dir-ancestor))
! 					       file-name-history))
  				   (ediff-get-default-file-name ff 2)))
  	   )))
    (setq startup-hooks (cons 'ediff-merge-on-startup startup-hooks))
--- 1160,1170 ----
  					   ediff-last-dir-ancestor
  					 (file-name-directory ff)))
  				 (progn
! 				   (add-to-history 'file-name-history
! 						   (ediff-abbreviate-file-name
! 						    (expand-file-name
! 						     (file-name-nondirectory ff)
! 						     dir-ancestor)))
  				   (ediff-get-default-file-name ff 2)))
  	   )))
    (setq startup-hooks (cons 'ediff-merge-on-startup startup-hooks))


Index: lisp/env.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/env.el,v
retrieving revision 1.36
diff -c -r1.36 env.el
*** lisp/env.el	18 Apr 2006 21:17:50 -0000	1.36
--- lisp/env.el	4 May 2006 22:42:16 -0000
***************
*** 117,123 ****
       (let* ((var (read-envvar-name "Set environment variable: " nil))
  	    (value (getenv var)))
         (when value
! 	 (push value setenv-history))
         ;; Here finally we specify the args to give call setenv with.
         (list var
  	     (read-from-minibuffer (format "Set %s to value: " var)
--- 117,123 ----
       (let* ((var (read-envvar-name "Set environment variable: " nil))
  	    (value (getenv var)))
         (when value
! 	 (add-to-history 'setenv-history value))
         ;; Here finally we specify the args to give call setenv with.
         (list var
  	     (read-from-minibuffer (format "Set %s to value: " var)


Index: lisp/isearch.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.287
diff -c -r1.287 isearch.el
*** lisp/isearch.el	3 May 2006 23:27:53 -0000	1.287
--- lisp/isearch.el	4 May 2006 22:42:46 -0000
***************
*** 831,851 ****
  (defun isearch-update-ring (string &optional regexp)
    "Add STRING to the beginning of the search ring.
  REGEXP if non-nil says use the regexp search ring."
!   (if regexp
!       (when (or (null regexp-search-ring)
! 		(not (string= string (car regexp-search-ring))))
! 	(when history-delete-duplicates
! 	  (setq regexp-search-ring (delete string regexp-search-ring)))
! 	(push string regexp-search-ring)
! 	(when (> (length regexp-search-ring) regexp-search-ring-max)
! 	  (setcdr (nthcdr (1- search-ring-max) regexp-search-ring) nil)))
!     (when (or (null search-ring)
! 	      (not (string= string (car search-ring))))
!       (when history-delete-duplicates
! 	(setq search-ring (delete string search-ring)))
!       (push string search-ring)
!       (when (> (length search-ring) search-ring-max)
! 	(setcdr (nthcdr (1- search-ring-max) search-ring) nil)))))
  
  ;; Switching buffers should first terminate isearch-mode.
  ;; ;; For Emacs 19, the frame switch event is handled.
--- 831,839 ----
  (defun isearch-update-ring (string &optional regexp)
    "Add STRING to the beginning of the search ring.
  REGEXP if non-nil says use the regexp search ring."
!   (add-to-history
!    (if regexp 'regexp-search-ring 'search-ring)
!    (if regexp regexp-search-ring-max search-ring-max)))
  
  ;; Switching buffers should first terminate isearch-mode.
  ;; ;; For Emacs 19, the frame switch event is handled.


Index: lisp/kmacro.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/kmacro.el,v
retrieving revision 1.32
diff -c -r1.32 kmacro.el
*** lisp/kmacro.el	6 Feb 2006 14:33:34 -0000	1.32
--- lisp/kmacro.el	4 May 2006 22:43:11 -0000
***************
*** 349,358 ****
  (defun kmacro-push-ring (&optional elt)
    "Push ELT or current macro onto `kmacro-ring'."
    (when (setq elt (or elt (kmacro-ring-head)))
!     (let ((len (length kmacro-ring)))
!       (setq kmacro-ring (cons elt kmacro-ring))
!       (if (>= len kmacro-ring-max)
! 	  (setcdr (nthcdr len kmacro-ring) nil)))))
  
  
  (defun kmacro-split-ring-element (elt)
--- 349,355 ----
  (defun kmacro-push-ring (&optional elt)
    "Push ELT or current macro onto `kmacro-ring'."
    (when (setq elt (or elt (kmacro-ring-head)))
!     (add-to-history 'kmacro-ring elt kmacro-ring-max t)))
  
  
  (defun kmacro-split-ring-element (elt)
***************
*** 377,387 ****
      (kmacro-pop-ring1 raw)))
  
  
- (defun kmacro-ring-length ()
-   "Return length of macro ring, including pseudo head."
-   (+ (if last-kbd-macro 1 0) (length kmacro-ring)))
- 
- 
  (defun kmacro-ring-empty-p (&optional none)
    "Tell user and return t if `last-kbd-macro' is nil or `kmacro-ring' is empty.
  Check only `last-kbd-macro' if optional arg NONE is non-nil."
--- 374,379 ----
***************
*** 577,589 ****
      (let ((append (and arg (listp arg))))
        (unless append
  	(if last-kbd-macro
! 	    (let ((len (length kmacro-ring)))
! 	      (setq kmacro-ring
! 		    (cons
! 		     (list last-kbd-macro kmacro-counter kmacro-counter-format-start)
! 		     kmacro-ring))
! 	      (if (>= len kmacro-ring-max)
! 		  (setcdr (nthcdr len kmacro-ring) nil))))
  	(setq kmacro-counter (or (if arg (prefix-numeric-value arg))
  				 kmacro-initial-counter-value
  				 0)
--- 569,576 ----
      (let ((append (and arg (listp arg))))
        (unless append
  	(if last-kbd-macro
! 	    (kmacro-push-ring
! 	     (list last-kbd-macro kmacro-counter kmacro-counter-format-start)))
  	(setq kmacro-counter (or (if arg (prefix-numeric-value arg))
  				 kmacro-initial-counter-value
  				 0)

Index: lisp/server.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/server.el,v
retrieving revision 1.109
diff -c -r1.109 server.el
*** lisp/server.el	4 Mar 2006 16:06:46 -0000	1.109
--- lisp/server.el	4 May 2006 22:43:33 -0000
***************
*** 411,417 ****
  	;; deleted file, offer to write it.
  	(let* ((filen (car file))
  	       (obuf (get-file-buffer filen)))
! 	  (push filen file-name-history)
  	  (if (and obuf (set-buffer obuf))
  	      (progn
  		(cond ((file-exists-p filen)
--- 411,417 ----
  	;; deleted file, offer to write it.
  	(let* ((filen (car file))
  	       (obuf (get-file-buffer filen)))
! 	  (add-to-history 'file-name-history filen)
  	  (if (and obuf (set-buffer obuf))
  	      (progn
  		(cond ((file-exists-p filen)

Index: lisp/progmodes/grep.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/grep.el,v
retrieving revision 1.55
diff -c -r1.55 grep.el
*** lisp/progmodes/grep.el	2 May 2006 08:49:59 -0000	1.55
--- lisp/progmodes/grep.el	4 May 2006 22:43:51 -0000
***************
*** 676,682 ****
  	      (setq command
  		    (read-from-minibuffer "Confirm: "
  					  command nil nil 'grep-history))
! 	    (push command grep-history))))
        (when command
  	;; Setting process-setup-function makes exit-message-function work
  	;; even when async processes aren't supported.
--- 676,682 ----
  	      (setq command
  		    (read-from-minibuffer "Confirm: "
  					  command nil nil 'grep-history))
! 	    (add-to-history 'grep-history command))))
        (when command
  	;; Setting process-setup-function makes exit-message-function work
  	;; even when async processes aren't supported.
***************
*** 742,748 ****
  	      (setq command
  		    (read-from-minibuffer "Confirm: "
  					  command nil nil 'grep-find-history))
! 	    (push command grep-find-history))
  	  (compilation-start command 'grep-mode))))))
  
  
--- 742,748 ----
  	      (setq command
  		    (read-from-minibuffer "Confirm: "
  					  command nil nil 'grep-find-history))
! 	    (add-to-history 'grep-find-history command))
  	  (compilation-start command 'grep-mode))))))
  
  
  
Index: lisp/progmodes/vhdl-mode.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/vhdl-mode.el,v
retrieving revision 1.50
diff -c -r1.50 vhdl-mode.el
*** lisp/progmodes/vhdl-mode.el	10 Feb 2006 09:00:30 -0000	1.50
--- lisp/progmodes/vhdl-mode.el	4 May 2006 22:45:20 -0000
***************
*** 16723,16730 ****
  	  (progn (save-buffer)
  		 (kill-buffer (current-buffer))
  		 (set-buffer orig-buffer)
! 		 (setq file-name-history
! 		       (cons makefile-path-name file-name-history)))
  	(vhdl-warning-when-idle
  	 (format "File not writable: \"%s\""
  		 (abbreviate-file-name makefile-path-name)))
--- 16723,16729 ----
  	  (progn (save-buffer)
  		 (kill-buffer (current-buffer))
  		 (set-buffer orig-buffer)
! 		 (add-to-history 'file-name-history makefile-path-name))
  	(vhdl-warning-when-idle
  	 (format "File not writable: \"%s\""
  		 (abbreviate-file-name makefile-path-name)))

Index: lisp/progmodes/xscheme.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/xscheme.el,v
retrieving revision 1.9
diff -c -r1.9 xscheme.el
*** lisp/progmodes/xscheme.el	10 Feb 2006 09:00:30 -0000	1.9
--- lisp/progmodes/xscheme.el	4 May 2006 22:45:38 -0000
***************
*** 580,591 ****
  ;;;; Scheme expressions ring
  
  (defun xscheme-insert-expression (string)
!   (setq xscheme-expressions-ring (cons string xscheme-expressions-ring))
!   (if (> (length xscheme-expressions-ring) xscheme-expressions-ring-max)
!       (setcdr (nthcdr (1- xscheme-expressions-ring-max)
! 		      xscheme-expressions-ring)
! 	      nil))
!   (setq xscheme-expressions-ring-yank-pointer xscheme-expressions-ring))
  
  (defun xscheme-rotate-yank-pointer (arg)
    "Rotate the yanking point in the kill ring."
--- 580,588 ----
  ;;;; Scheme expressions ring
  
  (defun xscheme-insert-expression (string)
!   (setq xscheme-expressions-ring-yank-pointer
! 	(add-to-history 'xscheme-expressions-ring string
! 			xscheme-expressions-ring-max)))
  
  (defun xscheme-rotate-yank-pointer (arg)
    "Rotate the yanking point in the kill ring."

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: should search ring contain duplicates?

[Richard Stallman]

    > I am not interested in considering such a complex proposal.

    There is nothing complex about it (with the latest add-to-history)
    version.

I was talking about a different proposal, one involving properties to
control lengths.  It was very complex.

RE: should search ring contain duplicates?

[Drew Adams]

        > I am not interested in considering such a complex proposal.

        There is nothing complex about it (with the latest add-to-history)
        version.
    
    I was talking about a different proposal, one involving properties to
    control lengths.  It was very complex.
    
To be clear, was it this proposal from Juri:

    A related feature that specifies the maximum length of the history list
    uses the `history-length' _property_ of the history list _symbol_
    to override the default value of the _variable_ `history-length'
    for a particular history list.  Exactly the same thing could be
    implemented for `history-delete-duplicates', i.e. the property
    `history-delete-duplicates' would override its default value
    for a particular history list.

Re: should search ring contain duplicates?

[David Kastrup]

"Drew Adams" <drew.adams@oracle.com> writes:

>         > I am not interested in considering such a complex proposal.
>
>         There is nothing complex about it (with the latest add-to-history)
>         version.
>     
>     I was talking about a different proposal, one involving properties to
>     control lengths.  It was very complex.
>     
> To be clear, was it this proposal from Juri:
>
>     A related feature that specifies the maximum length of the history list
>     uses the `history-length' _property_ of the history list _symbol_
>     to override the default value of the _variable_ `history-length'
>     for a particular history list.  Exactly the same thing could be
>     implemented for `history-delete-duplicates', i.e. the property
>     `history-delete-duplicates' would override its default value
>     for a particular history list.

I don't know how you gather this when Richard directly replied to the
posting of yours
Message-ID: <DNEMKBNJBGPAOPIJOOICAEJDDFAA.drew.adams@oracle.com>
containing the proposal:

    I think that for most uses of a history it makes no sense to keep
    duplicates. I'd vote for making the default value be t and letting those few
    modes where duplicates might make sense (e.g. shell-mode?) bind it to nil
    unless the user has explicitly specified otherwise.

    IOW the option values could be:

     nil            - means never remove duplicates
     t (default)    - means remove duplicates, but this can be
                      overridden by a mode (e.g. shell-mode)
     non-nil, non-t - means always remove duplicates (never override)

    This would require code changes only for those few modes that want to
    override the default (t). Plus a change to the defcustom.


-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

RE: should search ring contain duplicates?

[Drew Adams]

    >         > I am not interested in considering such a complex proposal.
    >
    >         There is nothing complex about it (with the latest
    >         add-to-history) version.
    >
    >     I was talking about a different proposal, one involving
    >     properties to control lengths.  It was very complex.
    >
    > To be clear, was it this proposal from Juri:
    >
    >     A related feature that specifies the maximum length of
    >     the history list
    >     uses the `history-length' _property_ of the history list _symbol_
    >     to override the default value of the _variable_ `history-length'
    >     for a particular history list.  Exactly the same thing could be
    >     implemented for `history-delete-duplicates', i.e. the property
    >     `history-delete-duplicates' would override its default value
    >     for a particular history list.

    I don't know how you gather this

I don't gather anything. It was meant as a question: "Was it this proposal?"

    when Richard directly replied to the
    posting of yours ... containing the proposal:

        I think that for most uses of a history it makes no sense to keep
        duplicates. I'd vote for making the default value be t and
        letting those few modes where duplicates might make sense (e.g.
        shell-mode?) bind it to nil unless the user has explicitly
        specified otherwise.

        IOW the option values could be:

         nil            - means never remove duplicates
         t (default)    - means remove duplicates, but this can be
                          overridden by a mode (e.g. shell-mode)
         non-nil, non-t - means always remove duplicates (never override)

        This would require code changes only for those few modes
        that want to override the default (t). Plus a change to the
        defcustom.

Precisely why I asked RMS for clarification. You make me glad I did, because
I suspect that you, too, might be confused in trying to interpret Richard's
message.

My proposal said nothing about "properties to control lengths", and I don't
think it was very "complex". Juri's email was a reply to mine, and he did
propose something fairly complex about controlling history-list lengths
using symbol properties. Juri changed the subject to "a related feature" of
his choice without changing the Subject line. In spite of Juri's mention of
"a related feature", there is no real connection between the two proposals.

RMS's reply (sent to me, you are correct) was not clear (to me). Richard has
told me before that he pays little or no attention to the To: destination. I
suspect that he perhaps hit Reply to my email but wrote his response to
Juri's after reading it (!?). Otherwise, I don't understand his reply.
Clarification welcome.

I think things would be clearer if people used emacs-devel (only) in the To:
field and cc'd any additional destinations (it would also be good if
emacs-devel was the default To: value for a Reply).

This is, I suspect, a good example of the confusion that can arise from 1)
not paying attention to To: and 2) not explicitly referencing what you're
talking about. #2 is more important than #1: be clear what you are referring
to.

This has been a long thread with multiple proposals. Richard: please clarify
your position(s).

Re: should search ring contain duplicates?

[Richard Stallman]

	I think that for most uses of a history it makes no sense to keep
	duplicates. I'd vote for making the default value be t and letting those few
	modes where duplicates might make sense (e.g. shell-mode?) bind it to nil
	unless the user has explicitly specified otherwise.

	IOW the option values could be:

	 nil            - means never remove duplicates
	 t (default)    - means remove duplicates, but this can be
			  overridden by a mode (e.g. shell-mode)
	 non-nil, non-t - means always remove duplicates (never override)

When I responded to this one, I must have been thinking about this
one.  It appears that I forgot about this one afterward, and
remembered only the other complex proposal about properties.  I
apologize for the confusion.

Both this and the proposal about properties are too complex.

Even if these features were desirable in general, now would be the
wrong time for them.  We are close to being ready for pretest.
Please propose only things that MUST be done before the release.

Re: should search ring contain duplicates?

[Juri Linkov]

>         > I am not interested in considering such a complex proposal.
>
>         There is nothing complex about it (with the latest add-to-history)
>         version.
>
>     I was talking about a different proposal, one involving properties to
>     control lengths.  It was very complex.
>
> To be clear, was it this proposal from Juri:
>
>     A related feature that specifies the maximum length of the history list
>     uses the `history-length' _property_ of the history list _symbol_
>     to override the default value of the _variable_ `history-length'
>     for a particular history list.  Exactly the same thing could be
>     implemented for `history-delete-duplicates', i.e. the property
>     `history-delete-duplicates' would override its default value
>     for a particular history list.

The first part of this paragraph is not a proposal.  It describes
how `history-length' already uses the property of the history symbol.
The Lisp code of `add-to-history' Kim submitted to this list,
demonstrates very simple implementation of this functionality as:

    (or (get history-var 'history-length) history-length)

I proposed to do the same for history-delete-duplicates,
i.e. to use the following condition in `add-to-history':

    (or (get history-var 'history-delete-duplicates) history-delete-duplicates)

So my proposal is not very complex, and I don't think Richard replied to
my email.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: should search ring contain duplicates?

[Kim F. Storm]

Juri Linkov <juri@jurta.org> writes:

> I proposed to do the same for history-delete-duplicates,
> i.e. to use the following condition in `add-to-history':
>
>     (or (get history-var 'history-delete-duplicates) history-delete-duplicates)

But this will not allow you to inhibit delete duplicates on a specific history-var
if the history-delete-duplicates variable is set.

You would have to differentiate between having NO history-delete-duplicates
property and having a nil history-delete-duplicates property.  That's more
complex -- and it is not what the C-level code does.

It's not a bad idea, but after the release....

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

RE: should search ring contain duplicates?

[Drew Adams]

    >     A related feature that specifies the maximum length of
    >     the history list
    >     uses the `history-length' _property_ of the history list _symbol_
    >     to override the default value of the _variable_ `history-length'
    >     for a particular history list.  Exactly the same thing could be
    >     implemented for `history-delete-duplicates', i.e. the property
    >     `history-delete-duplicates' would override its default value
    >     for a particular history list.

    The first part of this paragraph is not a proposal.  It describes
    how `history-length' already uses the property of the history symbol.
    The Lisp code of `add-to-history' Kim submitted to this list,
    demonstrates very simple implementation of this functionality as:

        (or (get history-var 'history-length) history-length)

    I proposed to do the same for history-delete-duplicates,
    i.e. to use the following condition in `add-to-history':

        (or (get history-var 'history-delete-duplicates)
            history-delete-duplicates)

    So my proposal is not very complex, and I don't think Richard replied to
    my email.

Well, as I said, I do not understand what Richard was referring to - that's
why I asked. He mentioned a proposal "involving properties to control
lengths. It was very complex."

Your proposal involves properties to control lengths. Mine doesn't have
anything to do with that, and I don't see anything else in the thread about
that. I can't speak to the complexity of your proposal, but mine seems
trivial to implement, understand, use, and document.

I guess we'll just have to wait until Richard decides to enlightens us as to
what he meant.

Re: should search ring contain duplicates?

[Dan Nicolaescu]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

  > > `isearch-update-ring' could at least do that when
  > > history-delete-duplicates is t.
  > 
  > BTW, is there any reason why history-delete-duplicates defaults to nil?

See the thread at http://lists.gnu.org/archive/html/emacs-devel/2004-09/msg00038.html

Re: should search ring contain duplicates?

[Richard Stallman]

    BTW, is there any reason why history-delete-duplicates defaults to nil?

Let's not consider changing it now.

Re: should search ring contain duplicates?

[Richard Stallman]

    `isearch-update-ring' could at least do that when
    history-delete-duplicates is t.

You wrote the same patch I did.  Would you please install it?

Re: should search ring contain duplicates?

[Dan Nicolaescu]

Richard Stallman <rms@gnu.org> writes:

  >     `isearch-update-ring' could at least do that when
  >     history-delete-duplicates is t.
  > 
  > You wrote the same patch I did.  Would you please install it?

I did. I will update the code to use the proposed history-push macro
after it appears in CVS.

Re: should search ring contain duplicates?

[Richard Stallman]

Thanks for fixing this.