<div dir="ltr"><br><br><br>On Wed, Feb 26, 2014 at 12:54 AM, Mads Kiilerich <<a href="mailto:mads@kiilerich.com">mads@kiilerich.com</a>> wrote:<br>><br>> On 02/24/2014 11:31 PM, Matt Mackall wrote:<br>>><br>
>> On Mon, 2014-02-24 at 23:19 +0100, Mads Kiilerich wrote:<br>>>><br>>>> [...]<br>>>> The function seemed to solve a harder problem than necessary. It gave some<br>>>> common ancestors priority over others. [...]<br>
>><br>>> I'll need to think on this. But you should probably share some<br>>> benchmarks.<br>><br>><br>> What benchmarks do you have in mind?<br>><br>> My concern here is only correctness, not performance.<br>
> The patch just removes a post processing step in<br>> the gca calculation [...]<br><br>When bos added the function you're removing,<br>he was very happy of the speedup he got:<br><a href="http://selenic.com/hg/rev/2f7186400a07">http://selenic.com/hg/rev/2f7186400a07</a><br>
<br>The previous ancestor() code was renamed to genericancestor()<br>(dead code you are removing in another patch).<br>The reason that code wasn't deleted I guess<br>is in this 0/N message of that same series by bos:<br>
<a href="http://markmail.org/message/64gbckmjmmwvdt3b">http://markmail.org/message/64gbckmjmmwvdt3b</a><br><br>I think an important point is:<br>does mercurial need to bend the definition of Greater Common Ancestor,<br>which is heads(::X and ::Y), adding the clause that the base<br>
of merges needs also to be "the furthest from the root" ?<br><br>One can say that it kinda make sense because "furthest from a root"<br>can mean "more recent" in the topological sense.<br>But if we say that we drop this, you're indeed saving some work<br>
with your code removal.<br>Otherwise, it has to be done elsewhere and you might not have all info you need at that point.<br><br>Cheers,<br>GGhh<br><br></div>