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

[Xavier Maillard <zedek@gnu.org>]

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