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 - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html