Re: should search ring contain duplicates?

[storm@cua.dk (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