Patchwork [2,of,2] discovery: properly filter changeset in 'peer.known' (issue4982)

login
register
mail settings
Submitter Pierre-Yves David
Date Dec. 6, 2015, 8:54 p.m.
Message ID <cce8d1dfcefa5495af79.1449435258@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/11867/
State Superseded
Headers show

Comments

Pierre-Yves David - Dec. 6, 2015, 8:54 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1449101535 28800
#      Wed Dec 02 16:12:15 2015 -0800
# Node ID cce8d1dfcefa5495af7900ea5cff796691ca6d07
# Parent  179e1f468d683a1b9e7e8df76af0690b454c4025
# EXP-Topic fixknown
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r cce8d1dfcefa
discovery: properly filter changeset in 'peer.known' (issue4982)

The 'peer.known' call (handled at the repository level) was applying its own
manual filtering (looking at phases) instead of relying on the repoview
mechanism. This led to the discovery finding more "common" node that
'getbundle' was willing to recognised. From there, bad things happen, issue4982
is a symptom of it. While situations like described in issue4982 can still
happen because of race conditions, fixing 'peer.known' is important for
consistency in all cases.

We update the code to use 'repoview' filtering. This lead to small changes in
the tests for exchanging obsolescence marker because the discovery yields
different results.
Gregory Szorc - Dec. 6, 2015, 9:04 p.m.
On Sun, Dec 6, 2015 at 12:54 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1449101535 28800
> #      Wed Dec 02 16:12:15 2015 -0800
> # Node ID cce8d1dfcefa5495af7900ea5cff796691ca6d07
> # Parent  179e1f468d683a1b9e7e8df76af0690b454c4025
> # EXP-Topic fixknown
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
> cce8d1dfcefa
> discovery: properly filter changeset in 'peer.known' (issue4982)
>

This should be marked BC because it changes semantics of the "known" wire
protocol command. (The new semantics make sense to me modulo the bug
described below.)


>
> The 'peer.known' call (handled at the repository level) was applying its
> own
> manual filtering (looking at phases) instead of relying on the repoview
> mechanism. This led to the discovery finding more "common" node that
> 'getbundle' was willing to recognised. From there, bad things happen,
> issue4982
> is a symptom of it. While situations like described in issue4982 can still
> happen because of race conditions, fixing 'peer.known' is important for
> consistency in all cases.
>
> We update the code to use 'repoview' filtering. This lead to small changes
> in
> the tests for exchanging obsolescence marker because the discovery yields
> different results.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -806,16 +806,17 @@ class localrepository(object):
>
>          repo = (remote and remote.local()) and remote or self
>          return repo[key].branch()
>
>      def known(self, nodes):
> -        nm = self.changelog.nodemap
> -        pc = self._phasecache
> +        cl = self.filtered('served').changelog
>

hgweb supports specifying the filter to serve with. Therefore, I believe
manually specifying a filter here is wrong, as a server started with `hg
--hidden serve` won't expose hidden changesets even though you told it to.
(This also likely points to a gap in test coverage.)

Why can't you simply use self.changelog? wireproto.py should be operating
on a repo instance with the appropriate filter (if any) already in place.


> +        nm = cl.nodemap
> +        filtered = cl.filteredrevs
>          result = []
>          for n in nodes:
>              r = nm.get(n)
> -            resp = not (r is None or pc.phase(self, r) >= phases.secret)
> +            resp = not (r is None or r in filtered)
>              result.append(resp)
>          return result
>
>      def local(self):
>          return self
> diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
> --- a/tests/test-obsolete.t
> +++ b/tests/test-obsolete.t
> @@ -743,12 +743,11 @@ This test issue 3805
>    searching for changes
>    3:323a9c3ddd91 (draft) [tip ] A
>    $ hg outgoing
>    comparing with $TESTTMP/tmpe/repo-issue3805 (glob)
>    searching for changes
> -  no changes found
> -  [1]
> +  1:29f0c6921ddd (draft) [tip ] A
>
>  #if serve
>
>    $ hg serve -R ../repo-issue3805 -n test -p $HGPORT -d --pid-file=hg.pid
> -A access.log -E errors.log
>    $ cat hg.pid >> $DAEMON_PIDS
> @@ -758,12 +757,11 @@ This test issue 3805
>    searching for changes
>    2:323a9c3ddd91 (draft) [tip ] A
>    $ hg outgoing http://localhost:$HGPORT
>    comparing with http://localhost:$HGPORT/
>    searching for changes
> -  no changes found
> -  [1]
> +  1:29f0c6921ddd (draft) [tip ] A
>
>    $ killdaemons.py
>
>  #endif
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Dec. 6, 2015, 10:10 p.m.
On 12/06/2015 01:04 PM, Gregory Szorc wrote:
> On Sun, Dec 6, 2015 at 12:54 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@fb.com
>     <mailto:pierre-yves.david@fb.com>>
>     # Date 1449101535 28800
>     #      Wed Dec 02 16:12:15 2015 -0800
>     # Node ID cce8d1dfcefa5495af7900ea5cff796691ca6d07
>     # Parent  179e1f468d683a1b9e7e8df76af0690b454c4025
>     # EXP-Topic fixknown
>     # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>     #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/
>     -r cce8d1dfcefa
>     discovery: properly filter changeset in 'peer.known' (issue4982)
>
>
> This should be marked BC because it changes semantics of the "known"
> wire protocol command. (The new semantics make sense to me modulo the
> bug described below.)

Meh, this is fixing a bug in a wire protocol command that only affects 
people using an experimental extensions. I do not think it deserve any 
special flagging.

[...]
>     diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>     --- a/mercurial/localrepo.py
>     +++ b/mercurial/localrepo.py
>     @@ -806,16 +806,17 @@ class localrepository(object):
>
>               repo = (remote and remote.local()) and remote or self
>               return repo[key].branch()
>
>           def known(self, nodes):
>     -        nm = self.changelog.nodemap
>     -        pc = self._phasecache
>     +        cl = self.filtered('served').changelog
>
>
> hgweb supports specifying the filter to serve with. Therefore, I believe
> manually specifying a filter here is wrong, as a server started with `hg
> --hidden serve` won't expose hidden changesets even though you told it
> to. (This also likely points to a gap in test coverage.)
>
> Why can't you simply use self.changelog? wireproto.py should be
> operating on a repo instance with the appropriate filter (if any)
> already in place.


Ha, nice catch, I got a bit over zealous here. I've sending a V2 with 
the filtering call dropped.
Gregory Szorc - Dec. 6, 2015, 10:20 p.m.
On Sun, Dec 6, 2015 at 2:10 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 12/06/2015 01:04 PM, Gregory Szorc wrote:
>
>> On Sun, Dec 6, 2015 at 12:54 PM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>> wrote:
>>
>>     # HG changeset patch
>>     # User Pierre-Yves David <pierre-yves.david@fb.com
>>     <mailto:pierre-yves.david@fb.com>>
>>     # Date 1449101535 28800
>>     #      Wed Dec 02 16:12:15 2015 -0800
>>     # Node ID cce8d1dfcefa5495af7900ea5cff796691ca6d07
>>     # Parent  179e1f468d683a1b9e7e8df76af0690b454c4025
>>     # EXP-Topic fixknown
>>     # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>>     #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/
>>     -r cce8d1dfcefa
>>     discovery: properly filter changeset in 'peer.known' (issue4982)
>>
>>
>> This should be marked BC because it changes semantics of the "known"
>> wire protocol command. (The new semantics make sense to me modulo the
>> bug described below.)
>>
>
> Meh, this is fixing a bug in a wire protocol command that only affects
> people using an experimental extensions. I do not think it deserve any
> special flagging.
>

BC is BC. We want this to show up in the release notes so that anyone using
an experimental or unknown 3rd party extension isn't confounded when 3.7
changes behavior.


>
> [...]
>
>>     diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>>     --- a/mercurial/localrepo.py
>>     +++ b/mercurial/localrepo.py
>>     @@ -806,16 +806,17 @@ class localrepository(object):
>>
>>               repo = (remote and remote.local()) and remote or self
>>               return repo[key].branch()
>>
>>           def known(self, nodes):
>>     -        nm = self.changelog.nodemap
>>     -        pc = self._phasecache
>>     +        cl = self.filtered('served').changelog
>>
>>
>> hgweb supports specifying the filter to serve with. Therefore, I believe
>> manually specifying a filter here is wrong, as a server started with `hg
>> --hidden serve` won't expose hidden changesets even though you told it
>> to. (This also likely points to a gap in test coverage.)
>>
>> Why can't you simply use self.changelog? wireproto.py should be
>> operating on a repo instance with the appropriate filter (if any)
>> already in place.
>>
>
>
> Ha, nice catch, I got a bit over zealous here. I've sending a V2 with the
> filtering call dropped.
>
>
> --
> Pierre-Yves David
>
Augie Fackler - Dec. 7, 2015, 7:03 p.m.
On Sun, Dec 06, 2015 at 02:20:59PM -0800, Gregory Szorc wrote:
> On Sun, Dec 6, 2015 at 2:10 PM, Pierre-Yves David <
> pierre-yves.david@ens-lyon.org> wrote:

[...]

> >> This should be marked BC because it changes semantics of the "known"
> >> wire protocol command. (The new semantics make sense to me modulo the
> >> bug described below.)
> >>
> >
> > Meh, this is fixing a bug in a wire protocol command that only affects
> > people using an experimental extensions. I do not think it deserve any
> > special flagging.
> >
>
> BC is BC. We want this to show up in the release notes so that anyone using
> an experimental or unknown 3rd party extension isn't confounded when 3.7
> changes behavior.

+1, mark it BC to be safe.
Pierre-Yves David - Dec. 7, 2015, 7:04 p.m.
On 12/07/2015 11:03 AM, Augie Fackler wrote:
> On Sun, Dec 06, 2015 at 02:20:59PM -0800, Gregory Szorc wrote:
>> On Sun, Dec 6, 2015 at 2:10 PM, Pierre-Yves David <
>> pierre-yves.david@ens-lyon.org> wrote:
>
> [...]
>
>>>> This should be marked BC because it changes semantics of the "known"
>>>> wire protocol command. (The new semantics make sense to me modulo the
>>>> bug described below.)
>>>>
>>>
>>> Meh, this is fixing a bug in a wire protocol command that only affects
>>> people using an experimental extensions. I do not think it deserve any
>>> special flagging.
>>>
>>
>> BC is BC. We want this to show up in the release notes so that anyone using
>> an experimental or unknown 3rd party extension isn't confounded when 3.7
>> changes behavior.
>
> +1, mark it BC to be safe.

okay, can someone fix it inflight?
Augie Fackler - Dec. 8, 2015, 3:41 p.m.
On Mon, Dec 07, 2015 at 11:04:34AM -0800, Pierre-Yves David wrote:
>
>
> On 12/07/2015 11:03 AM, Augie Fackler wrote:
> >On Sun, Dec 06, 2015 at 02:20:59PM -0800, Gregory Szorc wrote:
> >>On Sun, Dec 6, 2015 at 2:10 PM, Pierre-Yves David <
> >>pierre-yves.david@ens-lyon.org> wrote:
> >
> >[...]
> >
> >>>>This should be marked BC because it changes semantics of the "known"
> >>>>wire protocol command. (The new semantics make sense to me modulo the
> >>>>bug described below.)
> >>>>
> >>>
> >>>Meh, this is fixing a bug in a wire protocol command that only affects
> >>>people using an experimental extensions. I do not think it deserve any
> >>>special flagging.
> >>>
> >>
> >>BC is BC. We want this to show up in the release notes so that anyone using
> >>an experimental or unknown 3rd party extension isn't confounded when 3.7
> >>changes behavior.
> >
> >+1, mark it BC to be safe.
>
> okay, can someone fix it inflight?

Queued with (BC) added.

>
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -806,16 +806,17 @@  class localrepository(object):
 
         repo = (remote and remote.local()) and remote or self
         return repo[key].branch()
 
     def known(self, nodes):
-        nm = self.changelog.nodemap
-        pc = self._phasecache
+        cl = self.filtered('served').changelog
+        nm = cl.nodemap
+        filtered = cl.filteredrevs
         result = []
         for n in nodes:
             r = nm.get(n)
-            resp = not (r is None or pc.phase(self, r) >= phases.secret)
+            resp = not (r is None or r in filtered)
             result.append(resp)
         return result
 
     def local(self):
         return self
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -743,12 +743,11 @@  This test issue 3805
   searching for changes
   3:323a9c3ddd91 (draft) [tip ] A
   $ hg outgoing
   comparing with $TESTTMP/tmpe/repo-issue3805 (glob)
   searching for changes
-  no changes found
-  [1]
+  1:29f0c6921ddd (draft) [tip ] A
 
 #if serve
 
   $ hg serve -R ../repo-issue3805 -n test -p $HGPORT -d --pid-file=hg.pid -A access.log -E errors.log
   $ cat hg.pid >> $DAEMON_PIDS
@@ -758,12 +757,11 @@  This test issue 3805
   searching for changes
   2:323a9c3ddd91 (draft) [tip ] A
   $ hg outgoing http://localhost:$HGPORT
   comparing with http://localhost:$HGPORT/
   searching for changes
-  no changes found
-  [1]
+  1:29f0c6921ddd (draft) [tip ] A
 
   $ killdaemons.py
 
 #endif