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

[Jeff King <peff@peff.net>]

On Wed, Sep 07, 2016 at 02:52:04PM +0200, Johannes Schindelin wrote:

> > 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.

Eek, thanks. Somehow I got it into my head that diff_flush_patch_id()
below was what added it to the list, but clearly that is not the case.
Looking at it again, I can't imagine how that is the case.

> 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 agree that would work, though it does mean carrying extra useless
entries in the patch_id hash. I'll see how bad it would be to simply
omit them entirely, but this seems like a good fallback plan.

Thanks, and sorry for the obviously braindead patch.

-Peff