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