Re: [PATCH] patch-ids: handle duplicate hashmap entries

From: Jeff King <peff@peff.net>
Date: Jan 13, 2021, 12:59 UTC
Message-ID: <X/7ur/IcCg1AqTdZ@coredump.intra.peff.net>
In-Reply-To: <20210113092448.GE32482@sync>

On Wed, Jan 13, 2021 at 09:24:48AM +0000, Arnaud Morin wrote:

Show 26 quoted lines

> Without this patch, that's even worst, consistency is broken.
> Let me explain.
> 
> With your history example:
> 
>      ---o---o---M---o---o---W---o---o---M---o--- branch
>          \
>           o---o---o---M---o--- master
> 
> # WITHOUT PATCH
> If we imagine that master is having more commits count than branch.
> The result of rev-list will be like you described:
> $ git rev-list --left-right --cherry-pick branch...master
> <M
> <W
> 
> In other words, it's showing both W and M.
> 
> BUT, if we imagine now that master is having less commits count than branch.
> $ git rev-list --left-right --cherry-pick branch...master
> <W
> 
> It's only showing W!
> 
> So the result is not consistent, depending on which branch is having
> more commits.

Right. There's definitely a question of whether --cherry-pick is the most useful thing in such a situation, but the current behavior was simply broken. It did not behave as advertised, and it treated one side of the history different from the other. So whether we want to do anything else to help that case, I think this at least makes --cherry-pick sane. :)

Here are two related histories to think about.

This is the same history, but the revert and re-do happens on both sides (and this is actually the setup in the new test):

      ---o---o---M---o---o---W---o---o---M---o--- branch
          \
           o---o---M---o---o---W---o---o---M---o--- master

There we really _do_ want to clear out both M's (really, all four), because we're doing so _from both sides_. And that is ultimately the point of the cherry-pick option: show commits that were picked from one side to the other.

Another interesting case is when the first "M" is actually bundled into another commit, like:

      ---o---o---M+X---o---o---W---o---o---M---o--- branch
          \
           o---o---o--M---o--- master

where "M+X" has the changes from "M" plus some other changes. It will get a different patch-id, and --cherry-pick would show both M+X and W, but not M for either side. This is a limitation of the patch-id concept: we are looking at whole commits, not overall changes.

I suspect for most operations we care less about "remove all cherry-picks from both sides", but rather "give me one side, omitting commits that are already on the other side" (because you want to rebase or cherry-pick some subset).

-Peff