Summarizing the purpose of a change.

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 <monnier@iro.umontreal.ca> 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  <monnier@iro.umontreal.ca>
> +
> +	* 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  <rgm@gnu.org>
>  
>  	* mail/rmailmm.el (rmail-mime): Decode in fundamental-mode.  (Bug#4993)
>  
>  2009-11-20  Ken Brown  <kbrown@cornell.edu>  (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  <karl.fogel@red-bean.com>
>  
> @@ -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  <thierry.volpiatto@gmail.com>
>  
> -	* 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.")
>  
> -
> -\f
>  ;; 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))
> -
>  \f
>  ;;; Menu bar stuff.  Prefix is "bookmark-menu".
>  
>
>
> _______________________________________________
> Emacs-diffs mailing list
> Emacs-diffs@gnu.org
> http://lists.gnu.org/mailman/listinfo/emacs-diffs

Re: Summarizing the purpose of a change.

[Miles Bader]

Karl Fogel <kfogel@red-bean.com> writes:
> 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.

And if possible, make the summary not "1 or 2" sentences, but a single
not-to-long line, such as you'd use for an email Subject: header.

-Miles

-- 
The automobile has not merely taken over the street, it has dissolved the
living tissue of the city.  Its appetite for space is absolutely insatiable;
moving and parked, it devours urban land, leaving the buildings as mere
islands of habitable space in a sea of dangerous and ugly traffic.
[James Marston Fitch, New York Times, 1 May 1960]

Re: Summarizing the purpose of a change.

[Eli Zaretskii]

> From: Miles Bader <miles@gnu.org>
> Date: Sat, 21 Nov 2009 17:07:29 +0900
> Cc: emacs-devel@gnu.org
> 
> Karl Fogel <kfogel@red-bean.com> writes:
> > 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.
> 
> And if possible, make the summary not "1 or 2" sentences, but a single
> not-to-long line, such as you'd use for an email Subject: header.

I suspect pointing to the relevant discussion on emacs-devel, or to a
bug number (if there is any) would be a lesser burden.

Re: Summarizing the purpose of a change.

[Giorgos Keramidas]

On Sat, 21 Nov 2009 11:11:15 +0200, Eli Zaretskii <eliz@gnu.org> wrote:
>Karl Fogel <kfogel@red-bean.com> writes:
>>> 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.
>>
>> And if possible, make the summary not "1 or 2" sentences, but a single
>> not-to-long line, such as you'd use for an email Subject: header.
>
> I suspect pointing to the relevant discussion on emacs-devel, or to a
> bug number (if there is any) would be a lesser burden.

Yes, when there *is* a bug number this is useful.

A short summary in the first line of each commit is often useful for
other changes too; changes that are not necessarily bug fixes.

For example, Bazaar has a "--line" option that can display long lists of
changesets by showing a single-line summary for each commit.  In a small
sample repository with the following history:

: keramida@kobe:/tmp/koko$ bzr log
: ------------------------------------------------------------
: revno: 3
: committer: Giorgos Keramidas <keramida@kobe>
: branch nick: koko
: timestamp: Sat 2009-11-21 13:19:31 +0200
: message:
:   bug #27 -- add a '.localnet' suffix to the default 'foo' name
:
:   The installation scripts can set the default domain name to a more
:   sensible value, but there are bits of the installer code that may
:   have to read 'foo' before DNS, NIS or some other resolver scheme
:   is up and running.  So add a default '.localnet' domain part to
:   the name of 'foo', and let the resolver code replace it with the
:   real domain name later.
: ------------------------------------------------------------
: revno: 2
: committer: Giorgos Keramidas <keramida@kobe>
: branch nick: koko
: timestamp: Sat 2009-11-21 13:18:24 +0200
: message:
:   bug #19 -- 'foo' always displays a short name
:
:   We need the full name of the 'foo' utility here.  Shortening the name
:   to something less chatty can be done by the GUI utilities.
: ------------------------------------------------------------
: revno: 1
: committer: Giorgos Keramidas <keramida@kobe>
: branch nick: koko
: timestamp: Sat 2009-11-21 13:18:07 +0200
: message:
:   Add foo
: keramida@kobe:/tmp/koko$

the bzr(1) utility can show a short summary of all the changes in just 3
lines of output:

: keramida@kobe:/tmp/koko$ bzr log --line
: 3: Giorgos Keramidas 2009-11-21 bug #27 -- add a '.localnet' suffix to the default 'foo' name
: 2: Giorgos Keramidas 2009-11-21 bug #19 -- 'foo' always displays a short name
: 1: Giorgos Keramidas 2009-11-21 Add foo
: keramida@kobe:/tmp/koko$

The format of the commits logs I used in the sample repository make the
short log summary more useful because their first line includes enough
context about the intent of each change.

Re: Summarizing the purpose of a change.

[Alan Mackenzie]

Hi, Karl,

On Sat, Nov 21, 2009 at 01:09:19AM -0600, Karl Fogel wrote:
> 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?

I do this every time.  So far, nobody's complained.

> 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.)

Agreed.

> -Karl

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Summarizing the purpose of a change.

[Eli Zaretskii]

> Date: Sat, 21 Nov 2009 12:19:50 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: emacs-devel@gnu.org
> 
> > 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?
> 
> I do this every time.  So far, nobody's complained.

I do this when I commit large sets of changes.  It could be a nuisance
to do that for simple changes in a single file, though.

Re: Summarizing the purpose of a change.

[Karl Fogel]

Eli Zaretskii <eliz@gnu.org> writes:
>> > 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?
>> 
>> I do this every time.  So far, nobody's complained.
>
> I do this when I commit large sets of changes.  It could be a nuisance
> to do that for simple changes in a single file, though.

Yup.  I'm talking about just the situation where a summary is necessary.
For an easy fix in a single file, the technical description of the
change can also be the summary -- no one needs more.  Only when the
change is not its own summary is a summary is needed.

So far, everyone who's chimed in has basically said "Yeah, I try to do
this".  But my question is more about making it policy, in the sense
that of making it appropriate to follow up to change saying "How about a
summary line for that?"

-K

Re: Summarizing the purpose of a change.

[Stefan Monnier]

> So far, everyone who's chimed in has basically said "Yeah, I try to do
> this".  But my question is more about making it policy, in the sense
> that of making it appropriate to follow up to change saying "How about a
> summary line for that?"

We could try it.  I think adding such a summary line can be
useful, indeed.  We could try to make it more of a convention, indeed.
As somebody else pointed out, many VCS have special support for such
"summary line".
And after all, we already do the same for docstrings.
Let's try it for a while,


        Stefan

Re: Summarizing the purpose of a change.

[Karl Fogel]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> So far, everyone who's chimed in has basically said "Yeah, I try to do
>> this".  But my question is more about making it policy, in the sense
>> that of making it appropriate to follow up to change saying "How about a
>> summary line for that?"
>
> We could try it.  I think adding such a summary line can be
> useful, indeed.  We could try to make it more of a convention, indeed.
> As somebody else pointed out, many VCS have special support for such
> "summary line".
> And after all, we already do the same for docstrings.
> Let's try it for a while,

Great.  I guess the way to do that is to follow up when we see it not
done and suggest it; if we're all happy with the results, we can
document the convention (where is the right place for that?)

-K

Re: Summarizing the purpose of a change.

[Thien-Thi Nguyen]

() Karl Fogel <kfogel@red-bean.com>
() Sun, 22 Nov 2009 20:20:25 -0600

   if we're all happy with the results, we can
   document the convention (where is the right place for that?)

Probably in file admin/notes/changelogs is best.

thi

Re: Summarizing the purpose of a change.

[Alfred M. Szmidt]

      if we're all happy with the results, we can document the
      convention (where is the right place for that?)

   Probably in file admin/notes/changelogs is best.

A better place would be to add this to the GNU Coding Standards.

Re: Summarizing the purpose of a change.

[Richard Stallman]

I can consider a change in the coding standards if people write a proposal.

Re: Summarizing the purpose of a change.

[Thien-Thi Nguyen]

Is the standards.texi in gnulib the canonical source?
If not, what is the canonical source (for purposes of
submitting a proposal in the form of a diff)?

thi

Re: Summarizing the purpose of a change.

[Richard Stallman]

    Is the standards.texi in gnulib the canonical source?

Yes.