Page MenuHomePhabricator

Identify features Gerrit users would miss in Phabricator
Closed, ResolvedPublic

Description

Which features would Gerrit users really miss? If you are not sure whether feature X is supported, under development, roadmapped... just ask. Let's create new subtasks for features considered blockers.

Details

Reference
fl47
TitleReferenceAuthorSource BranchDest Branch
Draft: Do not inherit subscribers and project tags when creating a subtaskrepos/phabricator/phabricator!56aklapperT239378subtaskRmTagsCCwmf/stable
Allow to change email address (from MediaWiki SUL) at account registrationrepos/phabricator/phabricator!54aklapperT230590customEmailAddresswmf/stable
Preset MediaWiki SUL email address via OAuth at account registrationrepos/phabricator/extensions!35aklapperT230590accountRegEmailwmf/stable
Draft: bullseye-mariadb: Initial versionrepos/releng/dev-images!38kharlanT238925main
Customize query in GitLab

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusSubtypeAssignedTask
DuplicateNone
ResolvedQgil
DeclinedNone
ResolvedQgil
Resolved mmodell
ResolvedQgil
InvalidQgil
DeclinedNone
Resolved mmodell
InvalidQChris
ResolvedNone
DeclinedNone
Resolvedyuvipanda
DeclinedNone
DeclinedNone
DeclinedNone
DeclinedNone
DeclinedNone
DeclinedNone
Declinedgreg
DeclinedNone
DuplicateNone
DeclinedNone
DeclinedNone

Event Timeline

flimport raised the priority of this task from to High.Sep 12 2014, 1:21 AM
flimport set Reference to fl47.

milimetric wrote on 2014-04-08 23:55:59 (UTC)

  • Keyboard shortcuts (next/previous comment, next/previous diff block, next/previous file)
  • image diffs
  • having learned an insanely complicated tool / workflow and being able to explain it to newcomers :P

(that third one's just me getting in the Phab spirit :))

csteipp wrote on 2014-04-09 18:58:51 (UTC)

Are there no inline comments? That's going to make life hard.

scfc wrote on 2014-04-10 14:22:55 (UTC)

@csteipp: I saw inline comments at https://secure.phabricator.com/D212, so they seem to be possible.

However, I cannot find open changes in this site. Could someone provide a link or upload a bunch of nonsense changes?

milimetric wrote on 2014-04-10 19:55:59 (UTC)

@scfc, that diff at the link you shared is buried very deep at the bottom of a ton of comments. Is there a way to have it show up at the top? The layout seems impossible to work with otherwise.

scfc wrote on 2014-04-15 17:51:53 (UTC)

@Milimetric: Eureka! At http://fab.wmflabs.org/rMW85d4e39ff096d05386fd2f333cd6acea66ad6f90, I double-clicked on the line number, and I was able to add an inline-comment (and I suppose that is the review interface?).

What concerns me a bit: If we consider moving to Phabricator, and this is the primary discussion page for all things Gerrit about that, I'd assume immediate responses (modulo time zones and weekends) to such basic questions. Throwing away a working environment for one which apparently few people are familiar with sounds very risky to me.

milimetric wrote on 2014-04-15 19:51:30 (UTC)

@scfc, I agree with your point that we should become more familiar with the tool. I think that's what we're trying to do with having some of us prototype it for our daily work. I volunteered and the Growth team is showing interest as well.

On my other point, my problem was that non-inline comments cluttered up the space around the diff itself. But it looks like each diff can be browsed standalone, so it's not a problem:

http://fab.wmflabs.org/diffusion/MW/change/master/includes/parser/Parser.php;85d4e39ff096d05386fd2f333cd6acea66ad6f90

qgil wrote on 2014-04-23 20:47:24 (UTC)

Someone asks in the Phabricator RfC FAQ: "Gerrit - Phabricator is not at all opinionated about how to structure one's history. Gerrit forces us to rebase and very cleanly package these neat little changes together. Has this been considered?"

mattflaschen wrote on 2014-04-23 21:45:15 (UTC)

Can someone who's used Phabricator for real work shed some light on this?

A single Differential entry can have multiple commits (https://secure.phabricator.com/D7936) if I'm understanding "Local Commits" right. Is it possible to see the individual commits, or only the overall diff?

How does amending in response to code review work? Can you only add more commits? Can you amend, can you replace all the commits (I think GitHub is totally flexible in this regard).

I don't think reviewing more than one commit per changeset is necessarily bad (it's still possible to make neat commits even if there's more than one per changeset), but it's certainly quite different (and people might *not* make neat commits).

milimetric wrote on 2014-04-23 22:01:25 (UTC)

@mattflaschen, as it happens I was just trying this out for myself. So this Differential is a combination of two Gerrit changes: http://fab.wmflabs.org/D1

As far as I can tell from talking to Chase (from DevianArt, has Phab experience), there's no way to see the changes made by individual commits in a Differential. For that, it seems like you should be able to use dependencies between two differentials. But when I tried to do that (http://fab.wmflabs.org/D2 being the first commit), I wasn't able to add the second commit and convince Phabricator that it was not actually an update on D1. In other words, I was trying to make D3. Maybe I have to abandon / delete D1 first, I'm not familiar enough yet.

Also, back to your question about re-submitting D1. You can do *anything* you want. Because basically, when you submit D1, you do:

arc diff master

... Do some work including rebasing, amending, etc. etc. ...

arc diff master

And so your history, however you choose to lay it out, will be dumped altogether into the Differential. Now, Evan Priestley seems hyper-active on Stack Overflow since every google leads to his answers [1,2,3] so it may be worthwhile to ask stuff there after we play with it a bit.

[1] http://stackoverflow.com/questions/20756320/how-to-prevent-phabricator-from-eating-my-commit-history
[2] http://stackoverflow.com/questions/21631681/commits-that-have-gone-through-differential-show-up-in-phabricator-audit-system
[3] http://stackoverflow.com/questions/19819252/can-you-arc-diff-against-a-particular-git-branch

qgil wrote on 2014-04-24 21:53:08 (UTC)

Interesting collateral comment from @epriestley on IRC, after replying some of the questions our users have asked in tasks or the RfC:

<epriestley> Generally, I think everyone is pretty skeptical of the Phabricator model at first because it's unfamiliar, and a bit dissimilar to Gerrit / GitHub, where workflows are tied more closely to branches and checkpoint commits are common.

<epriestley> But once they get used to it everyone seems pretty happy with it (or possibly we have a massive selection bias because people who don't get used to it just use something else instead of filing feature requests).
<epriestley> Our current view is that these featuers are somewhat important to make it easier and smoother to onboard new users coming from GitHub/Gerrit backgrounds, but not especially valuable on their own.

I can't speak for Gerrit, but we are seeing a similar pattern with Bugzilla users. Let's see.

Rush wrote on 2014-04-25 16:39:42 (UTC)

@mattflaschen

@millimetric

Let's not make the mistake of assuming I am authoritative on anything :D Conversely, I have used phabricator on a day-to-day basis previously, though it was -- in some respects-- a simpler setup.

But onward!

Can someone who's used Phabricator for real work shed some light on this?

A single Differential entry can have multiple commits (https://secure.phabricator.com/D7936) 
if I'm understanding "Local Commits" right. Is it possible to see the individual commits, or only the overall diff?

How does amending in response to code review work? Can you only add more commits? 
Can you amend, can you replace all the commits (I think GitHub is totally flexible in this regard).

I don't think reviewing more than one commit per changeset is necessarily bad (it's still possible to make neat commits even if there's more than one per changeset),
 but it's certainly quite different (and people might *not* make neat commits).

iA single differential can have multiple local commits. It is possible to see the hashes involved in the differential, with the UI I don't think there is a way to inspect how the commits this person -- who submitted for review -- were organized. If you want to know it is possible to do an 'arc land DXXX' and then you get their work in it's entirety I believe. When Dan and I spoke I really wasn't sure on this point, just because I had never tried before (breaking down their commits). What becomes the reviewable content in a differential is really anything you can 'git diff'. In my experience how someone organizes their work commit wise tends to be a really personal thing, but I do understand that gerrit making every commit a review object this is very different thinking.

Because the _native_ behavior post diff and accept of 'arc land' is to squash a differential in a single commit the idea that work has to be structured a certain way within their commits by the submitter has not come up for me before. It is possible to retain history when you merge into master, but in the past it was a rare occurance. The truth is that with both gerrit and Phab you can do a one-commit-per-review item workflow. There is nothing that stops you, but in the instance of Phab if you need to change x to y you can do it in a new commit and the history shows up in the review process. With Gerrit you have to amend it and that history is lost. This is just my view of it. I think in that regard the within-a-review workflow is better in Phab, as it's more organic to the git process. That's very subjective though.

If you wanted to you could submit foo change, get a comment, and swap x for y, and amend and then arc diff again and it would see your change exactly as doing a git diff would. It wouldn't know the difference. With differential however you could also add a commit that changes x to y and then when the next person comes in and says 'NO WAY Y SHOULD BE X' it is simply a revert and re-'arc diff'. With all comments and history etc kept for posterity.

As far as I can tell from talking to Chase (from DevianArt, has Phab experience), there's no way to see the changes made by individual commits in a Differential. 
For that, it seems like you should be able to use dependencies between two differentials. 
But when I tried to do that (http://fab.wmflabs.org/D2 being the first commit), 
I wasn't able to add the second commit and convince Phabricator that it was not actually an update on D1.
 In other words, I was trying to make D3. Maybe I have to abandon / delete D1 first, I'm not familiar enough yet.

Also, back to your question about re-submitting D1. You can do *anything* you want. Because basically, when you submit D1, you do:

arc diff master

... Do some work including rebasing, amending, etc. etc. ...

arc diff master

And so your history, however you choose to lay it out, will be dumped altogether into the Differential. 
Now, Evan Priestley seems hyper-active on Stack Overflow since every google leads to his answers [1,2,3] 
so it may be worthwhile to ask stuff there after we play with it a bit.

On the subject of a new diff, I will outline what I think you are doing :)

master
 -> mybranch
    -> commit1
    -> commit2

In this case post-commit1 you could do 'arc diff master' and it would make a differential with what you see if you do a 'git diff master'. But if you make commit2 and you do 'arc diff master' it will assume you want to update the existing differential. That is because the content of a differential is meta to the content of individual commits. (maybe bad wording?). Essentially, I would do 'arc diff master' post commit1, and if I want commit2 to be a new commit I would do 'arc diff commit2'. There is a --new-diff flag or something for cases where it is ambigious, but I believe in this case it would now have created two diffs. Typically for me I have organized my work as one branch == one differential, simply because it's easier for me to think that way. In that case 'branch1' could 'arc diff master' and branch2 could 'arc diff branch1'. I know that does create two differentials, I used that workflow numerous times. Unrolling that can be a pain if you try to merge branch2 into master directly or something, simply because...git. But in general, anything you can 'git diff' you can differential. It turns out there are tricks and such I haven't used notated here https://secure.phabricator.com/book/phabricator/article/arcanist_commit_ranges/, but the general idea is simpler than I think that makes it seem.

Rush wrote on 2014-04-25 16:55:18 (UTC)

I noticed I started thinking of my code as reviewable chunks in terms of differential instead of commits. As in, a single commit could be a reviewable chunk, but a reviewable chunk does not have to be a single commit.

maybe helpful?

Rush wrote on 2014-04-25 16:58:56 (UTC)

I just noticed epriestley is copied on this ticket...hope I haven't misrepresented things insanely! Thanks for all the effort man.

Rush wrote on 2014-04-25 17:20:13 (UTC)

correction for above and then I'm done spamming this ticket...

...instead of..if I want commit2 to be a new commit I would do 'arc diff commit2'.

...if I want commit2 to be a new commit I would do 'arc diff commit1'....

epriestley wrote on 2014-04-25 17:45:10 (UTC)

I think that's all pretty accurate. Some supplemental notes / flavor / context:

Keyboard shortcuts (next/previous comment, next/previous diff block, next/previous file)

I think we have these -- press "?" to show keyboard shortcuts:

{F237}

Here are the shortcuts we currently support:

{F239}

If you're missing anything, let us know.

image diffs

We support these too, although they aren't quite as fancy as GitHub's (I'm not sure about Gerrit's). Basically, just shows the old/new images but doesn't have fancy "highlight the changes" sorts of features. There's no technical reason we can't build these, but there hasn't been a ton of demand for it.

Are there no inline comments? That's going to make life hard.

Click a line number, or click-and-drag a range of line numbers to comment on a range of lines.

(This is something we need to do a better job of explaining to new users, but it's sort of hard to fit it in the UI in a way that doesn't impact experienced users, and we've been very hesitant to add, e.g., a "clippy" feature.)

I wasn't able to add the second commit and convince Phabricator that it was not actually an update on D1

You should be able to use arc which to understand what arc diff will do and why. You can use arc diff --update and arc diff --create to override arc's guesses about what you mean.

Generally, arc diff guesses two things:

  • Which range of commits do you want to send for review?
  • Do you want to create a new revision, or update an existing one?

You can use arc diff X to tell it the answer to the first question, explicitly. You can use --create or --update to tell it the answer to the second question, explicitly. If you don't provide these flags, it guesses. You can use arc which to understand what its guesses are and why, and arc set-config base ... to tell it what rules it should use to make guesses, according to the document that @Rush linked.

Overall, this is more complicated than we'd like it to be and we'd generally like to make it easier than it is. However, previous (easier, less powerful) versions got a lot of negative feedback for being insufficiently capable or expressive, so we gradually ramped the power up until we ended up here, where experienced users seem to be satisfied -- we just need to make it easier for new users to get comfortable and gain experience.

Is it possible to see the individual commits, or only the overall diff?

Only the overall diff. It would be nice to show the individual commits too (see https://secure.phabricator.com/T1508) but there's been very little demand for it so we haven't built it yet.

How does amending in response to code review work? Can you only add more commits? Can you amend, can you replace all the commits (I think GitHub is totally flexible in this regard).

As others have noted, there are no restrictions. You can add commits, amend commits, replace all the commits, or use commits from an entirely different repository (or even a different version control system).

A narrative here is that a code review is essentially discussing the solution to problem ("fix bug X", "add feature Y"), and during discussion of the change it's reasonable to decide to make a fix somewhere else instead ("this is good, but now that we understand what was causing X, we should really fix it in library Z"). You can update a revision with changes from a completely different source (like "library Z"), and this is reasonable and recommended if that's a better way to accomplish the goal. All the discussion stays in one place and can be referenced later, so the decision to fix library Z can be understood in context. This doesn't happen too often, but is the most general case of the utility of this approach.

I don't think reviewing more than one commit per changeset is necessarily bad (it's still possible to make neat commits even if there's more than one per changeset), but it's certainly quite different (and people might *not* make neat commits).

The tool isn't very opinionated on this. Lots of rambling below, which you can safely skip if you want:

The workflow we use in the upstream is: one commit per review, amend locally to update, squash-merge to master after review (with "arc land"). This makes master linear, gives every commit a rich/informative commit message, and means every commit in the remote has been reviewed, and every commit on master is "good". However, it also means that we willingly discard some local commit information.

{F235}

Other workflows balance these things differently, and Phabricator is generally not opinionated about what approach you want to take (although you may need to make some configuration adjustments to tell it what your preferences are).

Our subjective/anecdotal/opinionated take on this is that of these workflow properties, local history is near-zero or slightly-below-zero value, and all the other properties are somewhere between "nice to have" and "really quite valuable", so it's an easy choice to throw away local history and embrace an amended, linear master comprise only of reviewed changes. However, this view isn't wildly popular (a lot of projects consider it valuable to publish "checkpoint" commits and/or make local history immutable or restrict its mutability, and Mercurial didn't even permit local mutability at all in the core until fairly recently), so we aren't especially evangelical about it.

The approach we use is very similar to what Facebook does, and a lot of the advantages are more evident at that scale, but less evident at the scale of Mediawiki or Phabricator.

To give a concrete example, a lot of the popular build tools (like Travis CI, I think) just continually build HEAD of some branch, say master. This is perfectly reasonable at small scales, since each new build usually has a very small number of different changes in it (probably only 1 a lot of the time) and it's easy to figure out what broke when you get a build failure.

At Facebook's scale, the duration of the tests and the velocity of the repository mean that a build failure would routinely implicate dozens of changes, which would take an unreasonably long time to sift through by hand and require alerting dozens of different authors/reviewers that their change might have caused a build failure. Facebook has to build every commit so it can know which change broke things. This means every commit in "master" must be buildable, and build failures must be unexpected. You can accomplish this by forcing merge commits, or through other metadata, but these practices aren't widely used because most repositories haven't hit the scale where they have this problem.

nuria wrote on 2014-04-29 10:30:09 (UTC)

Are we open to consider just moving to Phabricator just for tasks and keep using Gerrit as a CR tool?

qgil wrote on 2014-04-29 13:25:42 (UTC)

@Nuria, which are your blockers to add Phabricator code review tools to the mix? The default option is to take the whole Phabricator except Phriction (their wiki engine, because we are developing a dedicated wiki engine).

We started this RfC because of the split of project management tools between Bugzilla, Trello, Mingle, and Scrumbugz. The fact that alternative we found also supports code review is a collateral effect, so to say. If we want to go for Phabricator keeping Gerrit we can of course do so, but the reasons to do this should outweigh the cost of keeping an extra tool (maintaining it, mastering it and onboarding newcomers to it) instead of the ones that Phabricator supports.

I'm not a software developer, but so far it looks to me that the effort of keeping Gerrit in the long run would be higher than the effort of adapting Phabricator to our code review needs, and also adapting ourselves a bit to them (just like we adapted to some gerritisms despite of the initial resistance).

milimetric wrote on 2014-04-29 15:16:49 (UTC)

  • The code review in Phabricator is missing the number of changed lines (e.g. "+231, -105") for each file and for the whole change. We are using this information very heavily in Gerrit.

This is true but things like this are very small issues that we can contribute ourselves to upstream phabricator. The raw diff is available, here's one example:

https://secure.phabricator.com/file/data/otxr4fryq2wmu5om2oe6/PHID-FILE-tg4qgdvbphx4othfjuta/D7936.vson.id17957.whitespaceignore-all.diff

So we could easily add the +/- summary.

  • It also misses the possibility to vote (+1, -2 and such) as well as the current quick overview of all the votes.

This is tracked in a task: http://fab.wmflabs.org/T229

  • I can not invite people to review a patch (or I'm unable to find that feature).

This actually works quite nicely in Phabricator. When you submit your patch for review, you do "arc diff". This pops up a commit message with several sections, and one of them is a "reviewers" section. You can type in people's names there and arc actually checks that those are valid users before submitting.

  • I don't have (or can't find) a list of personal reviews and patches other people want me to review.

This is probably because nobody added you to a diff yet :) I added you to one of my test ones, and you can find a "Revisions Waiting On You" section on the home page: http://fab.wmflabs.org/

For me, all these are blockers. See my full post on mediawiki.org.

It would be useful if you could add your issues here as dependent tasks. There's a "Create Subtask" link on the top right on T47 (this issue). That way we can eliminate duplicates and track the blockers better.

Thanks for your input.

thiemowmde wrote on 2014-04-29 16:31:07 (UTC)

The question asked is not if we can reimplement everything Gerrit does. We sure can. We are developers. The question is whats currently missing.

you do "arc diff".

You are talking about some kind of command line tool, are you? This currently is a feature in Gerrit and should be a feature in Phabricator. We shouldn't dig the trench between capricious volunteers and our set of tools deeper than it currently is. This Phabricator move should go in the other direction.

I see I got added as an "Auditor" to some commits now but I can neither find a way to remove myself from a commit if I don't want to review it nor can I add other people. I still consider this a blocker.

thiemowmde wrote on 2014-04-29 16:33:13 (UTC)

  • The code review in Phabricator is missing the number of changed lines (e.g. "+231, -105") for each file and for the whole change. We are using this information very heavily in Gerrit.
  • It also misses the possibility to vote (+1, -2 and such) as well as the current quick overview of all the votes.
  • I can not invite people to review a patch (or I'm unable to find that feature).
  • I don't have (or can't find) a list of personal reviews and patches other people want me to review.

For me, all these are blockers. See my full post on mediawiki.org.

Rush wrote on 2014-04-29 16:38:10 (UTC)

@thiemowmde

Hi! We don't know each other I don't think? But I have lived with phabricator in the past.

Adding reviewers is typically a at-time-of-diff thing from the cli. This has been a nice workflow for me in the past :)

But...it can be done through the UI :)

phab.png (26×149 px, 3 KB)

{F458}

profile-CWzEEdh0.jpeg (50×50 px, 1 KB)

{F460}

epriestley wrote on 2014-04-29 16:58:12 (UTC)

You can also use the "Add Reviewers" action, available in the comment area:

{F466}

nuria wrote on 2014-04-29 17:01:07 (UTC)

@Qgil I was seeing the feature gap between fabricator CRs and Gerrit and it seemed pretty big so I was thinking maybe we could do away w/o the CR side of the tool. For example the voting seems a crucial feature of wikimedia's CR process.

thiemowmde wrote on 2014-04-29 17:23:05 (UTC)

@Rush @epriestley I don't see any of these interfaces. It seems this feature can exclusively be used by the uploader of a patch. If this is true then we may have found a better description for my problem, but it will still be the same blocker. I can't even remove myself from a review if I'm not interested and don't want the emails but still want the ones I'm interested in.

Adding reviewers is typically a at-time-of-diff thing from the cli.

Sure, I agree it is nice to have the possibility to do this earlier when submitting a patch. But as things currently are this is removing a major social aspect from our tool chain. One of the few that actually work in Gerrit. In my opinion it's an indisputable must-have to add and remove reviewers via the UI. Every time. Everywhere. No matter if I own a patch or project.

epriestley wrote on 2014-04-29 17:35:57 (UTC)

That sounds like a bug. Can you click this link:

http://fab.wmflabs.org/D7

...and scroll down to the bottom of the page, and tell me what options are available in the "Action" dropdown for you?

qgil wrote on 2014-04-29 18:01:31 (UTC)

@thiemowmde please have a bit of discipline discussing topics in the tasks that correspond. This is the best way to discuss the problems you are finding and eventually file them as bugs or requests upstream.

@Nuria, imho we are still measuring this "feature gap", whether it is an actual gap or just a different flavor, and what Wikimedia or Phabricator upstream could do to create what is missing. This is why it is useful to discuss further the potential blockers where they are being discussed, e.g. T229: Catch up on CX work (thank you @Milimetric for the pointers!).

Rush wrote on 2014-04-29 18:27:29 (UTC)

@thiemowmde

hmmm...I am not the uploader of http://fab.wmflabs.org/D9 but I see the 'edit revision' option in the upper right?

I see it for another unrelated diff here too http://fab.wmflabs.org/D3

Maybe you have to be a member of the project or something? It's definitely not only for the uploader and does phab does support adding reviewers in the UI post upload.

thiemowmde wrote on 2014-04-29 21:57:32 (UTC)

please have a bit of discipline discussing topics in the tasks that correspond.

@Qgil Sorry, what's wrong? Aren't we discussing "features Gerrit users would miss in Phabricator"? Even if it turns out the features I'm missing are right under my nose, the sole fact that I miss them tells me that there must be something wrong with the UX design in Phabricator. And it's not that I'm used to Gerrit. I started using it some weeks ago. Gerrit's UX design is clearly flawed. Would I miss Gerrit? I find more and more reasons to say "yes, I would".

scroll down to the bottom of the page

@epriestley Why should I scroll down to an "add comments" form to do something different than adding a comment? Does Phabricator bury major workflow features in a comment form? An option field in a form that switches the form? Wow, this UX makes me feel like I started using computers yesterday. That's a disaster.

I see the 'edit revision' option

@Rush Whats a revision in the context of a code review? Aren't we reviewing commits any more? I tried to click "edit" multiple times and gave up.

mattflaschen wrote on 2014-04-29 22:36:46 (UTC)

nuria:

Are we open to consider just moving to Phabricator just for tasks and keep using Gerrit as a CR tool?

If we're just switching from Bugzilla to Maniphest, I wouldn't consider it worth it at all. If we're turning off Bugzilla and at least some others (e.g. Trello, Mingle), it would provide an incremental benefit, but still might not be worth it.

Unified issue tracking and code review is part of what I find compelling.

thiemowmde:

@Qgil Sorry, what's wrong? Aren't we discussing "features Gerrit users would miss in Phabricator"?

Yes, and we're trying to do so in a structured way, with issues/tasks for each specific problem.

Even if it turns out the features I'm missing are right under my nose, the sole fact that I miss them tells me that there must be something wrong with the UX design in Phabricator.

No, it doesn't. It tells us there might be something wrong with the UX. There's always going to be a certain amount of learning involved in a new UI. For example, it took me a while to notice and grasp the utility of the star in Gerrit. It's not because it's buried (it's somewhat central, though not big), and other people may have picked it up right away. The point is different people experience UI's differently.

@epriestley Why should I scroll down to an "add comments" form to do something different than adding a comment?

I assume by this response that you do see "Add Reviewers".

It does add a comment (if you type one in), but it also allows you to do other things at the same time. I actually think this is useful for common workflows. "Add John Smith as a reviewer and explain what you want him to look at", "Lower priority and explain why", etc.

Does Phabricator bury major workflow features in a comment form?

It's not buried, and this is only one way to add reviewers. You can also do this by editing the differential. It sounds like you may be mixing up the code audit tool and the reviewing tool. To add a reviewer without using the comment interface, go to {D8} then click "Edit Revision", taking you to http://fab.wmflabs.org/differential/revision/edit/8/ . You will then see a reviewers field.

Note, Gerrit has the same thing (integrated comments and actions). Note how you use the same field for +2/Accept (which is an action, not really a pure comment) and actual comments.

An option field in a form that switches the form?

This is extremely common. Just one example, shopping sites where you can click "My shipping address is the same as my billing address", and it hides the shipping address fields.

When you switch it here, you do not lose the comment text.

qgil wrote on 2014-04-29 22:40:25 (UTC)

@thiemowmde, with all respect, I think you should assume good faith and give an opportunity to (anything) before qualifying it as a disaster. Maybe you are seeing it upside down? If you want to comment, you can carry an extra action in the same go. If you don't want to comment, just "Edit revision". Find these links at e.g. D8.

PS: All the "Depends On" tasks are related to "Identify features Gerrit users would miss in Phabricator". Creating subtasks for e.g. "Show added/deleted characters in diffs" would help the discussion and your case indeed.

thiemowmde wrote on 2014-04-29 23:11:17 (UTC)

There's always going to be a certain amount of learning

@mattflaschen Sorry, but no. For the sake of simplicity lets assume I have a background in UX design. You don't switch from form A to form B by choosing an option in form A. If I want to comment I use a comment form. I don't use a comment form to send invitations. If I want to invite somebody to look at a patch I do not want to change the patch. Task A: Edit submission. Task B: Invite somebody to look at submission. Two different things. Gerrit clearly separates them (and Bugzilla does not, I'm aware of that, but this is the "Gerrit users would miss" task).

you should assume good faith and give an opportunity to (anything)

@Qgil How do you know I didn't do that? I only spend my time on things that are worth it. Think about this, please. I'm expressing my conclusions here, maybe without listing what went into them. Else my posts would be ten times that long.

qgil wrote on 2014-05-01 19:07:37 (UTC)

Please have a look to Proposing to put Gerrit migration aside. I have also created a project to track exclusively code review specific tasks.

qgil wrote on 2014-05-06 17:16:37 (UTC)

The task of identification of missing features is completed. If you find new ones please add them as subtasks.

aklapper wrote on 2014-09-03 14:58:43 (UTC)

I've removed again those "blocking tasks" that are not related to actual Gerrit / code review issues but rather generic.

I miss the code-signing feature from git in arcanist and the requirement from gerrit to accept signed commits only (maybe something for your list, maybe not)

Yes the bug title is "Identify features" which is probably done unless you miss something in the "Blocked By" list. Then you should add it there, but after that this task is resolved again, so no need to reopen it just to add a blocking task.

Now a question that I asked myself is: Which is the task that all the features that should be implemented before we switch to differential block on? That is probably T18. Perhaps it clutters that task if we were to directly add them there, so instead, I'll reopen this and rename it to "implement missing necessary features of Differential" if nobody disagress. Same with T597.