From: Xavier Maillard <zedek@gnu.org>
Date: Wed, 28 Mar 2007 10:29:16 GMT
Subject: Re: [PATCH] git-blame.el: pick a set of random colors for each git-blame turn
Message-ID: <200703281029.l2SATGlZ004563@localhost.localdomain>
In-Reply-To: <874po54tle.fsf@morpheus.local>

Hi David,

   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.

You are damned right ! I did not check this before.

   I'd prefer something like this:

       (let ((color (if colors
		      (elt colors (random (length colors)))
		     git-blame-ancient-color)))

I agree too.

I hope the next patch will be the last for this "feature" :)
Thank you very much for all your comments.

Xavier