Re: [[PATCH v2] 4/4] rebase: avoid computing unnecessary patch IDs

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 29, 2016, 21:46 UTC
Message-ID: <xmqqa8h0m82f.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160729161920.3792-5-kcwillford@gmail.com>

Kevin Willford <kcwillford@gmail.com> writes:

Show 15 quoted lines

>  static int patch_id_cmp(struct patch_id *a,
>  			struct patch_id *b,
> -			void *keydata)
> +			struct diff_options *opt)
>  {
> +	if (is_null_sha1(a->patch_id) &&
> +	    commit_patch_id(a->commit, opt, a->patch_id, 0))
> +		return error("Could not get patch ID for %s",
> +			oid_to_hex(&a->commit->object.oid));
> +	if (is_null_sha1(b->patch_id) &&
> +	    commit_patch_id(b->commit, opt, b->patch_id, 0))
> +		return error("Could not get patch ID for %s",
> +			oid_to_hex(&b->commit->object.oid));
>  	return hashcmp(a->patch_id, b->patch_id);
>  }

These error returns initially looks slightly iffy in that in general the caller of any_cmp_fn() wants to know how a/b compares, but by returning error(), it always says "a is smaller than b". This however may be OK because the callers in hashmap_get* implementation only wants to know "are they equal?", and we are saying "no they cannot possibly be equal" here. The original that ran a full commit_patch_id() in now-removed add_commit() helper function didn't even diagnose this error and silently omitted the commit from the candidate list, so this may be even seen as an improvement.

The idea of using the two level hash, computing the more expensive one only when the hashmap hashes of the result of the cheaper hash function collide, is excellent. Thanks.