Yay

Primarily, global rules bypass access control policies. They can also apply different actions.

Great and very helpful change, thank you!

This also happens with integers and epochs:

See also T9367, perhaps, where users apparently accepted changes with serious bugs because of timezone differences between remote teams, although the exact use case behind that task remains murky to me.

I will say that even if it is the same rate of honest carelessness, reducing it even by 0.5% would be very helpful in preventing massive bugs/incidents.

Responding to as much as I can. @eadler @scode feel free to chime in / correct anything below.

I don't currently have any plans to change the behavior described in T10109. At least there, I think the behavior was just confusing when testing rules, and not actually bad. My inclination is to try to find a way to make the behavior more obvious instead of changing it to resolve that confusion.

(I have no idea on this either, the related object task search is definitely fulltext.)

We truncate these summaries at 80 bytes because the underlying storage column is text80, which is only 80 bytes long when represented using binary collation in older MySQL without utf8mb4 support. We can not store more text in the general case without changing this column size.

Thanks for the pointers, I'll try that.

Not in the Phacility cluster today.

I don't want to distract this too much with the "automatic land on accept without author intervention" case. Is there a way to achieve the less controversial "author-triggered land without going to the CLI" case?

There's no real way to do this today, since this isn't a workflow that I think we've seen in professional environments.

In the common case, I'm not arguing against that principle -- "closing a revision" in my example above is a trigger initiated by a human. I'm just not requiring them to switch branches and type arc land. T182 supports this thinking, it seems.

I think these things make sense in an open-source world, where nearly every contribution is from a new source, and having clear, action-based feedback is a net positive. But Phabricator is engineered towards enterprise/teams/trusted circles and these things can burden both parties pretty quickly, thus making us less efficient. Having to classify each piece of feedback becomes an extra thought loop users have to classify in a binary manor, leaving out the possibility for grey conversations to take place.

In T11450#189540, @chad wrote:

Personally my only anecdote is additional controls actually slow me down.

For example, I can describe several different levels of "careless mistakes", after the author lands a revision without addressing the feedback:

Specifically, we feel that the act of landing code (and deployment) should be triggered by a human, because computers are stupid and don't know I have to go out early today and won't be here to fix the new bugs I just committed.

Mostly concerned this method (public tasks on our tracker) is not a scalable way for every company that uses Phabricator manage it's interests.

I'd prefer it if another way were found to track internal issues.

Not really; We normally just land it when it's ready to land; We generally think that the author should be the one to actually land the code, because they know the details, and use "accepted" as a signal from the reviewers to the author that it's OK.

Personally my only anecdote is additional controls actually slow me down. Having to label each comment as an reviewer or address the control as the author is an extra mental step that has to occur before finishing the review. All the times I've had difficulty managing a review as the author come from times I've shipped a very large diff. In those cases just being able to manage "Done" more effectively would have been very appreciated. I don't think the context was important, just the overall list and state.

I could imagine a possible future like:

Are you manually closing the revision as a signal that the revision is "ready to be landed"?`

Yes, that's what I was aiming to achieve. Is there a better way to do this?

Revisions normally "Close" as a result of arc land / git push - by seeing the relevant commit on the repository. T182 talks about moving the "land" action to the web ui, but I believe that's not available in Phacility.
Are you manually closing the revision as a signal that the revision is "ready to be landed"?

Maybe, but we really do a bad job today of allowing inlines to be managed and displayed. I think resolving those UI concerns go a long way to correcting any root issue here. At least, only after cleaning up the UI should we look at whether additional controls are needed. Even just adding some 'remarkup hints' to the inline comment box could resolve this in a lightweight, optional way.

Try editing it from the central administration console:

It perhaps does, I'm still trying to figure all of this out. So a harbormaster http request would notify the bot to deploy, and a herald rule would trigger that request. If we wanted to deploy multiple revisions in one push, we'd just buffer the http requests on the deploybot's end.

Playing devil's advocate, it's not too much of a stretch of the imagination to assume that one could then make it impossible to land things which have unaddressed "issues" vs "comments" on a review. That would be the only concrete advantage I see to explicitly defining these things rather than relying on human parsing.

The "expected" workflow is to have a Harbormaster build do/start the deployment, and have Herald execute that build when a new Commit is landed to the repository... Will that not work for you?

As a reviewer, I cannot distinguish between a comment I expect the author to take action on and a comment I do not expect the author to take any action on.

T8250 I think is what you're looking for @venky

I will add my vote for a small subset/version of this, ie. hoist inline comments and their "done" state into an easily perusable checklist.

The other thing that was confusing was that "Config" didn't actually have all the settings -- some settings are in Config, some user settings are in my "profile", some other settings are in individual apps. It would help to consolidate these into one directory.

Is it ok for me to retitle this feature request?

per @em

Sure, but in my case they were just freshly created, so the situation was different.

This is a duplicate task, but is open for internal tracking reasons.

I'll dig around the tasks, but in short we feel this significantly changes the concept of "Code Review" to being "just fix these things" and not about "why was this code written".

Individually going into a panel to assess frequently seems like quite a workload.

If cleanup is the goal I'd probably opt to build something directly into ApplicationSearch.

In general we don't expect people to need to actively archive panels

Ha! I didn't notice it was a duplicate. I'll closes this and point to T6018. Thanks!

Panels can be embedded anywhere in Remarkup, not just Dashboards or other Panels. In general we don't expect people to need to actively archive panels. It'd be better for us to understand why you need to do this.

See also T6018

Sorry, what is not clear?

Sorry, I'm not sure what I'm missing here.

We even search the description.

How can I reproduce this?

Can you check out Contributing Feature Requests please and make sure to always state a root problem your having using Phabricator when filing a feature request? Thanks!

Can you check out Contributing Feature Requests please and make sure to always state a root problem your having using Phabricator when filing a feature request? Thanks!

Thanks, the patch definitely helps a lot in my case!

@epriestley Is there a pathway for reading (not writing) edges which would be open to community contribution? Having arc patch Tnnnn functionality, pulling all the revisions attached to a particular task, would be really attractive for us as we tend to have a larger number of revisions per task.

Cool. Well that clears up one concern right away. :-D

The comment area takes remarkup. I think every textarea in Phabricator
takes remarkup, regardless of if it has the hint bar or not.

This it exciting, @chad and mostly I like the look of it.

I was experiencing this problem too and it works in current stable for me now.

There's a separate task floating around for making Phirction changes reviewable. I think that should be separate from this task to keep things lightweight.

Changes for T9365 might cause some API changes (Moving lint/unit excuses from diff properties to harbormaster.sendmessage), so might be worth doing them around the same time.

(Just so no one thinks too highly of me, I'll underscore that I very briefly peaked at #11. I think I had slid to about #35,000 by the end of the lightning round and "-∞ dumpster garbage rank" by the end of the competition.)

No! I hadn't noticed it, sorry. I've had a look now and will endeavour to give some specific comments over there later. I don't think the 'addressing comments in multiple updates' will be a problem, though.

Did you check out the mock in Pholio?

It's vaguely possible we could use a bunch of Javascript to try to capture the path that an edit takes and then submit it alongside the new text to track inlines, like Docs/Word do, but without implementing WYSIWYG. I suspect this would still be very complicated.