Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change 0f3c1db: Changed semantics of first-undoable-change-hook.

[Stefan Monnier]

> +(defun undo-needs-boundary-p ()
> +  "Returns t if `buffer-undo-list' needs a boundary at the start."
             ^^^
           non-nil

> +   ;; buffer-undo-list can be t
> +   (listp buffer-undo-list)
> +   ;; first element of buffer-undo-list is nil

Please capitalize and punctuate your comments.

> +run_first_undo_hook()

Please always add spaces before the open parens.

> +  list = BVAR(current_buffer, undo_list);

Here as well (and ,many other places).

> +  if (CONSP (list) && NILP (XCAR (list)))
> +    {
> +      safe_run_hooks(Qundo_first_undoable_change_hook);
> +    }

I think this fails to run the hook when list is nil.
Also, I don't think we need the `safe' version of run-hooks here because
we shouldn't be in a special context like redisplay where signals would
have nasty consequences.

 
> +
>  DEFUN ("undo-boundary", Fundo_boundary, Sundo_boundary, 0, 0, 0,

Try to avoid adding spurious empty lines.

It looks pretty good.  Remaining problems I noticed:

- run_first_undo_hook is defined to take no argument, yet it's always
  called with `current_buffer' as argument.

- in src/keyboard.c we push undo-boundaries after every command.
  This is now made redundant by your code, so you might like to
  remove it.

- when trying to remove that code, you'll notice that
  Fself_insert_command (and Fdelete_char, IIRC) depends on that code to
  know whether the last boundary can be removed (so as to implement the
  "bundling").

- It looks like run_first_undo_hook is often called in an "unclean"
  state where the C code has set current_buffer rather than calling
  set_buffer_internal.  This is an optimization that only works when we
  know exactly the code that might possibly be run before current_buffer
  is set back to its previous value, but it can't be used when running
  arbitrary Elisp code.  So we should call run_first_undo_hook *before*
  setting current_buffer, and run_first_undo_hook should
  set_buffer_internal when needed.


        Stefan