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

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

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