Re: [PATCH 2/2] git-blame.el: pick a set of random colors when blaming

["David Kågedal" <davidk@lysator.liu.se>]

Xavier Maillard <zedek@gnu.org> writes:

> I thought it would be cooler to have different set of colors each time
> I blame.

But the code for it looks weird:

> @@ -302,9 +320,8 @@ See also function `git-blame-mode'."
>            (inhibit-point-motion-hooks t)
>            (inhibit-modification-hooks t))
>        (when (not info)
> -        (let ((color (pop git-blame-colors)))
> -          (unless color
> -            (setq color git-blame-ancient-color))
> +        (let ((color (or (elt git-blame-colors (random (length git-blame-colors)))
> +			 git-blame-ancient-color)))
>            (setq info (list hash src-line res-line num-lines
>                             (git-describe-commit hash)
>                             (cons 'color color))))

Instead of using the colors one at a time, you randomly select one of
them. This means that you might select the same color twice or more,
and even twice in a row.  And you will never run out of colors, so
git-blame-ancient-color will never be used.

This change should probably not go in, but your patch has other stuff
that's good.

> * Prevent (future possible) namespace clash by renaming `color-scale'
> into `git-blame-color-scale'. Definition has been changed to be more
> in the "lisp" way (thanks for help goes to #emacs). Also added a small
> description of what it does.

Ok, but the heavier cl dependency is noted below.

> * Added docstrings at some point and instructed defvar when a variable
> was candidate to customisation by users.

Good.

> * Added fix to silent byte-compilers (git-blame-file,
> git-blame-current)

Good.

> * Do not require 'cl at startup.

You removed the pop calls, but added a couple of dolist calls.  So you
still need to require cl.

> * Added more informations on compatibility

Good.

> Signed-off-by: Xavier Maillard <zedek@gnu.org>
> ---
>  contrib/emacs/git-blame.el |   71 +++++++++++++++++++++++++++----------------
>  1 files changed, 44 insertions(+), 27 deletions(-)
>
> diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
> index bd87a86..6d0c1b0 100644
> --- a/contrib/emacs/git-blame.el
> +++ b/contrib/emacs/git-blame.el
> @@ -8,8 +8,8 @@
>  ;; License:    GPL
>  ;; Keywords:   git, version control, release management
>  ;;
> -;; Compatibility: Emacs21
> -
> +;; Compatibility: Emacs21, Emacs22 and EmacsCVS
> +;;                Git 1.5 and up
>  
>  ;; This file is *NOT* part of GNU Emacs.
>  ;; This file is distributed under the same terms as GNU Emacs.
> @@ -61,8 +61,9 @@
>  
>  ;;; Compatibility:
>  ;;
> -;; It requires GNU Emacs 21.  If you'are using Emacs 20, try
> -;; changing this:
> +;; It requires GNU Emacs 21 or later and Git 1.5.0 and up
> +;; 
> +;; If you'are using Emacs 20, try changing this:
>  ;;
>  ;;            (overlay-put ovl 'face (list :background
>  ;;                                         (cdr (assq 'color (cddddr info)))))
> @@ -77,30 +78,43 @@
>  ;;
>  ;;; Code:
>  
> -(require 'cl)			      ; to use `push', `pop'
> -
> -(defun color-scale (l)
> -  (let* ((colors ())
> -         r g b)
> -    (setq r l)
> -    (while r
> -      (setq g l)
> -      (while g
> -        (setq b l)
> -        (while b
> -          (push (concat "#" (car r) (car g) (car b)) colors)
> -          (pop b))
> -        (pop g))
> -      (pop r))
> -    colors))
> +(eval-when-compile (require 'cl))			      ; to use `push', `pop'
> +
> +
> +(defun git-blame-color-scale (&rest elements)
> +  "Given a list, returns a list of triples formed with each
> +elements of the list.
> +
> +a b => bbb bba bab baa abb aba aaa aab"
> +  (let (result)
> +    (dolist (a elements)
> +      (dolist (b elements)
> +        (dolist (c elements)
> +          (setq result (cons (format "#%s%s%s" a b c) result)))))
> +    result))
> +
> +;; (git-blame-color-scale "0c" "04" "24" "1c" "2c" "34" "14" "3c") =>
> +;; ("#3c3c3c" "#3c3c14" "#3c3c34" "#3c3c2c" "#3c3c1c" "#3c3c24"
> +;; "#3c3c04" "#3c3c0c" "#3c143c" "#3c1414" "#3c1434" "#3c142c" ...)
>  
>  (defvar git-blame-dark-colors
> -  (color-scale '("0c" "04" "24" "1c" "2c" "34" "14" "3c")))
> +  (git-blame-color-scale "0c" "04" "24" "1c" "2c" "34" "14" "3c")
> +  "*List of colors (format #RGB) to use in a dark environment.
> +
> +To check out the list, evaluate (list-colors-display git-blame-dark-colors).")
>  
>  (defvar git-blame-light-colors
> -  (color-scale '("c4" "d4" "cc" "dc" "f4" "e4" "fc" "ec")))
> +  (git-blame-color-scale "c4" "d4" "cc" "dc" "f4" "e4" "fc" "ec")
> +  "*List of colors (format #RGB) to use in a light environment.
> +
> +To check out the list, evaluate (list-colors-display git-blame-light-colors).")
>  
> -(defvar git-blame-ancient-color "dark green")
> +(defvar git-blame-colors '()
> +  "Colors used by git-blame. The list is built once when activating git-blame
> +minor mode.")
> +  
> +(defvar git-blame-ancient-color "dark green"
> +  "*Color to be used for ancient commit.")
>  
>  (defvar git-blame-autoupdate t
>    "*Automatically update the blame display while editing")
> @@ -125,6 +139,10 @@
>    "A queue of update requests")
>  (make-variable-buffer-local 'git-blame-update-queue)
>  
> +;; FIXME: docstrings
> +(defvar git-blame-file nil)
> +(defvar git-blame-current nil)
> +
>  (defvar git-blame-mode nil)
>  (make-variable-buffer-local 'git-blame-mode)
>  
> @@ -177,7 +195,7 @@ See also function `git-blame-mode'."
>    "Recalculate all blame information in the current buffer"
>    (interactive)
>    (unless git-blame-mode
> -    (error "git-blame is not active"))
> +    (error "Git-blame is not active"))
>    
>    (git-blame-cleanup)
>    (git-blame-run))

-- 
David Kågedal