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

From: David Kågedal <davidk@lysator.liu.se>
Date: Mar 28, 2007, 09:15 UTC
Message-ID: <874po54tle.fsf@morpheus.local>
In-Reply-To: <200703272151.l2RLpqD7012317@localhost.localdomain>

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