Re: [PATCH 2/2] patch-ids: skip merge commits

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Wed, 7 Sep 2016, Jeff King wrote:

> [...]
> This patch just ignores merge commits entirely when
> generating patch-ids, meaning they will never be matched
> (from either side of a symmetric-diff traversal).

... except it does not ignore merge commits:

> diff --git a/patch-ids.c b/patch-ids.c
> index 77e4663..b1f8514 100644
> --- a/patch-ids.c
> +++ b/patch-ids.c
> @@ -7,10 +7,12 @@
>  int commit_patch_id(struct commit *commit, struct diff_options *options,
>  		    unsigned char *sha1, int diff_header_only)
>  {
> -	if (commit->parents)
> +	if (commit->parents) {
> +		if (commit->parents->next)
> +			return 0;
>  		diff_tree_sha1(commit->parents->item->object.oid.hash,
>  			       commit->object.oid.hash, "", options);
> -	else
> +	} else

With this change, commit_patch_id() will return 0 for merge commits
(indicating success) but it will not have touched the sha1! Which means it
may very well have all kinds of crap in the sha1 that may, or may not,
match another, real patch ID randomly.

See e.g. the call site in builtin/log.c's prepare_bases():

	[...]
	if (commit_patch_id(commit, &diffopt, sha1, 0))
		die(_("cannot get patch id"));
	ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);
	patch_id = bases->patch_id + bases->nr_patch_id;
	hashcpy(patch_id->hash, sha1);
	bases->nr_patch_id++;

So the sha1 is actually used, even if it was not initialized.

I would suggest to simply copy the merge commit's SHA-1. It is no patch
ID, of course, but collisions are as unlikely as commit name collisions,
and it would make the "patch ID" of a merge commit deterministic again.

I.e. something like

 	if (commit->parents) {
-		if (commit->parents->next)
+		if (commit->parents->next) {
+			hashcpy(sha1, commit->object.oid.hash);
 			return 0;
+		}
   		diff_tree_sha1(commit->parents->item->object.oid.hash,

on top.

Ciao,
Dscho