In the read/write case, it's too late once we hit a hook -- we need to be able to decide which cluster node to send the traffic to, and we need to know if it's a read or write request before we make that decision, since there may be a nearby read-only node and a distant read/write node. If we can examine the stream and make a determination that the request is read-only, we can freely pick a nearby read-only node.

It's not entirely phabricator specific, as others probably have similar use cases. I'm literally the only hg dev that knows this is an issue (someone flagged me on twitter a month ago, and it's only a strange inability to sleep tonight that got me deep enough in my stack taht I got to this), but if you go to our ML with a decent problem statement ("we need a hook to let us identify read vs write transactions and do our own ACL checking" sounds like a start?) you'll get more help than just me when I'm unable to sleep. :)

Aha

I'm not a phabricator developer so I'm not sure if supporting mercurial hooks is something they're willing to take upstream. I can join the hg mailing list, but wouldn't this conversation be specific to phabricator and beneficial to be carried out here?

Today, we technically do not need to decode the protocol, but we will need to be able to distinguish between read and write requests to implement cluster read-only nodes (T10883) in the future and need to be able to rewrite the protocol to keep parity with fork-like/virtual-ref features that we plan to support in Git (T10691, T8092).

Ah, okay. We can probably do a lot better with some sort of pre-transaction hook then (that was my gut feeling to begin with). Once you've got an idea what the use case is, maybe we can chat on the hg mailing list?

I am totally on-board with finding something more elegant/maintainable, and would be able to help with some testing and possibly development. I'll try to look over the code in the near future to recall what it was doing, but I believe it's trying to determine whether the mercurial operations are read/write in order to determine whether the actor has necessary permissions to do so. I *believe* that's what it's doing, but it's been close to a year since I was looking through this.

We've got a volunteer (slowly) working on documenting formats, but right now bundle2 is not documented extensively outside of the code and the wiki page already mentioned in this thread.

@durin42 - that would probably be better than parsing the bundle stream but, is that documented anywhere? I tried looking fairly extensively for details on the bundle2 protocol and never came across that. Also, the bundle stream itself isn't what's being parsed/modified, it's the capabilities/outer protocol which I found at least some documentation, which is what the parsing/filtering is based on.

I'm not sure why phabricator is trying to parse the bundle stream (I don't know anything about phabricator), but you should be able to invoke hg with bundle2 disabled by setting experimental.bundle2-advertise to false in an hgrc (or using --config if you're shelling out to hg). Given that it's an experimental flag, it's not something I can promise will exist forever, but it might be a start.

So, yes, the bundle2 thing is definitely orthogonal to this. For now, I'd suggest that if phabricator breaks bundle2, filter the response to the capabilities command and omit bundle2=.* from the response.

Test

The action is indistinguishable from an update from Herald's perspective. Herald runs on the state of the object, not changes to it, so "Changed file content" will just match whatever the diff changes.

Commandeer is a pretty infrequent action so this doesn't matter much, but out of curiosity could I avoid this with a better Herald rule? I guess I can try Changed file content matches regexp '.'.

No one actually seems confused by Paste. Obviously documentation "should" exist, but probably not worth keeping this task around since it has just been idle for more than a year. I suspect we'll write it as soon as someone runs into a problem that needs to be documented.

We haven't seen more interest in this in a long time, and I don't currently plan to make compatibility-focused changes to the stack.

I'm going to merge this into T10448, which discusses limitations and improvements to the mail tags system. This is an example of a cross-cutting tag ("No Application/System Settings") that isn't handled gracefully by the current system, but correcting this and other similar issues is a goal of future work.

T7385 is probably also approximately this.

Commandeer fires Herald rules so stuff like "Require Legalpad Signature" or "Add Reviewers: that intern's wrangler" work.

The first page I found on the internet claims the iPhone 7 will be 1920x1080, so my strategy looks like it's paying off!

I think most of these are just moving into Application configuration over time, which tends to be a bit easier to use. I expect to just continue that trend in the future and this probably isn't worth building separately. I can't recall too much other confusion, and most of these are called <appname>.stuff anyway, which is fairly not-misleading.

This doesn't seem to be doing anything bad in practice. I think you can put the data in the URI if you want too and it works fine. Not clear that this is wrong/invalid enough to completely prevent it, and hasn't moved forward in a year and a half or so.

I think this was effectively resolved by D15775, which improved behavior for multiple cluster frontends.

T7074 may not be this, exactly, but they're close enough for government work.

This seems to be working, I think the last step is replacing the original call in the backup workflow.

We made this a little smarter and a little less prominent (on a non-default tab) and I feel like it's generally in a more reasonable place now. I don't currently expect to make it significantly more complex, and none of the rules proposed here feel like simple, unambiguous improvements to me.

We haven't seen other requests in this vein, exactly, in about two years, and we generally don't expect users to rewrite published commits as a routine workflow.

T6706 was the IP-address specific task.

We haven't seen other requests for this in nearly two years, and it can likely be implemented as an extension, except possibly for the weird cases mentioned above.

I have read https://secure.phabricator.com/phame/post/view/766/write_review_merge_publish_phabricator_review_workflow/ , and it briefly addresses this:

Oh, yeah, maybe need @avivey or @epriestley for those questions. Over my pay grade.

Yes, that works if you decide to collaborate ahead of time. If you decide to "stack" on top of a revision under review, you have to either

Personally, I think it's maybe easier to do review along the way, but you can go the other route where you just both git push to the remote branch, then review run arc diff when you want to merge it into master.

Hmm, but that's not quite the same thing as what I'm asking (I think) -- in my scenario a pair of authors are adding commits to a branch off master (imagine it's a refactor and we're fixing different tests), and the stack of commits is reviewed in one PR. I know I can do this today by just creating a remote branch, collaborating with someone on it, and doing no code review until all the commits land (at which time one of us creates the Differential revision).

I can't easily find an example, think its been about 18 months since we last did a feature branch.

It's sort of like:

Could you point me at an example revision to see how this looks?

We haven't seen other users run into difficulty with this in the last couple of years, so I don't currently plan to change it.

This was filed a while ago, but it's not clear to me why this is useful -- no obvious reason to want to do this springs to mind.

No clue what this was, haven't had issues recently. I think config may have become simpler in the last couple years.

Although I don't think this is necessarily a bad idea, I haven't seen too much of a need for this. Some alternatives:

Ah, thanks! I think that should be relatively easy to fix.

This happens for everyone here in the company on windows chrome with the HRV (Croatian) layout, where the @ symbol is achieved with the right side alt (ALTGR) + "V". Think that registers ALT + CTRL + V.

We have an existing repository_badcommit table which is used to mark commits as "bad", where "bad" approximately means "the repository as a whole works, but the VCS tools can not read or should not read this particular commit for whatever reason". This is rare, but can happen when, e.g., third-party tools are used to write into repositories.

Yes, I have tried to do storage upgrade but it ends up with an error.

Have you tried following the instructions given in the error message?