Patchwork [10,of,10] phabricator: do not read a same revision twice

login
register
mail settings
Submitter Jun Wu
Date July 5, 2017, 1:58 a.m.
Message ID <6f1f74ecc788edf66ab9.1499219915@x1c>
Download mbox | patch
Permalink /patch/22005/
State Accepted
Headers show

Comments

Jun Wu - July 5, 2017, 1:58 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1499219548 25200
#      Tue Jul 04 18:52:28 2017 -0700
# Node ID 6f1f74ecc788edf66ab9b0e8717724b97666f037
# Parent  0e2c9cf54e09bacb4a019bc51693a9fecf1051a3
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 6f1f74ecc788
phabricator: do not read a same revision twice

It's possible to set up non-linear dependencies in Phabricator like:

  o   D4
  |\
  | o D3
  | |
  o | D2
  |/
  o   D1

The old `phabread` code will print D1 twice. This patch adds de-duplication
to prevent that.

Test Plan:
Construct the above dependencies in a Phabricator test instance and make
sure the old code prints D1 twice while the new code won't.
Sean Farley - July 5, 2017, 2:41 a.m.
Jun Wu <quark@fb.com> writes:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1499219548 25200
> #      Tue Jul 04 18:52:28 2017 -0700
> # Node ID 6f1f74ecc788edf66ab9b0e8717724b97666f037
> # Parent  0e2c9cf54e09bacb4a019bc51693a9fecf1051a3
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 6f1f74ecc788
> phabricator: do not read a same revision twice

This series is generic for any phabricator instance, correct? I mean,
we're not hardcoding any assumptions or things like that.

> It's possible to set up non-linear dependencies in Phabricator like:
>
>   o   D4
>   |\
>   | o D3
>   | |
>   o | D2
>   |/
>   o   D1
>
> The old `phabread` code will print D1 twice. This patch adds de-duplication
> to prevent that.
>
> Test Plan:
> Construct the above dependencies in a Phabricator test instance and make
> sure the old code prints D1 twice while the new code won't.

Is part of this test going to include sending a (threaded) email with
the patch? Hopefully, comments from phabricator will get mirrored to the
list as well so that subscribers can see the reviews (it seems that
mailing list -> phabricator doesn't exist currently).

There is another issue I worry a lot about with any new review system
and that is keeping the discussion archives on the mailing list. Having
more than one place of archives is completely untenable in my experience
(do I search phabricator? the mailing list? something else?).
Furthermore, the mailing list is archived by many entities right now.
Having a hosted solution runs the risk of a single point of failure
(server dying, etc.).
Phillip Cohen - July 5, 2017, 5:25 a.m.
> Hopefully, comments from phabricator will get mirrored to the
list as well so that subscribers can see the reviews

+1 on this. It should be pretty easy to configure on the Phab side.

On Tue, Jul 4, 2017 at 7:41 PM, Sean Farley <sean@farley.io> wrote:
>
> Jun Wu <quark@fb.com> writes:
>
>> # HG changeset patch
>> # User Jun Wu <quark@fb.com>
>> # Date 1499219548 25200
>> #      Tue Jul 04 18:52:28 2017 -0700
>> # Node ID 6f1f74ecc788edf66ab9b0e8717724b97666f037
>> # Parent  0e2c9cf54e09bacb4a019bc51693a9fecf1051a3
>> # Available At https://bitbucket.org/quark-zju/hg-draft
>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 6f1f74ecc788
>> phabricator: do not read a same revision twice
>
> This series is generic for any phabricator instance, correct? I mean,
> we're not hardcoding any assumptions or things like that.
>
>> It's possible to set up non-linear dependencies in Phabricator like:
>>
>>   o   D4
>>   |\
>>   | o D3
>>   | |
>>   o | D2
>>   |/
>>   o   D1
>>
>> The old `phabread` code will print D1 twice. This patch adds de-duplication
>> to prevent that.
>>
>> Test Plan:
>> Construct the above dependencies in a Phabricator test instance and make
>> sure the old code prints D1 twice while the new code won't.
>
> Is part of this test going to include sending a (threaded) email with
> the patch? Hopefully, comments from phabricator will get mirrored to the
> list as well so that subscribers can see the reviews (it seems that
> mailing list -> phabricator doesn't exist currently).
>
> There is another issue I worry a lot about with any new review system
> and that is keeping the discussion archives on the mailing list. Having
> more than one place of archives is completely untenable in my experience
> (do I search phabricator? the mailing list? something else?).
> Furthermore, the mailing list is archived by many entities right now.
> Having a hosted solution runs the risk of a single point of failure
> (server dying, etc.).
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - July 5, 2017, 5:13 p.m.
On Tue, Jul 04, 2017 at 07:41:04PM -0700, Sean Farley wrote:
>
> Jun Wu <quark@fb.com> writes:
>
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1499219548 25200
> > #      Tue Jul 04 18:52:28 2017 -0700
> > # Node ID 6f1f74ecc788edf66ab9b0e8717724b97666f037
> > # Parent  0e2c9cf54e09bacb4a019bc51693a9fecf1051a3
> > # Available At https://bitbucket.org/quark-zju/hg-draft
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 6f1f74ecc788
> > phabricator: do not read a same revision twice
>
> This series is generic for any phabricator instance, correct? I mean,
> we're not hardcoding any assumptions or things like that.
>
> > It's possible to set up non-linear dependencies in Phabricator like:
> >
> >   o   D4
> >   |\
> >   | o D3
> >   | |
> >   o | D2
> >   |/
> >   o   D1
> >
> > The old `phabread` code will print D1 twice. This patch adds de-duplication
> > to prevent that.
> >
> > Test Plan:
> > Construct the above dependencies in a Phabricator test instance and make
> > sure the old code prints D1 twice while the new code won't.
>
> Is part of this test going to include sending a (threaded) email with
> the patch? Hopefully, comments from phabricator will get mirrored to the
> list as well so that subscribers can see the reviews (it seems that
> mailing list -> phabricator doesn't exist currently).

It's our intent to turn on email notifications to the list, however:

 * I doubt phabricator gets threading right
 * The emails I've seen don't look very text/plain friendly

So it may be a mixed bag.

> There is another issue I worry a lot about with any new review system
> and that is keeping the discussion archives on the mailing list. Having
> more than one place of archives is completely untenable in my experience
> (do I search phabricator? the mailing list? something else?).
> Furthermore, the mailing list is archived by many entities right now.
> Having a hosted solution runs the risk of a single point of failure
> (server dying, etc.).

Yep. Which is why phabricator is an experiment. We need to kick the
tires a bit before we decide if we want to use it (wether optionally
or as the one true place to do reviews).

Kevin and I are still trying to sort out the configuration issues that
were only discoverable by having it set up, so it's not really
"usable" per se yet (but I hope it will be today).


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Sean Farley - July 5, 2017, 8:47 p.m.
Augie Fackler <raf@durin42.com> writes:

> On Tue, Jul 04, 2017 at 07:41:04PM -0700, Sean Farley wrote:
>>
>> Jun Wu <quark@fb.com> writes:
>>
>> > # HG changeset patch
>> > # User Jun Wu <quark@fb.com>
>> > # Date 1499219548 25200
>> > #      Tue Jul 04 18:52:28 2017 -0700
>> > # Node ID 6f1f74ecc788edf66ab9b0e8717724b97666f037
>> > # Parent  0e2c9cf54e09bacb4a019bc51693a9fecf1051a3
>> > # Available At https://bitbucket.org/quark-zju/hg-draft
>> > #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 6f1f74ecc788
>> > phabricator: do not read a same revision twice
>>
>> This series is generic for any phabricator instance, correct? I mean,
>> we're not hardcoding any assumptions or things like that.
>>
>> > It's possible to set up non-linear dependencies in Phabricator like:
>> >
>> >   o   D4
>> >   |\
>> >   | o D3
>> >   | |
>> >   o | D2
>> >   |/
>> >   o   D1
>> >
>> > The old `phabread` code will print D1 twice. This patch adds de-duplication
>> > to prevent that.
>> >
>> > Test Plan:
>> > Construct the above dependencies in a Phabricator test instance and make
>> > sure the old code prints D1 twice while the new code won't.
>>
>> Is part of this test going to include sending a (threaded) email with
>> the patch? Hopefully, comments from phabricator will get mirrored to the
>> list as well so that subscribers can see the reviews (it seems that
>> mailing list -> phabricator doesn't exist currently).
>
> It's our intent to turn on email notifications to the list, however:
>
>  * I doubt phabricator gets threading right

This makes me sad :-(

>  * The emails I've seen don't look very text/plain friendly

This makes me even double sad!!

> So it may be a mixed bag.
>
>> There is another issue I worry a lot about with any new review system
>> and that is keeping the discussion archives on the mailing list. Having
>> more than one place of archives is completely untenable in my experience
>> (do I search phabricator? the mailing list? something else?).
>> Furthermore, the mailing list is archived by many entities right now.
>> Having a hosted solution runs the risk of a single point of failure
>> (server dying, etc.).
>
> Yep. Which is why phabricator is an experiment. We need to kick the
> tires a bit before we decide if we want to use it (wether optionally
> or as the one true place to do reviews).

Gotcha. I guess we'll see what tires need to be kicked in the next few
weeks.
Augie Fackler - July 5, 2017, 10:30 p.m.
On Tue, Jul 04, 2017 at 06:58:35PM -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1499219548 25200
> #      Tue Jul 04 18:52:28 2017 -0700
> # Node ID 6f1f74ecc788edf66ab9b0e8717724b97666f037
> # Parent  0e2c9cf54e09bacb4a019bc51693a9fecf1051a3
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 6f1f74ecc788
> phabricator: do not read a same revision twice

Queued this one too, thanks

(That leaves only patch 9, where I had some questions)

>
> It's possible to set up non-linear dependencies in Phabricator like:
>
>   o   D4
>   |\
>   | o D3
>   | |
>   o | D2
>   |/
>   o   D1
>
> The old `phabread` code will print D1 twice. This patch adds de-duplication
> to prevent that.
>
> Test Plan:
> Construct the above dependencies in a Phabricator test instance and make
> sure the old code prints D1 twice while the new code won't.
>
> diff --git a/contrib/phabricator.py b/contrib/phabricator.py
> --- a/contrib/phabricator.py
> +++ b/contrib/phabricator.py
> @@ -382,4 +382,5 @@ def querydrev(repo, params, stack=False)
>          return prefetched[key]
>
> +    visited = set()
>      result = []
>      queue = [params]
> @@ -387,4 +388,7 @@ def querydrev(repo, params, stack=False)
>          params = queue.pop()
>          drev = fetch(params)
> +        if drev[r'id'] in visited:
> +            continue
> +        visited.add(drev[r'id'])
>          result.append(drev)
>          if stack:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -382,4 +382,5 @@  def querydrev(repo, params, stack=False)
         return prefetched[key]
 
+    visited = set()
     result = []
     queue = [params]
@@ -387,4 +388,7 @@  def querydrev(repo, params, stack=False)
         params = queue.pop()
         drev = fetch(params)
+        if drev[r'id'] in visited:
+            continue
+        visited.add(drev[r'id'])
         result.append(drev)
         if stack: