Re: [PATCH v2 4/4] describe: Delay looking up commits until searching for an inexact match

From: Anders Kaseorg <andersk@ksplice.com>
Date: Dec 8, 2010, 23:47 UTC
Message-ID: <alpine.DEB.2.02.1012081800540.23348@dr-wily.mit.edu>
In-Reply-To: <7v7hfjkhfm.fsf@alter.siamese.dyndns.org>

On Wed, 8 Dec 2010, Junio C Hamano wrote:

Show 8 quoted lines

> You seem to have gone in a slightly different direction with this reroll.
> I am not sure if use of hash_table in this code would actually improve
> anything (aside from the general readability and "reusing code from a good
> infrastructure is a good thing" principle), though, no matter how many
> tags you have in your repository.  In the code from the earlier round,
> lookup_commit_reference_gently call in the fallback codepath to populate
> commit->util used the *obj_hash[] to quickly look up the commits with the
> same object name already.

Yes; the problem now is that, in order to avoid calling replace_name() excessively, I need to resolve all multiply-tagged commits in one pass, before I have those commits in the obj_hash. This could have been done with the linked list by deleting any commit_name from the linked list the first time replace_name() decides it should be superseded by a different commit_name, but the hash table approach seems less fragile to me. The hash table also has the advantage of avoiding the O(#tags * #arguments) complexity when describe is given many arguments.

Let me know if you’d like me to try it the other way.

> I don't know how the above would work in the face of hash collisions.

Oh right.  I’ll fix that.

static int set_util(void *chain)
{
	struct commit_name *n;
	for (n = chain; n; n = n->next) {
		struct commit *c = lookup_commit_reference_gently(n->peeled, 1);
		if (c)
			c->util = n;
	}
	return 0;
}

> Do we know that all the archs we will be compiled on will be happy with
> this potentially unaligned access?  hash_filespec() in diffcore-rename.c
> is written in a way to avoid such an issue, and I would feel safer to 
> see this do the same.

I’ll fix that too.

static inline unsigned int hash_sha1(const unsigned char *sha1)
{
	unsigned int hash;
	memcpy(&hash, sha1, sizeof(hash));
	return hash;
}

Anders