On Wed, Sep 07, 2016 at 02:46:53PM -0400, Jeff King wrote: > > 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... > > 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