Patchwork [1,of,2] discovery: weaken claim about returned common heads if ancestorsof are given

login
register
mail settings
Submitter Manuel Jacob
Date July 15, 2020, 11:56 a.m.
Message ID <b7d3118b1fe0d41e45f6.1594814177@tmp>
Download mbox | patch
Permalink /patch/46749/
State Accepted
Headers show

Comments

Manuel Jacob - July 15, 2020, 11:56 a.m.
# HG changeset patch
# User Manuel Jacob <me@manueljacob.de>
# Date 1594799471 -7200
#      Wed Jul 15 09:51:11 2020 +0200
# Node ID b7d3118b1fe0d41e45f630c4dd2422f56f753d81
# Parent  6a5dcd75484259095abca2c5b3f7ab12421a94ad
# EXP-Topic discovery
discovery: weaken claim about returned common heads if ancestorsof are given

As the test case shows, the claim is not true in general.
Yuya Nishihara - July 16, 2020, 12:50 p.m.
On Wed, 15 Jul 2020 13:56:17 +0200, Manuel Jacob wrote:
> # HG changeset patch
> # User Manuel Jacob <me@manueljacob.de>
> # Date 1594799471 -7200
> #      Wed Jul 15 09:51:11 2020 +0200
> # Node ID b7d3118b1fe0d41e45f630c4dd2422f56f753d81
> # Parent  6a5dcd75484259095abca2c5b3f7ab12421a94ad
> # EXP-Topic discovery
> discovery: weaken claim about returned common heads if ancestorsof are given

Looks good to me, but I don't have expertise about discovery logic.

Pierre-Yves, can you review these patches?
Pierre-Yves David - July 16, 2020, 2:24 p.m.
I'll have a look, thanks for the ping.

Please not that my email subscribed to the list is 
pierre-yves.david@ens-lyon.org (not my @octobus.net address) and CCing 
me on the other one is inconvenient :-)

On 7/16/20 2:50 PM, Yuya Nishihara wrote:
> On Wed, 15 Jul 2020 13:56:17 +0200, Manuel Jacob wrote:
>> # HG changeset patch
>> # User Manuel Jacob <me@manueljacob.de>
>> # Date 1594799471 -7200
>> #      Wed Jul 15 09:51:11 2020 +0200
>> # Node ID b7d3118b1fe0d41e45f630c4dd2422f56f753d81
>> # Parent  6a5dcd75484259095abca2c5b3f7ab12421a94ad
>> # EXP-Topic discovery
>> discovery: weaken claim about returned common heads if ancestorsof are given
> Looks good to me, but I don't have expertise about discovery logic.
>
> Pierre-Yves, can you review these patches?
Pierre-Yves David - July 17, 2020, 3:15 a.m.
This looks correct to me. Thanks of lot to Manuel for making this 
clarification work.

You should consider using mercurial.util.nouideprecwarn to catch (and 
have extension catch) user of the old attribute.



On 7/16/20 2:50 PM, Yuya Nishihara wrote:
> On Wed, 15 Jul 2020 13:56:17 +0200, Manuel Jacob wrote:
>> # HG changeset patch
>> # User Manuel Jacob <me@manueljacob.de>
>> # Date 1594799471 -7200
>> #      Wed Jul 15 09:51:11 2020 +0200
>> # Node ID b7d3118b1fe0d41e45f630c4dd2422f56f753d81
>> # Parent  6a5dcd75484259095abca2c5b3f7ab12421a94ad
>> # EXP-Topic discovery
>> discovery: weaken claim about returned common heads if ancestorsof are given
> 
> Looks good to me, but I don't have expertise about discovery logic.
> 
> Pierre-Yves, can you review these patches?
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Manuel Jacob - July 17, 2020, 4:16 a.m.
On 2020-07-17 05:15, Pierre-Yves David wrote:
> This looks correct to me. Thanks of lot to Manuel for making this
> clarification work.
> 
> You should consider using mercurial.util.nouideprecwarn to catch (and
> have extension catch) user of the old attribute.

In the current patch, I forward the new name to the old name without 
changing the users. However, I think that changing the name at all the 
users is a good idea because then it will become more obvious in the 
code if there is a bug. We’ll need to review all places using the 
attribute (and possibly fix some). Using "ancestorsof" at all the users 
will make the fix more obvious in the patch.

I could either:

1) send a follow-up patch doing the mass-rename and adding the warning 
(this means the already sent patch stays less noisy), or
2) send a new patch combining the docstring change and the mass-rename.

> On 7/16/20 2:50 PM, Yuya Nishihara wrote:
>> On Wed, 15 Jul 2020 13:56:17 +0200, Manuel Jacob wrote:
>>> # HG changeset patch
>>> # User Manuel Jacob <me@manueljacob.de>
>>> # Date 1594799471 -7200
>>> #      Wed Jul 15 09:51:11 2020 +0200
>>> # Node ID b7d3118b1fe0d41e45f630c4dd2422f56f753d81
>>> # Parent  6a5dcd75484259095abca2c5b3f7ab12421a94ad
>>> # EXP-Topic discovery
>>> discovery: weaken claim about returned common heads if ancestorsof 
>>> are given
>> 
>> Looks good to me, but I don't have expertise about discovery logic.
>> 
>> Pierre-Yves, can you review these patches?
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
Pierre-Yves David - July 17, 2020, 6:49 a.m.
On 7/17/20 6:16 AM, Manuel Jacob wrote:
> On 2020-07-17 05:15, Pierre-Yves David wrote:
>> This looks correct to me. Thanks of lot to Manuel for making this
>> clarification work.
>>
>> You should consider using mercurial.util.nouideprecwarn to catch (and
>> have extension catch) user of the old attribute.
> 
> In the current patch, I forward the new name to the old name without 
> changing the users. However, I think that changing the name at all the 
> users is a good idea because then it will become more obvious in the 
> code if there is a bug. We’ll need to review all places using the 
> attribute (and possibly fix some). Using "ancestorsof" at all the users 
> will make the fix more obvious in the patch.
> 
> I could either:
> 
> 1) send a follow-up patch doing the mass-rename and adding the warning 
> (this means the already sent patch stays less noisy), or
> 2) send a new patch combining the docstring change and the mass-rename.

Sending follow ups seems better.
Manuel Jacob - July 17, 2020, 7:54 a.m.
On 2020-07-17 08:49, Pierre-Yves David wrote:
> On 7/17/20 6:16 AM, Manuel Jacob wrote:
>> On 2020-07-17 05:15, Pierre-Yves David wrote:
>>> This looks correct to me. Thanks of lot to Manuel for making this
>>> clarification work.
>>> 
>>> You should consider using mercurial.util.nouideprecwarn to catch (and
>>> have extension catch) user of the old attribute.
>> 
>> In the current patch, I forward the new name to the old name without 
>> changing the users. However, I think that changing the name at all the 
>> users is a good idea because then it will become more obvious in the 
>> code if there is a bug. We’ll need to review all places using the 
>> attribute (and possibly fix some). Using "ancestorsof" at all the 
>> users will make the fix more obvious in the patch.
>> 
>> I could either:
>> 
>> 1) send a follow-up patch doing the mass-rename and adding the warning 
>> (this means the already sent patch stays less noisy), or
>> 2) send a new patch combining the docstring change and the 
>> mass-rename.
> 
> Sending follow ups seems better.

I send such a patch in reply to the patch fixing the docstring.
Yuya Nishihara - July 17, 2020, 11 a.m.
On Fri, 17 Jul 2020 09:54:41 +0200, Manuel Jacob wrote:
> On 2020-07-17 08:49, Pierre-Yves David wrote:
> > On 7/17/20 6:16 AM, Manuel Jacob wrote:
> >> On 2020-07-17 05:15, Pierre-Yves David wrote:
> >>> This looks correct to me. Thanks of lot to Manuel for making this
> >>> clarification work.
> >>> 
> >>> You should consider using mercurial.util.nouideprecwarn to catch (and
> >>> have extension catch) user of the old attribute.
> >> 
> >> In the current patch, I forward the new name to the old name without 
> >> changing the users. However, I think that changing the name at all the 
> >> users is a good idea because then it will become more obvious in the 
> >> code if there is a bug. We’ll need to review all places using the 
> >> attribute (and possibly fix some). Using "ancestorsof" at all the 
> >> users will make the fix more obvious in the patch.
> >> 
> >> I could either:
> >> 
> >> 1) send a follow-up patch doing the mass-rename and adding the warning 
> >> (this means the already sent patch stays less noisy), or
> >> 2) send a new patch combining the docstring change and the 
> >> mass-rename.
> > 
> > Sending follow ups seems better.
> 
> I send such a patch in reply to the patch fixing the docstring.

Queued these, thanks.

Patch

diff --git a/mercurial/discovery.py b/mercurial/discovery.py
--- a/mercurial/discovery.py
+++ b/mercurial/discovery.py
@@ -41,8 +41,8 @@ 
       any longer.
     "heads" is either the supplied heads, or else the remote's heads.
     "ancestorsof" if not None, restrict the discovery to a subset defined by
-      these nodes. Changeset outside of this set won't be considered (and
-      won't appears in "common")
+      these nodes. Changeset outside of this set won't be considered (but may
+      still appear in "common").
 
     If you pass heads and they are all known locally, the response lists just
     these heads in "common" and in "heads".
diff --git a/tests/test-setdiscovery.t b/tests/test-setdiscovery.t
--- a/tests/test-setdiscovery.t
+++ b/tests/test-setdiscovery.t
@@ -1112,3 +1112,40 @@ 
   * @5d0b986a083e0d91f116de4691e2aaa54d5bbec0 (*)> found 101 common and 1 unknown server heads, 1 roundtrips in *.????s (glob)
   * @5d0b986a083e0d91f116de4691e2aaa54d5bbec0 (*)> -R r1 outgoing r2 *-T{rev} * --config *extensions.blackbox=* exited 0 after *.?? seconds (glob)
   $ cd ..
+
+Even if the set of revs to discover is restricted, unrelated revs may be
+returned as common heads.
+
+  $ mkdir ancestorsof
+  $ cd ancestorsof
+  $ hg init a
+  $ hg clone a b -q
+  $ cd b
+  $ hg debugbuilddag '.:root *root *root'
+  $ hg log -G -T '{node|short}'
+  o  fa942426a6fd
+  |
+  | o  66f7d451a68b
+  |/
+  o  1ea73414a91b
+  
+  $ hg push -r 66f7d451a68b -q
+  $ hg debugdiscovery --verbose --rev fa942426a6fd
+  comparing with $TESTTMP/ancestorsof/a
+  searching for changes
+  elapsed time:  * seconds (glob)
+  heads summary:
+    total common heads:          1
+      also local heads:          1
+      also remote heads:         1
+      both:                      1
+    local heads:                 2
+      common:                    1
+      missing:                   1
+    remote heads:                1
+      common:                    1
+      unknown:                   0
+  local changesets:              3
+    common:                      2
+    missing:                     1
+  common heads: 66f7d451a68b