Re: [RFC/PATCH 0/2] more patch-id speedups

[Jeff King <peff@peff.net>]

On Wed, Sep 07, 2016 at 03:06:36PM +0200, Johannes Schindelin wrote:

> > This is marked as "RFC" because I don't feel entirely confident that I'm
> > not missing some clever need for these options. But in both cases my gut
> > feeling is that they are simply unintended effects that nobody ever
> > noticed, because it would be very rare that they would affect the
> > output. And that if they _did_ affect the output, they would probably be
> > doing the wrong thing.
> 
> Given that the patch ID is *wrong* for merge commits (it only looks at the
> first parent, so each "-s ours" merge will have the same patch ID!), I
> would say that we can get away with re-defining the patch ID of merge
> commits.
> 
> The only case where it might change things that I can think of would be a
> `git rebase --preserve-merges`: it would probably have worked *by chance*
> before (or not, in case of "-s ours" merges), and now it would try to pick
> the merge commits even if rebased versions were already merged upstream.
> 
> If I read the --preserve-merges code correctly, that would result in the
> merge commit's parents to be 'rewritten' to HEAD. And as both parents
> would be rewritten to HEAD, they would be condensed into a single new
> parent, resulting in a cherry-pick that fails (because it tries to
> cherry-pick a merge commit without any -n option).
> 
> Of course, what we could do is to introduce a modifier, e.g.
> --cherry-pick=first-parent, that would trigger the old behavior and would
> be asked-for in the --preserve-merges mode.
> 
> But quite frankly, personally I would not worry about it *that* much. As
> you pointed out, the patch ID for merge commits is incorrect to begin
> with, and we may just redeclare all merge commits to be incomparable to
> one another when it comes to patch IDs.
> 
> In short: I would be fine with the change of behavior.

Thanks for this explanation; it matches what I was thinking, but you
went through it in a lot more detail.

So it sounds like this is the right thing, but as you pointed out, the
implementation is just silly. I'll see if I can come up with a working
v2.

-Peff