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

From: Jeff King <peff@peff.net>
Date: Sep 7, 2016, 18:46 UTC
Message-ID: <20160907184653.ed5c4oklyywjj2gp@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1609071338310.129229@virtualbox>

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

Show 21 quoted lines

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