Re: [PATCH] git-blame.el: pick a set of random...

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

From: David Kågedal <hidden>
Date: 2016-06-15 22:43:02

Xavier Maillard [off-list ref] writes:

quoted hunk

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

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

From: Xavier Maillard <hidden>
Date: 2016-06-15 22:43:02

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help