Summarizing the purpose of a change.

[Karl Fogel]

I'm using the change below as an example for a more general question:

In many projects, there is a convention of summarizing the purpose of a
change in one or two sentences at the start of the log entry.  This makes
the rest of the log entry (and the change itself) easier to understand.

Could we try that in Emacs?  Or is there some reason we don't do it?

For example, for the change below, even after reading the entire log
entry it's hard to figure out what the committer's overall intention
was.  Is it just a bunch of random improvements that happen to be sent
in one commit?  Or is it a unified change group all made toward one
overall purpose?  I'm not sure.  I guess I could figure it out if I
looked at the diff, but the committer already knows the answer...

(Nothing about this change looks wrong, I hasten to add, and it's great
that Stefan is cleaning stuff up.  It would just a lot easier to
evaluate the change, and follow what's going on in general, if one knew
the intentions.  In this case, even just saying "Random code cleanups
and minor bug fixes" would help set the right mood for the reviewer.)

-Karl

Stefan Monnier <address@hidden> writes:
> CVSROOT:      /sources/emacs
> Module name:  emacs
> Changes by:   Stefan Monnier <monnier>        09/11/21 04:43:15
>
> Modified files:
>       lisp           : ChangeLog bookmark.el 
>
> Log message:
>       (bookmark-search-prompt, bookmark-search-timer): Remove.
>       (bookmark-search-pattern): Move and leave unbound.
>       (bookmark-bmenu-mode-map): Change binding.
>       (bookmark-read-search-input): Simplify.
>       Don't use text-char-description.  Don't error on non-char events.
>       (bookmark-filtered-alist-by-regexp-only): Remove by folding into the
>       only caller (i.e. bookmark-bmenu-filter-alist-by-regexp).
>       (bookmark-bmenu-search): Don't check we're in a bookmark-list buffer.
>       Use a local var for the timer.
>       (bookmark-bmenu-cancel-search): Remove by folding into the only caller
>       (i.e. bookmark-bmenu-search).
>
> CVSWeb URLs:
> http://cvs.savannah.gnu.org/viewcvs/emacs/lisp/ChangeLog?cvsroot=emacs&r1=1.16693&r2=1.16694
> http://cvs.savannah.gnu.org/viewcvs/emacs/lisp/bookmark.el?cvsroot=emacs&r1=1.143&r2=1.144
>
> Patches:
> Index: ChangeLog
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/ChangeLog,v
> retrieving revision 1.16693
> retrieving revision 1.16694
> diff -u -b -r1.16693 -r1.16694
> --- ChangeLog 21 Nov 2009 02:36:54 -0000      1.16693
> +++ ChangeLog 21 Nov 2009 04:43:10 -0000      1.16694
> @@ -1,11 +1,25 @@
> +2009-11-21  Stefan Monnier  <address@hidden>
> +
> +     * bookmark.el (bookmark-search-prompt, bookmark-search-timer): Remove.
> +     (bookmark-search-pattern): Move and leave unbound.
> +     (bookmark-bmenu-mode-map): Change binding.
> +     (bookmark-read-search-input): Simplify.
> +     Don't use text-char-description.  Don't error on non-char events.
> +     (bookmark-filtered-alist-by-regexp-only): Remove by folding into the
> +     only caller (i.e. bookmark-bmenu-filter-alist-by-regexp).
> +     (bookmark-bmenu-search): Don't check we're in a bookmark-list buffer.
> +     Use a local var for the timer.
> +     (bookmark-bmenu-cancel-search): Remove by folding into the only caller
> +     (i.e. bookmark-bmenu-search).
> +
>  2009-11-21  Glenn Morris  <address@hidden>
>  
>       * mail/rmailmm.el (rmail-mime): Decode in fundamental-mode.  (Bug#4993)
>  
>  2009-11-20  Ken Brown  <address@hidden>  (tiny change)
>  
> -     * net/browse-url.el (browse-url-default-windows-browser): Use
> -     cygstart for cygwin.
> +     * net/browse-url.el (browse-url-default-windows-browser):
> +     Use cygstart for cygwin.
>  
>  2009-11-20  Karl Fogel  <address@hidden>
>  
> @@ -23,16 +37,16 @@
>       * subword.el (subword-forward, subword-backward, subword-mark)
>       (subword-kill, subword-backward-kill, subword-transpose)
>       (subword-downcase, subword-upcase, subword-capitalize)
> -     (subword-forward-internal, subword-backward-internal): Renamed
> -     from forward-subword, backward-subword, mark-subword kill-subword,
> -     backward-kill-subword, transpose-subwords, downcase-subword,
> -     upcase-subword, capitalize-subword forward-subword-internal,
> -     backward-subword-internal.
> +     (subword-forward-internal, subword-backward-internal):
> +     Rename from forward-subword, backward-subword, mark-subword,
> +     kill-subword, backward-kill-subword, transpose-subwords,
> +     downcase-subword, upcase-subword, capitalize-subword,
> +     forward-subword-internal, backward-subword-internal.
>  
>  2009-11-20  Thierry Volpiatto  <address@hidden>
>  
> -     * bookmark.el (bookmark-search-delay, bookmark-search-prompt): New
> -     options.
> +     * bookmark.el (bookmark-search-delay, bookmark-search-prompt):
> +     New options.
>       (bookmark-search-pattern, bookmark-search-timer, bookmark-quit-flag):
>       New vars.
>       (bookmark-read-search-input, bookmark-filtered-alist-by-regexp-only)
>
> Index: bookmark.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/bookmark.el,v
> retrieving revision 1.143
> retrieving revision 1.144
> diff -u -b -r1.143 -r1.144
> --- bookmark.el       20 Nov 2009 21:12:54 -0000      1.143
> +++ bookmark.el       21 Nov 2009 04:43:15 -0000      1.144
> @@ -196,19 +196,12 @@
>    :type 'integer
>    :group 'bookmark)
>  
> -
> +;; FIXME: Is it really worth a customization option?
>  (defcustom bookmark-search-delay 0.2
> -  "*When searching bookmarks, redisplay every `bookmark-search-delay' 
> seconds."
> +  "Time before `bookmark-bmenu-search' updates the display."
>    :group 'bookmark
>    :type  'integer)
>  
> -
> -(defcustom bookmark-search-prompt "Pattern: "
> -  "*Prompt used for `bookmark-bmenu-search'."
> -  :group 'bookmark
> -  :type  'string)
> -
> -
>  (defface bookmark-menu-heading
>    '((t (:inherit font-lock-type-face)))
>    "Face used to highlight the heading in bookmark menu buffers."
> @@ -332,19 +325,9 @@
>  This point is in `bookmark-curent-buffer'.")
>  
>  
> -(defvar bookmark-search-pattern ""
> -  "Store keyboard input for incremental search.")
> -
> -
> -(defvar bookmark-search-timer nil
> -  "Timer used for searching")
> -
> -
>  (defvar bookmark-quit-flag nil
>    "Non nil make `bookmark-bmenu-search' quit immediately.")
>  
> -
> -
>  ;; Helper functions.
>  
>  ;; Only functions on this page and the next one (file formats) need to
> @@ -1549,7 +1532,9 @@
>      (define-key map "a" 'bookmark-bmenu-show-annotation)
>      (define-key map "A" 'bookmark-bmenu-show-all-annotations)
>      (define-key map "e" 'bookmark-bmenu-edit-annotation)
> -    (define-key map "\M-g" 'bookmark-bmenu-search)
> +    ;; The original binding of M-g hides the M-g prefix map.
> +    ;; If someone has a better idea than M-g s, I'm open to suggestions.
> +    (define-key map [?\M-g ?s] 'bookmark-bmenu-search)
>      (define-key map [mouse-2] 'bookmark-bmenu-other-window-with-mouse)
>      map))
>  
> @@ -2099,69 +2084,58 @@
>  
>  ;;; Bookmark-bmenu search
>  
> +;; Store keyboard input for incremental search.
> +(defvar bookmark-search-pattern)
> +
>  (defun bookmark-read-search-input ()
>    "Read each keyboard input and add it to `bookmark-search-pattern'."
> -  (setq bookmark-search-pattern "")    ; Always reset pattern to empty string
> -  (let ((prompt       (propertize bookmark-search-prompt
> -                                  'face '((:foreground "cyan"))))
> -        (inhibit-quit t)
> -        (tmp-list     ())
> -        char)
> -    (catch 'break
> -      (while 1
> -        (catch 'continue
> -          (condition-case nil
> -              (setq char (read-char (concat prompt bookmark-search-pattern)))
> -            (error (throw 'break nil)))
> +  (let ((prompt       (propertize "Pattern: " 'face 'minibuffer-prompt))
> +        ;; (inhibit-quit t) ; inhibit-quit is evil.  Use it with extreme 
> care!
> +        (tmp-list     ()))
> +    (while
> +        (let ((char (read-key (concat prompt bookmark-search-pattern))))
>            (case char
> -            ((or ?\e ?\r) ; RET or ESC break search loop and lead to [1].
> -             (throw 'break nil)) 
> -            (?\d (pop tmp-list) ; Delete last char of 
> `bookmark-search-pattern'
> -                 (setq bookmark-search-pattern
> -                       (mapconcat 'identity (reverse tmp-list) ""))
> -                 (throw 'continue nil))
> -            (?\C-g (setq bookmark-quit-flag t) (throw 'break nil))
> +            ((?\e ?\r) nil) ; RET or ESC break the search loop.
> +            (?\C-g (setq bookmark-quit-flag t) nil)
> +            (?\d (pop tmp-list) t) ; Delete last char of pattern with DEL
>              (t
> -             (push (text-char-description char) tmp-list)
> +             (if (characterp char)
> +                 (push char tmp-list)
> +               (setq unread-command-events
> +                     (nconc (mapcar 'identity
> +                                    (this-single-command-raw-keys))
> +                            unread-command-events))
> +               nil))))
>               (setq bookmark-search-pattern
> -                   (mapconcat 'identity (reverse tmp-list) ""))
> -             (throw 'continue nil))))))))
> -
> -
> -(defun bookmark-filtered-alist-by-regexp-only (regexp)
> -  "Return a filtered `bookmark-alist' with bookmarks matching REGEXP."
> -  (loop for i in bookmark-alist
> -     when (string-match regexp (car i)) collect i into new
> -     finally return new))
> +            (apply 'string (reverse tmp-list))))))
>  
>  
>  (defun bookmark-bmenu-filter-alist-by-regexp (regexp)
>    "Filter `bookmark-alist' with bookmarks matching REGEXP and rebuild list."
> -  (let ((bookmark-alist (bookmark-filtered-alist-by-regexp-only regexp)))
> +  (let ((bookmark-alist
> +         (loop for i in bookmark-alist
> +               when (string-match regexp (car i)) collect i into new
> +               finally return new)))
>      (bookmark-bmenu-list)))
>  
> +
>  ;;;###autoload
>  (defun bookmark-bmenu-search ()
> -  "Incrementally search bookmarks matching `bookmark-search-pattern'.
> -`bookmark-search-pattern' is built incrementally with
> -`bookmark-read-search-input'."
> +  "Incremental search of bookmarks, hiding the non-matches as we go."
>    (interactive)
> -  (when (string= (buffer-name (current-buffer)) "*Bookmark List*")
> -    (let ((bmk (bookmark-bmenu-bookmark)))
> -      (unwind-protect
> -           (progn
> -             (setq bookmark-search-timer
> -                   (run-with-idle-timer
> +  (let ((bmk (bookmark-bmenu-bookmark))
> +        (bookmark-search-pattern "")
> +        (timer (run-with-idle-timer
>                      bookmark-search-delay 'repeat
>                      #'(lambda ()
>                          (bookmark-bmenu-filter-alist-by-regexp
> -                         bookmark-search-pattern))))
> -             (bookmark-read-search-input))
> -        (progn ; [1] Stop timer.
> -          (bookmark-bmenu-cancel-search)
> +                     bookmark-search-pattern)))))
> +    (unwind-protect
> +        (bookmark-read-search-input)
> +      (cancel-timer timer)
>            (when bookmark-quit-flag ; C-g hit restore menu list.
>              (bookmark-bmenu-list) (bookmark-bmenu-goto-bookmark bmk))
> -          (setq bookmark-quit-flag nil))))))
> +      (setq bookmark-quit-flag nil))))
>        
>  (defun bookmark-bmenu-goto-bookmark (name)
>    "Move point to bookmark with name NAME."
> @@ -2172,11 +2146,6 @@
>    (forward-line 0))
>            
>  
> -(defun bookmark-bmenu-cancel-search ()
> -  "Cancel timer used for searching in bookmarks."
> -  (cancel-timer bookmark-search-timer)
> -  (setq bookmark-search-timer nil))
> -
>  
>  ;;; Menu bar stuff.  Prefix is "bookmark-menu".
>  
>
>
> _______________________________________________
> Emacs-diffs mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/emacs-diffs