threads / patch / 7416

patch, 2 partsgit-blame.el: pick a set of random colors when blaming

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

## tl;dr

12 messages between Mar 26, 2007 and Mar 29, 2007. Diffs are folded; open one to read it.

replies: 11people: 2as markdown or json

Xavier Maillard· Mar 26, 2007, 21:01 UTC · lore

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

Show changes to contrib/emacs/git-blame.el +44 −27

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

David Kågedal· Mar 27, 2007, 08:44 UTC · re: Xavier Maillard · lore

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

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:

Show 12 quoted lines

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

Show 115 quoted lines

> 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

Xavier Maillard· Mar 27, 2007, 15:31 UTC · re: David Kågedal · lore

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

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

David Kågedal· Mar 28, 2007, 08:49 UTC · re: Xavier Maillard · lore

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

Xavier Maillard <zedek@gnu.org> writes:

Show 8 quoted lines

> 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

Xavier Maillard· Mar 27, 2007, 21:51 UTC · re: David Kågedal · lore

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

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

Show changes to contrib/emacs/git-blame.el +52 −30

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

David Kågedal· Mar 28, 2007, 09:15 UTC · re: Xavier Maillard · lore

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

Xavier Maillard <zedek@gnu.org> writes:

Show 29 quoted lines

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

Xavier Maillard· Mar 28, 2007, 10:29 UTC · re: David Kågedal · lore

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

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

Xavier Maillard· Mar 28, 2007, 10:31 UTC · re: David Kågedal · lore

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

* 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(-)

Show changes to contrib/emacs/git-blame.el +58 −30

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

David Kågedal· Mar 28, 2007, 12:02 UTC · re: Xavier Maillard · lore

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

Xavier Maillard <zedek@gnu.org> writes:

Show 6 quoted lines

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

Show 22 quoted lines

>  (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

Xavier Maillard· Mar 28, 2007, 16:44 UTC · re: David Kågedal · lore

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

* 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(-)

Show changes to contrib/emacs/git-blame.el +55 −28

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

David Kågedal· Mar 29, 2007, 09:26 UTC · re: Xavier Maillard · lore

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

Xavier Maillard <zedek@gnu.org> writes:

Show 6 quoted lines

> 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

Xavier Maillard· Mar 29, 2007, 09:59 UTC · re: David Kågedal · lore

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

   > 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