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

From: David Kågedal <[email-masked]>
Date: Wed, 28 Mar 2007 14:02:53 +0200

Xavier Maillard <[email-masked]> 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