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

From: Jeff King <peff@peff.net>
Date: Sep 7, 2016, 22:08 UTC
Message-ID: <20160907220803.3cvjkchpmyff7h5f@sigill.intra.peff.net>
In-Reply-To: <20160907184653.ed5c4oklyywjj2gp@sigill.intra.peff.net>

On Wed, Sep 07, 2016 at 02:46:53PM -0400, Jeff King wrote:

Show 8 quoted lines

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

Ah, I see. I initially was looking at an older git (v2.6.x), in which commit_patch_id() is a static function inside patch-ids.c, and we do not do any lazy-load trickery. But note that the patch is still wrong even there; it should return "-1" from commit_patch_id() to instruct the caller not to add it to the hash.

Anyway...

Show 7 quoted lines

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

It's not too hard to do so, but it raises a question of what "format-patch --base" would want. I've just sent another RFC cc-ing folks interested in that area.

Thanks again for the review.

-Peff