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

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

[Xavier Maillard]

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

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

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

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

* Do not require 'cl at startup.

* Added more informations on compatibility

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))
@@ -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))))
-- 
1.5.0.5

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

[David Kågedal]

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

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

[Xavier Maillard]

Hi,

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

   But the code for it looks weird:

Why ? It looks good to me except the "small" quircks :)

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

I partly agree with you.

Random is not enough and we need to delete the color we just
set. This is what I am currently doing in the next patch. There
is still an interrogation: what is the problem if we never fail
and thus, never use git-blame-ancient-color ?

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

I kept cl but I surrounded it into an eval-when-compile form as
requested by elisp standards.

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

Yep. See below.

Thank you for your review !

Xavier

[PATCH] git-blame.el: pick a set of random colors for each git-blame turn

[Xavier Maillard]

I thought it would be cool to have different set of colors for each
git-blame-mode. Function `git-blame-new-commit' does this for us
picking when possible, a random colors based on the set we build on
startup. When it fails, `git-blame-ancient-color' will be used. We
also take care not to use the same color more than once (thank you
David Kågedal).

* 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 to #emacs). Also added a small
description of what it does.

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

* Added missing defvar to silent byte-compilers (git-blame-file,
git-blame-current)

* Do not require 'cl at startup

* Added more informations on compatibility

Signed-off-by: Xavier Maillard <zedek@gnu.org>
---
 contrib/emacs/git-blame.el |   82 ++++++++++++++++++++++++++++----------------
 1 files changed, 52 insertions(+), 30 deletions(-)

diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index bd87a86..4703442 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))
@@ -294,18 +312,22 @@ See also function `git-blame-mode'."
         (t
          nil)))
 
-
 (defun git-blame-new-commit (hash src-line res-line num-lines)
   (save-excursion
     (set-buffer git-blame-file)
     (let ((info (gethash hash git-blame-cache))
           (inhibit-point-motion-hooks t)
-          (inhibit-modification-hooks t))
+          (inhibit-modification-hooks t)
+	  (colors git-blame-colors))
       (when (not info)
-        (let ((color (pop git-blame-colors)))
-          (unless color
-            (setq color git-blame-ancient-color))
-          (setq info (list hash src-line res-line num-lines
+	;; Assign a random color to each new commit info
+	;; Take care not to select the same color multiple times
+	(let* ((idx (random (length colors)))
+	       (color (or (elt colors idx)
+			  git-blame-ancient-color)))
+	  (and (assoc color colors)
+	       (setq colors (delete idx colors)))
+	  (setq info (list hash src-line res-line num-lines
                            (git-describe-commit hash)
                            (cons 'color color))))
         (puthash hash info git-blame-cache))
-- 
1.5.0.5

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

[David Kågedal]

Xavier Maillard <zedek@gnu.org> writes:

> Hi,
>
>    > I thought it would be cooler to have different set of colors each time
>    > I blame.
>
>    But the code for it looks weird:
>
> Why ? It looks good to me except the "small" quircks :)

I meant that it's doing strange things, like
 (or always-true-expression git-blame-ancient-color)

> I kept cl but I surrounded it into an eval-when-compile form as
> requested by elisp standards.

You are probably right.

-- 
David Kågedal

Re: [PATCH] git-blame.el: pick a set of random colors for each git-blame turn

[David Kågedal]

Xavier Maillard <zedek@gnu.org> writes:

> @@ -294,18 +312,22 @@ See also function `git-blame-mode'."
>          (t
>           nil)))
>  
> -
>  (defun git-blame-new-commit (hash src-line res-line num-lines)
>    (save-excursion
>      (set-buffer git-blame-file)
>      (let ((info (gethash hash git-blame-cache))
>            (inhibit-point-motion-hooks t)
> -          (inhibit-modification-hooks t))
> +          (inhibit-modification-hooks t)
> +	  (colors git-blame-colors))
>        (when (not info)
> -        (let ((color (pop git-blame-colors)))
> -          (unless color
> -            (setq color git-blame-ancient-color))
> -          (setq info (list hash src-line res-line num-lines
> +	;; Assign a random color to each new commit info
> +	;; Take care not to select the same color multiple times
> +	(let* ((idx (random (length colors)))
> +	       (color (or (elt colors idx)
> +			  git-blame-ancient-color)))
> +	  (and (assoc color colors)
> +	       (setq colors (delete idx colors)))
> +	  (setq info (list hash src-line res-line num-lines
>                             (git-describe-commit hash)
>                             (cons 'color color))))
>          (puthash hash info git-blame-cache))

I have a few questions here.  Why do you make a local reference
(color) to git-blame-colors, but you are still destructively updating
the list (using delete), possibly making git-blame-colors point to a
partial ruin of the original list?  My original version may look
similar, but pop is only destructive on the variable it is popping
from.  Any other references to the original list will be intact.

Remember that git-blame-colors is a buffer-local variable, but if it
points to a global list, any destructive changes will mess up the
global list.

Then it's this part

> +	(let* ((idx (random (length colors)))
> +	       (color (or (elt colors idx)
> +			  git-blame-ancient-color)))

If you have already consumed all colors, (length colors) will be zero
and random will return an arbitrary integer. And then you will do (elt
'() -47100) and check if that was nil.  It should work, but only by
luck.

I'd prefer something like this:

    (let ((color (if colors
                   (elt colors (random (length colors)))
                  git-blame-ancient-color)))

Then you have to remove it, and your (assoc color colors) looks
"weird", since assoc compares the car of each list element in colors,
but colors doesn't contain any pairs, so I don't really see how it
would ever return something.

You could break this out to a function:

(defmacro random-pop (l)
  "Remove a random element from l and update l"
  ;; only works on lists with unique elements
  `(let ((e (elt ,l (random (length ,l)))))
     (setq ,l (remove e ,l))
     e))

and use it like this:

    (let ((color (if colors
                   (random-pop colors)
                  git-blame-ancient-color)))

-- 
David Kågedal

Re: [PATCH] git-blame.el: pick a set of random colors for each git-blame turn

[Xavier Maillard]

Hi David,

   I have a few questions here.  Why do you make a local reference
   (color) to git-blame-colors, but you are still destructively updating
   the list (using delete), possibly making git-blame-colors point to a
   partial ruin of the original list?  My original version may look
   similar, but pop is only destructive on the variable it is popping
   from.  Any other references to the original list will be intact.

   Remember that git-blame-colors is a buffer-local variable, but if it
   points to a global list, any destructive changes will mess up the
   global list.

You are damned right ! I did not check this before.

   I'd prefer something like this:

       (let ((color (if colors
		      (elt colors (random (length colors)))
		     git-blame-ancient-color)))

I agree too.

I hope the next patch will be the last for this "feature" :)
Thank you very much for all your comments.

Xavier

[PATCH] git-blame.el: pick a set of random colors for each git-blame turn

[Xavier Maillard]

I thought it would be cool to have different set of colors for each
git-blame-mode. Function `git-blame-new-commit' does this for us
picking when possible, a random colors based on the set we build on
startup. When it fails, `git-blame-ancient-color' will be used. We
also take care not to use the same color more than once (thank you
David Kågedal).

* 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 to #emacs). Also added a small
description of what it does.

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

* Added missing defvar to silent byte-compilers (git-blame-file,
git-blame-current)

* Do not require 'cl at startup

* Added more informations on compatibility

Signed-off-by: Xavier Maillard <zedek@gnu.org>
---
 contrib/emacs/git-blame.el |   88 +++++++++++++++++++++++++++++---------------
 1 files changed, 58 insertions(+), 30 deletions(-)

diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index bd87a86..aa176fd 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,51 @@
 ;;
 ;;; 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" ...)
+
+(defmacro git-blame-random-pop (l)
+  "Select a random element from L and returns it. Also remove
+selected element from l."
+  ;; only works on lists with unique elements
+  `(let ((e (elt ,l (random (length ,l)))))
+     (setq ,l (remove e ,l))
+     e))
 
 (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 +147,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 +203,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))
@@ -294,18 +320,20 @@ See also function `git-blame-mode'."
         (t
          nil)))
 
-
 (defun git-blame-new-commit (hash src-line res-line num-lines)
   (save-excursion
     (set-buffer git-blame-file)
     (let ((info (gethash hash git-blame-cache))
           (inhibit-point-motion-hooks t)
-          (inhibit-modification-hooks t))
+          (inhibit-modification-hooks t)
+	  (colors git-blame-colors))
       (when (not info)
-        (let ((color (pop git-blame-colors)))
-          (unless color
-            (setq color git-blame-ancient-color))
-          (setq info (list hash src-line res-line num-lines
+	;; Assign a random color to each new commit info
+	;; Take care not to select the same color multiple times
+	(let ((color (if colors
+			 (git-blame-random-pop colors)
+		       git-blame-ancient-color)))
+	  (setq info (list hash src-line res-line num-lines
                            (git-describe-commit hash)
                            (cons 'color color))))
         (puthash hash info git-blame-cache))
-- 
1.5.0.5

Re: [PATCH] git-blame.el: pick a set of random colors for each git-blame turn

[David Kågedal]

Xavier Maillard <zedek@gnu.org> writes:

> I thought it would be cool to have different set of colors for each
> git-blame-mode. Function `git-blame-new-commit' does this for us
> picking when possible, a random colors based on the set we build on
> startup. When it fails, `git-blame-ancient-color' will be used. We
> also take care not to use the same color more than once (thank you
> David Kågedal).

Closer, but still no cigar :-)

>  (defun git-blame-new-commit (hash src-line res-line num-lines)
>    (save-excursion
>      (set-buffer git-blame-file)
>      (let ((info (gethash hash git-blame-cache))
>            (inhibit-point-motion-hooks t)
> -          (inhibit-modification-hooks t))
> +          (inhibit-modification-hooks t)
> +	  (colors git-blame-colors))
>        (when (not info)
> -        (let ((color (pop git-blame-colors)))
> -          (unless color
> -            (setq color git-blame-ancient-color))
> -          (setq info (list hash src-line res-line num-lines
> +	;; Assign a random color to each new commit info
> +	;; Take care not to select the same color multiple times
> +	(let ((color (if colors
> +			 (git-blame-random-pop colors)
> +		       git-blame-ancient-color)))
> +	  (setq info (list hash src-line res-line num-lines
>                             (git-describe-commit hash)
>                             (cons 'color color))))
>          (puthash hash info git-blame-cache))

You are still making a copy of the list head pointer (colors ->
git-blame-colors), and then you do (git-blame-random-pop colors).
This will not update git-blame-colors if the first element was popped,
which means that you will keep reusing that color.  Since you really
do want to always update the buffer-local git-blame-colors, I don't
see why you bind a local variable and work with that instead.

And the last diff line is whitespace-only.  You replaced eight spaces
with a TAB.

-- 
David Kågedal

Re: [PATCH] git-blame.el: pick a set of random colors for each git-blame turn

[Xavier Maillard]

I thought it would be cool to have different set of colors for each
git-blame-mode. Function `git-blame-new-commit' does this for us
picking when possible, a random colors based on the set we build on
startup. When it fails, `git-blame-ancient-color' will be used. We
also take care not to use the same color more than once (thank you
David Kågedal, really).

* 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 to #emacs). Also added a small
description of what it does.

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

* Added missing defvar to silent byte-compilers (git-blame-file,
git-blame-current)

* Do not require 'cl at startup

* Added more informations on compatibility

Signed-off-by: Xavier Maillard <zedek@gnu.org>
---

   Closer, but still no cigar :-)

Can I have my cigar now ? :) I am really feeling stupid when
thinkg at this issue, really. I just understood what you meant
after a really long time. Credits for this patch could probably
be shared with you :) Once again, thank you.

 contrib/emacs/git-blame.el |   83 +++++++++++++++++++++++++++++---------------
 1 files changed, 55 insertions(+), 28 deletions(-)

diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index bd87a86..3c5efd8 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,51 @@
 ;;
 ;;; 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" ...)
+
+(defmacro git-blame-random-pop (l)
+  "Select a random element from L and returns it. Also remove
+selected element from l."
+  ;; only works on lists with unique elements
+  `(let ((e (elt ,l (random (length ,l)))))
+     (setq ,l (remove e ,l))
+     e))
 
 (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 +147,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 +203,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))
@@ -294,7 +320,6 @@ See also function `git-blame-mode'."
         (t
          nil)))
 
-
 (defun git-blame-new-commit (hash src-line res-line num-lines)
   (save-excursion
     (set-buffer git-blame-file)
@@ -302,9 +327,11 @@ 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))
+	;; Assign a random color to each new commit info
+	;; Take care not to select the same color multiple times
+	(let ((color (if git-blame-colors
+			 (git-blame-random-pop git-blame-colors)
+		       git-blame-ancient-color)))
           (setq info (list hash src-line res-line num-lines
                            (git-describe-commit hash)
                            (cons 'color color))))
-- 
1.5.0.5

Re: [PATCH] git-blame.el: pick a set of random colors for each git-blame turn

[David Kågedal]

Xavier Maillard <zedek@gnu.org> writes:

> I thought it would be cool to have different set of colors for each
> git-blame-mode. Function `git-blame-new-commit' does this for us
> picking when possible, a random colors based on the set we build on
> startup. When it fails, `git-blame-ancient-color' will be used. We
> also take care not to use the same color more than once (thank you
> David Kågedal, really).

Excellent.  This version looks good.

-- 
David Kågedal

Re: [PATCH] git-blame.el: pick a set of random colors for each git-blame turn

[Xavier Maillard]

   > I thought it would be cool to have different set of colors for each
   > git-blame-mode. Function `git-blame-new-commit' does this for us
   > picking when possible, a random colors based on the set we build on
   > startup. When it fails, `git-blame-ancient-color' will be used. We
   > also take care not to use the same color more than once (thank you
   > David Kågedal, really).

   Excellent.  This version looks good.

Phew ! :) Thank you for your help and support.

Xavier