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

From: Arnaud Morin <arnaud.morin@gmail.com>
Date: Jan 13, 2021, 09:24 UTC
Message-ID: <20210113092448.GE32482@sync>
In-Reply-To: <xmqqy2gy3r5p.fsf@gitster.c.googlers.com>

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.

# WITH PATCH With the patch, everything is consistent, and only W is kept in ouptut, no matter the size of history: $ git.p rev-list --left-right --cherry-pick branch...master <W

Cheers,

-- 
Arnaud Morin

On 12.01.21 - 11:13, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > At the end, we'll have eliminated commits from both sides that have a
> > matching patch-id on the other side. But there's a subtle assumption
> > here: for any given patch-id, we must have exactly one struct
> > representing it. If two commits from A both have the same patch-id and
> > we allow duplicates in the hashmap, then we run into a problem:
> 
> In practical terms, for one side of the history to have two
> identical patches, the most likely scenerio for that to happen is to
> have a patch, its reversion, and its reapplication, intermixed in
> its history with other commits, e.g.
> 
>     ---o---o---M---o---o---W---o---o---M---o--- ...
>         \
>          o---o---o---M---o--- ...
> 
> where "M" are commits that does an identical change, and "W"
> (upside-down "M") is its reversion.  On the top history, "M" was
> introduced, the bottom history cherry-picked, the top history found
> problems in it and reverted with "W", and later (perhaps with the
> help of preparatory patches on top of "W"), the top history now
> considers that "M" is safe, so reapplies it.
> 
> And a "--cherry-pick" output that excludes "identical patches" that
> appear on both sides on such a history would hide all "M"'s, leaving
> a history like
> 
>     ---o---o-------o---o---W---o---o-------o--- ...
>         \
>          o---o---o-------o--- ...
> 
> But is this result what the user really wanted to see, I have to
> wonder.
> 
> I do not see any problem in the patch itself.  We used to hide only
> one "M" from the history on the top half in the picture, leaving one
> "M" and "W", while hiding the sole "M" from the bottom half.  Now if
> we want to no longer show any "M", the updated code would correctly
> hide all of them.
> 
> It just feels to me that the resulting view of the history look
> weird, leaving only the reversion of a patch that has never been
> applied in the first place.