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?

[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?

[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?

[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?

[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?

[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?

[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]

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?

[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?

[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]

Thanks for fixing this.

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

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]

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

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?

[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?

[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?

[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 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?

[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?

[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]

	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.

Could you show me what I said, back then, about a command history?  I
don't remember it, and it is not easy for me to look through a thread.

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?

[Drew Adams]

    	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.
    
    Could you show me what I said, back then, about a command history?  I
    don't remember it, and it is not easy for me to look through a thread.

Clicking the link shows this:

 There was one question left, if removing duplicates from history
 should be turned on by default. 
 IMHO duplicates should be removed from history by default, most users
 are more interested in the unique data present in history. If an exact
 sequence of events is desired M-x list-command-history can do that.

 [I reformated the last message from Stephan in that thread  to make it
 clear who said what]:

 Dan> How about the default? Should this be turned on by default? 
 Juri>     It seems reasonable to turn it on by default.

 RMS> Absolutely not!  It would be a great surprise, and it would mean
 RMS> you could not look around at your error messages without changing
 RMS> other buffers on the screen.

 Stephan> Huh?  I don't understand why removing duplicates from the history
 Stephan> would ever have any such effect.
 Stephan> Could you explain what use case you're thinking of ?

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