llvm dev - Jul 2014 - [LLVMdev] Usability of phabricator review threads for non-phab-users

Alp noted that the current setup on how phab reviews land on the list are
not working for him. I'd be curious whether his setup is special, or
whether there are more widespread problems. If this is more widely
perceived as a problem, please speak up, and I'll make sure to prioritize
the fixes (note that this is unrelated to the "lost email" problem -
those
are always highest priority and as far as I'm aware we diagnosed and fixed
all of them within 1-2 business days).

If you have the feeling that the phab email workflow makes it hard for you
to jump into reviews, keep track of reviews, or understand reviews if
you're not a phab user, please reply to this thread. You don't need to
provide details, "+1", "please fix", or "doesn't
work well for me" are all
acceptable replies here - I want to get a feeling for the magnitude of the
problem.

Thanks,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140701/a9fe4fd3/attachment.html>

On 01/07/2014 13:11, Manuel Klimek wrote:
> Alp noted that the current setup on how phab reviews land on the list are
> not working for him. I'd be curious whether his setup is special, or
> whether there are more widespread problems. If this is more widely
> perceived as a problem, please speak up, and I'll make sure to
prioritize
> the fixes (note that this is unrelated to the "lost email"
problem - those
> are always highest priority and as far as I'm aware we diagnosed and
fixed
> all of them within 1-2 business days).
>
> If you have the feeling that the phab email workflow makes it hard for you
> to jump into reviews, keep track of reviews, or understand reviews if
> you're not a phab user, please reply to this thread. You don't need
to
> provide details, "+1", "please fix", or
"doesn't work well for me" are all
> acceptable replies here - I want to get a feeling for the magnitude of the
> problem.
Hi Manuel,

thanks for looking into this.

I remember me having issues due to patch comments missing relevant 
context when looking at them on the mailing list. I think this has been 
both too little code lines, but possibly also previous comments which 
have not been cited. When people use email, they normally ensure that
the relevant code/comments are properly cited. Phabricator does not need 
this in the web interface, which makes the emails sometimes less useful.

However, I must say I don't have much experience with it and keep 
avoiding it, mostly because it does not work offline, I can not use 'vi'
to search/work through the patches, and I need to go to a web-interface 
to submit patches instead of doing this from the command line (maybe 
there exists a script already). To get these to worlds closer, it would 
be amazing if we could have phabricator to automatically open new review 
threads if patches appear on the mailing list, to integrate comments 
sent on the mailing list and to close issues if it detects a 'committed 
in'. I know this is a lot of work, but I still wanted to mention this as 
features that would make me use phabricator more. (Most likely still 
sending emails, but using it to get
an overview about open reviews or general review status)

Cheers,
Tobias

Hi,

I have found Phabricator a better organized way to review patches than the
e-mail lists. e-mail threads are sometimes split  so you have to search for
the previous thread. e-mail threads do not show as nice as Phab shows, with
all the diffs ready and highlighted (I have a diff viewer of course but
that's an extra step). It's not always easy to find the latest patch
version in long threads.

The main problems with the current setup are
1) Phab does not enforce posting on the list (this was already discussed on
the list).
2) The coordination issues Tobias listed, most important that e-mail
replies to the Phab thread (on the list) that do not appear in Phab. It
means that once a review was started on Phab no one should reply on the
list else his reply would not show in Phab. I try to manually copy such
replies and paste to Phab comments but that's not a solution.

Yaron






2014-07-01 14:33 GMT+03:00 Tobias Grosser <tobias at grosser.es>:
> On 01/07/2014 13:11, Manuel Klimek wrote:
>
>> Alp noted that the current setup on how phab reviews land on the list
are
>> not working for him. I'd be curious whether his setup is special,
or
>> whether there are more widespread problems. If this is more widely
>> perceived as a problem, please speak up, and I'll make sure to
prioritize
>> the fixes (note that this is unrelated to the "lost email"
problem - those
>> are always highest priority and as far as I'm aware we diagnosed
and fixed
>> all of them within 1-2 business days).
>>
>> If you have the feeling that the phab email workflow makes it hard for
you
>> to jump into reviews, keep track of reviews, or understand reviews if
>> you're not a phab user, please reply to this thread. You don't
need to
>> provide details, "+1", "please fix", or
"doesn't work well for me" are all
>> acceptable replies here - I want to get a feeling for the magnitude of
the
>> problem.
>>
>
> Hi Manuel,
>
> thanks for looking into this.
>
> I remember me having issues due to patch comments missing relevant context
> when looking at them on the mailing list. I think this has been both too
> little code lines, but possibly also previous comments which have not been
> cited. When people use email, they normally ensure that
> the relevant code/comments are properly cited. Phabricator does not need
> this in the web interface, which makes the emails sometimes less useful.
>
> However, I must say I don't have much experience with it and keep
avoiding
> it, mostly because it does not work offline, I can not use 'vi' to
> search/work through the patches, and I need to go to a web-interface to
> submit patches instead of doing this from the command line (maybe there
> exists a script already). To get these to worlds closer, it would be
> amazing if we could have phabricator to automatically open new review
> threads if patches appear on the mailing list, to integrate comments sent
> on the mailing list and to close issues if it detects a 'committed
in'. I
> know this is a lot of work, but I still wanted to mention this as features
> that would make me use phabricator more. (Most likely still sending emails,
> but using it to get
> an overview about open reviews or general review status)
>
> Cheers,
> Tobias
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140701/77e75d31/attachment.html>

On Tue, Jul 01, 2014 at 12:11:07PM +0100, Manuel Klimek
wrote:
> Alp noted that the current setup on how phab reviews land on the list are
> not working for him. I'd be curious whether his setup is special, or
> whether there are more widespread problems.
The biggest problem I see is the duplication of replies. E.g., if the
comment originated from the mailing list, phab should at most forward it
as BCC, but not as reply to the mail itself. That makes reading the
lists a lot more difficult to read.

Joerg

On Tue, Jul 1, 2014 at 4:11 AM, Manuel Klimek <klimek at google.com>
wrote:
> Alp noted that the current setup on how phab reviews land on the list are
> not working for him. I'd be curious whether his setup is special, or
> whether there are more widespread problems. If this is more widely
> perceived as a problem, please speak up, and I'll make sure to
prioritize
> the fixes (note that this is unrelated to the "lost email"
problem - those
> are always highest priority and as far as I'm aware we diagnosed and
fixed
> all of them within 1-2 business days).
>
> If you have the feeling that the phab email workflow makes it hard for you
> to jump into reviews, keep track of reviews, or understand reviews if
> you're not a phab user, please reply to this thread. You don't need
to
> provide details, "+1", "please fix", or
"doesn't work well for me" are all
> acceptable replies here - I want to get a feeling for the magnitude of the
> problem.
>
I have noticed, at least in some of the reviews, that replies made by email
to Phab's emails do not get into the web app. So after some time passes,
the Phab review is not useful as the sole reference to a change - one has
to fish the maillist archives for the discussion that surrounds it because
some replies were made by email.

Maybe it's a setting one has to tweak per review?

Other than that, Phab is certainly a step up from mailing patches. I think
we can all agree that all LLVM developers are acting in good faith (and
reverts are easy), and technical problems can be resolved.

 Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140701/c76cd447/attachment.html>

On 01/07/2014 13:33, Tobias Grosser wrote:
> On 01/07/2014 13:11, Manuel Klimek wrote:
>> Alp noted that the current setup on how phab reviews land on the list
are
>> not working for him. I'd be curious whether his setup is special,
or
>> whether there are more widespread problems. If this is more widely
>> perceived as a problem, please speak up, and I'll make sure to
prioritize
>> the fixes (note that this is unrelated to the "lost email"
problem -
>> those
>> are always highest priority and as far as I'm aware we diagnosed
and
>> fixed
>> all of them within 1-2 business days).
>>
>> If you have the feeling that the phab email workflow makes it hard for
>> you
>> to jump into reviews, keep track of reviews, or understand reviews if
>> you're not a phab user, please reply to this thread. You don't
need to
>> provide details, "+1", "please fix", or
"doesn't work well for me" are
>> all
>> acceptable replies here - I want to get a feeling for the magnitude of
>> the
>> problem.
>
> Hi Manuel,
>
> thanks for looking into this.
>
> I remember me having issues due to patch comments missing relevant
> context when looking at them on the mailing list. I think this has been
> both too little code lines, but possibly also previous comments which
> have not been cited. When people use email, they normally ensure that
> the relevant code/comments are properly cited. Phabricator does not need
> this in the web interface, which makes the emails sometimes less useful.
Below an example of such a "less useful reply".
> -------- Original Message --------
> Subject: Re: [PATCH] [Peephole] Advanced rewriting of copies to avoid cross
register banks copies.
> Date: Tue, 1 Jul 2014 13:04:01 +0000
> From: Quentin Colombet <qcolombet at apple.com>
> Reply-To: reviews+D4086+public+3593472fe90901bd at reviews.llvm.org
> To: qcolombet at apple.com
> CC: llvm-commits at cs.uiuc.edu
>
> ===============> Comment at: lib/CodeGen/PeepholeOptimizer.cpp:981
> @@ +980,3 @@
> +    return getNextSourceFromInsertSubreg(SrcIdx, SrcSubReg);
> +  if (Def->getOpcode() == TargetOpcode::EXTRACT_SUBREG)
> +    return getNextSourceFromExtractSubreg(SrcIdx, SrcSubReg);
> ----------------
> hfinkel at anl.gov wrote:
> > Quentin Colombet wrote:
> > > hfinkel at anl.gov wrote:
> > > > I think you might as well add isExtractSubreg().
> > > Agree.
> > > Though, I did not want to do that in that patch.
> > > I can either do it before or after this patch lands.
> > > Any preference?
> > I don't have a strong preference. Personally, I'd do it first.
> I thought there were plenty of users for this accessor but turns out all
the users check the opcode of a SDNode whereas I am looking at MachineInstr.
Thus, this patch is the first user of isExtractSubreg.
> So I have updated the patch to include that change.
>
> http://reviews.llvm.org/D4086
Here some more context:

   if (Def->isCopy())
     return getNextSourceFromCopy(SrcIdx, SrcSubReg);
   if (Def->isBitcast())
     return getNextSourceFromBitcast(SrcIdx, SrcSubReg);
   // All the remaining cases involve "complex" instructions.
   // Bails if we did not ask for the advanced tracking.
   if (!UseAdvancedTracking)
     return false;
   if (Def->isRegSequence())
     return getNextSourceFromRegSequence(SrcIdx, SrcSubReg);
   if (Def->isInsertSubreg())
     return getNextSourceFromInsertSubreg(SrcIdx, SrcSubReg);
   if (Def->getOpcode() == TargetOpcode::EXTRACT_SUBREG)

Only with the additional context it is visible, that all ifs are of the 
form:

   if (Def->isSomething)

but only the last is of the form:

   if (Def->getOpcode() == TargetOpcode::EXTRACT_SUBREG)

This is what Hal is referring to, but the email message lacks the
context and is consequently not understandable. I understand that you
try to keep the traffic low, but I believe adding context does not cost
a lot, as we can easily skim over it. So more context (+10 lines?)
before (and possibly even after the comment) would be very helpful.

Also, I am missing the function name this change is applied to. diff
is normally very good in finding the name of the function a change is
applied to. This information is lost in the phabricator emails.

And while at it. I think it is unfortunate that the 'author wrote' lines
are between the change and the comment as this reduces the distance 
between the code and the comments. Finally, by using indentation for the
patch itself, mail clients format the citation properly and the temporal 
order of the patch and the comments becomes clear.

Overall, I formatting this email as follows would change this email from 
not understandable to very useful.

hfinkel at anl.gov wrote:
> Quentin Colombet wrote:
> > hfinkel at anl.gov wrote:
> > > Comment at: lib/CodeGen/PeepholeOptimizer.cpp:981
> > > > @@ +980,3 @@ bool boolValueTracker::getNextSourceImpl()
> > > > + if (Def->isBitcast())
> > > > +   return getNextSourceFromBitcast(SrcIdx, SrcSubReg);
> > > > + // All the remaining cases involve "complex"
instructions.
> > > > + // Bails if we did not ask for the advanced tracking.
> > > > + if (!UseAdvancedTracking)
> > > > +   return false;
> > > > + if (Def->isRegSequence())
> > > > +   return getNextSourceFromRegSequence(SrcIdx, SrcSubReg);
> > > > + if (Def->isInsertSubreg())
> > > > +    return getNextSourceFromInsertSubreg(SrcIdx,
SrcSubReg);
> > > > +  if (Def->getOpcode() == TargetOpcode::EXTRACT_SUBREG)
> > > > +    return getNextSourceFromExtractSubreg(SrcIdx,
SrcSubReg);
> > > I think you might as well add isExtractSubreg().
> > Agree.
> > Though, I did not want to do that in that patch.
> > I can either do it before or after this patch lands.
> > Any preference?
> I don't have a strong preference. Personally, I'd do it first.
I thought there were plenty of users for this accessor but turns out all 
the users check the opcode of a SDNode whereas I am looking at 
MachineInstr. Thus, this patch is the first user of isExtractSubreg.
So I have updated the patch to include that change.
> > > > +   if (Def->isSubregToReg())
> > > > +     return getNextSourceFromSubregToReg(SrcIdx,
SrcSubReg);
> > > > +   return false;
> > > > + }
> > > > +
Cheers,
Tobias

On Tue, Jul 1, 2014 at 7:11 AM, Manuel Klimek <klimek at google.com>
wrote:
> Alp noted that the current setup on how phab reviews land on the list are
> not working for him. I'd be curious whether his setup is special, or
whether
> there are more widespread problems. If this is more widely perceived as a
> problem, please speak up, and I'll make sure to prioritize the fixes
(note
> that this is unrelated to the "lost email" problem - those are
always
> highest priority and as far as I'm aware we diagnosed and fixed all of
them
> within 1-2 business days).
>
> If you have the feeling that the phab email workflow makes it hard for you
> to jump into reviews, keep track of reviews, or understand reviews if
you're
> not a phab user, please reply to this thread. You don't need to provide
> details, "+1", "please fix", or "doesn't work
well for me" are all
> acceptable replies here - I want to get a feeling for the magnitude of the
> problem.
I gave Phab a shot a while back, found it didn't fit into my workflow,
and went back to performing all of my reviews via email. I would be
amenable to giving Phab another shot in the future, but right now, I
would say it doesn't work for me as a review tool and it's not an
interface I use when performing my reviews. From an email-only
reviewer perspective, Phab has some problems (but they seem to be
resolved quickly when brought up): Phab emails not making it to the
list, emails from the list not making it to Phab, Phab duplicating
emails sent to the list, incomprehensible messages from Phab, thread
splitting, etc. I chalk these up to "early adoption within a big
community", and while they're all annoying to varying degrees, nothing
has struck me as a deal-break or impossible to resolve.

Personally, I have a weak preference for reviewing items that do not
originate from Phab, but that doesn't discourage me from responding to
Phab reviews.

~Aaron

On 07/01/2014 04:11 AM, Manuel Klimek wrote:
> Alp noted that the current setup on how phab reviews land on the list 
> are not working for him. I'd be curious whether his setup is special, 
> or whether there are more widespread problems. If this is more widely 
> perceived as a problem, please speak up, and I'll make sure to 
> prioritize the fixes (note that this is unrelated to the "lost
email"
> problem - those are always highest priority and as far as I'm aware we 
> diagnosed and fixed all of them within 1-2 business days).
>
> If you have the feeling that the phab email workflow makes it hard for 
> you to jump into reviews, keep track of reviews, or understand reviews 
> if you're not a phab user, please reply to this thread. You don't
need
> to provide details, "+1", "please fix", or
"doesn't work well for me"
> are all acceptable replies here - I want to get a feeling for the 
> magnitude of the problem.
>
> Thanks,
> /Manuel
I generally prefer phabricator for longer or more detailed reviews. I 
will sometimes respond directly through the list for small patches when 
the code is obvious.

A few issues I've noticed:
- Confusion around adding llvm-commits
- Confusion around when email is actually sent.  Particularly with the 
approve and close steps.
- Phab doesn't recognize LGTM in an email response

Philip

On Tue, Jul 1, 2014 at 5:11 AM, Manuel Klimek <klimek at google.com>
wrote:
> Alp noted that the current setup on how phab reviews land on the list are
> not working for him. I'd be curious whether his setup is special, or
> whether there are more widespread problems. If this is more widely
> perceived as a problem, please speak up, and I'll make sure to
prioritize
> the fixes (note that this is unrelated to the "lost email"
problem - those
> are always highest priority and as far as I'm aware we diagnosed and
fixed
> all of them within 1-2 business days).
>
> If you have the feeling that the phab email workflow makes it hard for you
> to jump into reviews, keep track of reviews, or understand reviews if
> you're not a phab user, please reply to this thread. You don't need
to
> provide details, "+1", "please fix", or
"doesn't work well for me" are all
> acceptable replies here - I want to get a feeling for the magnitude of the
> problem.
>
My biggest issue is with email replies not getting into the webui (or
getting horribly mangled by being interpreted as "phab markdown").

A minor nuisance is that after replying on phab I get an email of what I
just wrote, which is sort of useless.

-- Sean Silva

>
> Thanks,
> /Manuel
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140701/7dc56b03/attachment.html>

On Tue, Jul 1, 2014 at 4:11 AM, Manuel Klimek <klimek at google.com>
wrote:
> Alp noted that the current setup on how phab reviews land on the list are
> not working for him. I'd be curious whether his setup is special, or
> whether there are more widespread problems. If this is more widely
> perceived as a problem, please speak up, and I'll make sure to
prioritize
> the fixes (note that this is unrelated to the "lost email"
problem - those
> are always highest priority and as far as I'm aware we diagnosed and
fixed
> all of them within 1-2 business days).
>
> If you have the feeling that the phab email workflow makes it hard for you
> to jump into reviews, keep track of reviews, or understand reviews if
> you're not a phab user, please reply to this thread. You don't need
to
> provide details, "+1", "please fix", or
"doesn't work well for me" are all
> acceptable replies here - I want to get a feeling for the magnitude of the
> problem.
>
I wonder if some of this stems from approach.

Here is how I view Phab: It is a tool to help me create emails for the
email-based code review. The code review is still 100% an email code review
in my mind. I take absolutely nothing on the Phab interface as
authoritative. This isn't a knock on Phab at all, it's just that I
understand at least some of my reviewers may not be using the tool, and I
want to make sure I'm interacting reasonably with them via email.

Some consequences of this approach:

1) I always check that my scribbles in Phab show up in email. When they
don't, I stop and try to figure out what happened. Usually waiting 30
minutes results in the email showing up.

2) I work really hard when composing a review response in Phab to quote the
code in question in a way that makes my comments readable in email. I
assume that the other end of the review may not read the comments in the
web interface.

3) I always check the email thread first for comments, and then switch to
the web interface when there are code-related-comments that aren't clear in
the nice contextualized diff format that Manuel taught Phab to produce for
the emails.

4) I will often provide non-code comments in a review thread directly via
email to simplify the process. This essentially always works, and doesn't
require me to do the extra work in #1 and #2.


Not sure this email-centric workflow works for everyone, but it's been a
very pragmatic pattern for me for the past year.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140701/d997c237/attachment.html>

Specifically the problem I've been seeing is that people using the 
website are unable to CC mailing list-based developers. As a result I 
don't get copied in on responses to my review comments, and rarely get 
any kind of direct mail with threading. You end up having to dig up 
historic responses in the mailing list archive which becomes tedious.

Often the CC on website reviews will include arbitrary names of people 
who have website accounts, while excluding the actual code owners and 
recent committers who you'd expect would be relevant. This leads me to 
guess that the website is actively blocking the email addresses of LLVM 
developers from getting added to the CC list unless they open an account 
on the service.

In fact as far as I can tell, mailing list-based developers are 
*completely* excluded from the CC list visible on the website. This 
creates a really poor workflow with responses often getting missed, and 
the right people not seeing patches (and conversely, it looks like 
people who aren't really relevant end up getting pressured into 
reviewing a patch in some area).

Alp.



On 01/07/2014 14:11, Manuel Klimek wrote:
> Alp noted that the current setup on how phab reviews land on the list 
> are not working for him. I'd be curious whether his setup is special, 
> or whether there are more widespread problems. If this is more widely 
> perceived as a problem, please speak up, and I'll make sure to 
> prioritize the fixes (note that this is unrelated to the "lost
email"
> problem - those are always highest priority and as far as I'm aware we 
> diagnosed and fixed all of them within 1-2 business days).
>
> If you have the feeling that the phab email workflow makes it hard for 
> you to jump into reviews, keep track of reviews, or understand reviews 
> if you're not a phab user, please reply to this thread. You don't
need
> to provide details, "+1", "please fix", or
"doesn't work well for me"
> are all acceptable replies here - I want to get a feeling for the 
> magnitude of the problem.
>
> Thanks,
> /Manuel
>
-- 
http://www.nuanti.com
the browser experts

+cfe-dev, bcc: cfe-commits (you meant cfe-dev, right?)
> On 2014-Jul-01, at 04:11, Manuel Klimek <klimek at google.com> wrote:
> 
> Alp noted that the current setup on how phab reviews land on the list are
not working for him. I'd be curious whether his setup is special, or whether
there are more widespread problems. If this is more widely perceived as a
problem, please speak up, and I'll make sure to prioritize the fixes (note
that this is unrelated to the "lost email" problem - those are always
highest priority and as far as I'm aware we diagnosed and fixed all of them
within 1-2 business days).
> 
> If you have the feeling that the phab email workflow makes it hard for you
to jump into reviews, keep track of reviews, or understand reviews if you're
not a phab user, please reply to this thread. You don't need to provide
details, "+1", "please fix", or "doesn't work well
for me" are all acceptable replies here - I want to get a feeling for the
magnitude of the problem.
> 
> Thanks,
> /Manuel
+1: I prefer non-phab reviews, and have caught myself skipping the ones
on phab.

It's great that you're working on this.  Mail clients seem only to get
worse at handling threads, and the features phab has for managing
patches and commits that need reviewing look promising.

These are the downsides from my perspective:

  - The email thread often doesn't have a patch attached.
  - Other emails/responses are missing.
  - Responses that are there are often duplicated (two emails per
    response).
  - Responses sometimes don't contain context.
  - Can't post patch series in single thread.

For context, my workflow for both pre- and post-commit review is to do
small reviews directly in email, and large ones by looking at the patch
and resulting commit locally (I'm far happier in my editor than a web
interface).

On 01/07/2014 21:28, Alp Toker wrote:
> Specifically the problem I've been seeing is that people using the 
> website are unable to CC mailing list-based developers. As a result I 
> don't get copied in on responses to my review comments, and rarely get 
> any kind of direct mail with threading. You end up having to dig up 
> historic responses in the mailing list archive which becomes tedious.
>
> Often the CC on website reviews will include arbitrary names of people 
> who have website accounts, while excluding the actual code owners and 
> recent committers who you'd expect would be relevant. This leads me to 
> guess that the website is actively blocking the email addresses of 
> LLVM developers from getting added to the CC list unless they open an 
> account on the service.
To back this up, I get about a dozen mails a month saying "I can't find
you on Phabricator", to which I usually reply "Just enter my committer
name / email address".

  AFAICT people rarely do that, or the site blocks the email address and 
tries to make me create an account which I'm not planning to do at present.

The net result is that other people else ends up CC'ed because they do 
have an account on the website, and they attempt to review the code even 
though someone else requested the changes. At that point it becomes a 
matter of dealing with the fallout and things get pointlessly awkward :-/

Alp.
>
> In fact as far as I can tell, mailing list-based developers are 
> *completely* excluded from the CC list visible on the website. This 
> creates a really poor workflow with responses often getting missed, 
> and the right people not seeing patches (and conversely, it looks like 
> people who aren't really relevant end up getting pressured into 
> reviewing a patch in some area).
>
> Alp.
>
>
>
> On 01/07/2014 14:11, Manuel Klimek wrote:
>> Alp noted that the current setup on how phab reviews land on the list 
>> are not working for him. I'd be curious whether his setup is
special,
>> or whether there are more widespread problems. If this is more widely 
>> perceived as a problem, please speak up, and I'll make sure to 
>> prioritize the fixes (note that this is unrelated to the "lost
email"
>> problem - those are always highest priority and as far as I'm aware
>> we diagnosed and fixed all of them within 1-2 business days).
>>
>> If you have the feeling that the phab email workflow makes it hard 
>> for you to jump into reviews, keep track of reviews, or understand 
>> reviews if you're not a phab user, please reply to this thread. You
>> don't need to provide details, "+1", "please
fix", or "doesn't work
>> well for me" are all acceptable replies here - I want to get a 
>> feeling for the magnitude of the problem.
>>
>> Thanks,
>> /Manuel
>>
>
-- 
http://www.nuanti.com
the browser experts

On 7/1/14, 12:28 PM, Alp Toker wrote:
> Specifically the problem I've been seeing is that people using the
website are
> unable to CC mailing list-based developers. As a result I don't get
copied in on
> responses to my review comments, and rarely get any kind of direct mail
with
> threading. You end up having to dig up historic responses in the mailing
list
> archive which becomes tedious.
>
> Often the CC on website reviews will include arbitrary names of people who
have
> website accounts, while excluding the actual code owners and recent
committers
> who you'd expect would be relevant. This leads me to guess that the
website is
> actively blocking the email addresses of LLVM developers from getting added
to
> the CC list unless they open an account on the service.
>
> In fact as far as I can tell, mailing list-based developers are
*completely*
> excluded from the CC list visible on the website. This creates a really
poor
> workflow with responses often getting missed, and the right people not
seeing
> patches (and conversely, it looks like people who aren't really
relevant end up
> getting pressured into reviewing a patch in some area).
+1

I've found this frustrating, especially coupled with the fact that
folks' email
addresses, phab usernames, and svn usernames are not always obviously related to
each other.

It would be nice to enforce (or very strongly suggest) a bijection on phab 
usernames and svn usernames, and then display them in the tool as something 
like: `jsmith2 "John Smith" <john at smith.com>` (for some
hypothetical developer).


Regards,
Jon
>
> Alp.
>
>
>
> On 01/07/2014 14:11, Manuel Klimek wrote:
>> Alp noted that the current setup on how phab reviews land on the list
are not
>> working for him. I'd be curious whether his setup is special, or
whether there
>> are more widespread problems. If this is more widely perceived as a
problem,
>> please speak up, and I'll make sure to prioritize the fixes (note
that this is
>> unrelated to the "lost email" problem - those are always
highest priority and
>> as far as I'm aware we diagnosed and fixed all of them within 1-2
business days).
>>
>> If you have the feeling that the phab email workflow makes it hard for
you to
>> jump into reviews, keep track of reviews, or understand reviews if
you're not
>> a phab user, please reply to this thread. You don't need to provide
details,
>> "+1", "please fix", or "doesn't work well
for me" are all acceptable replies
>> here - I want to get a feeling for the magnitude of the problem.
>>
>> Thanks,
>> /Manuel
>>
>
-- 
Jon Roelofs
jonathan at codesourcery.com
CodeSourcery / Mentor Embedded

On 7/1/14, 12:28 PM, Alp Toker wrote:
> Specifically the problem I've been seeing is that people using the
website are
> unable to CC mailing list-based developers. As a result I don't get
copied in on
> responses to my review comments, and rarely get any kind of direct mail
with
> threading. You end up having to dig up historic responses in the mailing
list
> archive which becomes tedious.
>
> Often the CC on website reviews will include arbitrary names of people who
have
> website accounts, while excluding the actual code owners and recent
committers
> who you'd expect would be relevant. This leads me to guess that the
website is
> actively blocking the email addresses of LLVM developers from getting added
to
> the CC list unless they open an account on the service.
>
> In fact as far as I can tell, mailing list-based developers are
*completely*
> excluded from the CC list visible on the website. This creates a really
poor
> workflow with responses often getting missed, and the right people not
seeing
> patches (and conversely, it looks like people who aren't really
relevant end up
> getting pressured into reviewing a patch in some area).
+1

I've found this frustrating, especially coupled with the fact that
folks' email
addresses, phab usernames, and svn usernames are not always obviously related to
each other.

It would be nice to enforce (or very strongly suggest) a bijection on phab 
usernames and svn usernames, and then display them in the tool as something 
like: `jsmith2 "John Smith" <john at smith.com>` (for some
hypothetical developer).


Regards,
Jon
>
> Alp.
>
>
>
> On 01/07/2014 14:11, Manuel Klimek wrote:
>> Alp noted that the current setup on how phab reviews land on the list
are not
>> working for him. I'd be curious whether his setup is special, or
whether there
>> are more widespread problems. If this is more widely perceived as a
problem,
>> please speak up, and I'll make sure to prioritize the fixes (note
that this is
>> unrelated to the "lost email" problem - those are always
highest priority and
>> as far as I'm aware we diagnosed and fixed all of them within 1-2
business days).
>>
>> If you have the feeling that the phab email workflow makes it hard for
you to
>> jump into reviews, keep track of reviews, or understand reviews if
you're not
>> a phab user, please reply to this thread. You don't need to provide
details,
>> "+1", "please fix", or "doesn't work well
for me" are all acceptable replies
>> here - I want to get a feeling for the magnitude of the problem.
>>
>> Thanks,
>> /Manuel
>>
>
-- 
Jon Roelofs
jonathan at codesourcery.com
CodeSourcery / Mentor Embedded

Manuel Klimek wrote:
> Alp noted that the current setup on how phab reviews land on the list
> are not working for him. I'd be curious whether his setup is special,
or
> whether there are more widespread problems. If this is more widely
> perceived as a problem, please speak up, and I'll make sure to
> prioritize the fixes (note that this is unrelated to the "lost
email"
> problem - those are always highest priority and as far as I'm aware we
> diagnosed and fixed all of them within 1-2 business days).
>
> If you have the feeling that the phab email workflow makes it hard for
> you to jump into reviews, keep track of reviews, or understand reviews
> if you're not a phab user, please reply to this thread. You don't
need
> to provide details, "+1", "please fix", or
"doesn't work well for me"
> are all acceptable replies here - I want to get a feeling for the
> magnitude of the problem.
I really don't like using phab. I don't like the lack attached patch 
files on the mailing list to do a normal review. I don't like going to 
the phab site and trying to add comments and it won't let me ... but 
only sometimes, and for no reason I can see. I hate the patch view in 
phab, the side-by-side view doesn't fit 80 chars of text in my browser 
for either side leading to wrapping.

The emails with comments that other people have made are nice.

I did recently discover that it has a dashboard of reviews addressed to 
me. If that's accurate, that may change my workflow going forward. Today 
I still rely on people to ping their patches.

Nick

On Thu, Jul 3, 2014 at 11:00 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
> I don't like the lack attached patch files on the mailing list to do a
> normal review.

Wait what? The emails I get from phab *have* an attached patch file. That
was a hard requirement when we first set up Phabricator.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140703/a557db9d/attachment.html>