Am 5/2/2013 8:46, schrieb Jeff King:
> On Thu, May 02, 2013 at 08:44:07AM +0200, Johannes Sixt wrote:
>> Am 5/1/2013 22:34, schrieb Jeff King:
>>> struct object *lookup_object(const unsigned char *sha1)
>>> {
>>> - unsigned int i;
>>> + unsigned int i, first;
>>> struct object *obj;
>>>
>>> if (!obj_hash)
>>> return NULL;
>>>
>>> - i = hashtable_index(sha1);
>>> + first = i = hashtable_index(sha1);
>>> while ((obj = obj_hash[i]) != NULL) {
>>> if (!hashcmp(sha1, obj->sha1))
>>> break;
>>> @@ -85,6 +85,11 @@ struct object *lookup_object(const unsigned char *sha1)
>>> if (i == obj_hash_size)
>>> i = 0;
>>> }
>>> + if (obj && i != first) {
>>> + struct object *tmp = obj_hash[i];
>>> + obj_hash[i] = obj_hash[first];
>>> + obj_hash[first] = tmp;
>>> + }
>>> return obj;
>>> }
>>
>> This is one of the places where I think the code does not speak for itself
>> and a comment is warranted: The new if statement is not about correctness,
>> but about optimization:
>
> I figured the lengthy description in the commit message would be
> sufficient,
It's absolutely sufficient *if* one reads the commit message. In this
case, though it goes more like "this function should be trivial, and it is
-- up to this if statement; what the heck is it good for?" and the reader
is forced to dig the history.
BTW, do you notice that the function is now modifying an object (the hash
table) even though this is rather unexpected from a "lookup" function?
-- Hannes
--
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