From: Junio C Hamano <hidden> Date: 2021-01-12 19:14:09
Jeff King [off-list ref] 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.
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 [off-list ref] writes:
quoted
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.
From: Jeff King <hidden> Date: 2021-01-13 13:00:15
On Wed, Jan 13, 2021 at 09:24:48AM +0000, Arnaud Morin wrote:
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