Restore parallel future resolution

Does that mean the ConduitClientFuture doesn't begin execution until resolve() is called or some other internal mechanism triggered by iterating a FutureIterator?

The desired/intended approach is to implement ArcanistMercurialCommitSymbolCommitHardpointQuery, following ArcanistGitCommitSymbolCommitHardpointQuery. Alas, this is a significantly more complicated change (but might make life easier down the road).

I did have a suspicion about a missing hardpoint query for Mercurial but the only one I was tracing through was HARDPOINT_REVISIONREFS which I think maps to (or, I mapped to) ArcanistMercurialWorkingCopyRevisionHardpointQuery which looks to be appropriately implemented and mostly matches the Git variant. Though I don't have a solid mental model of "hardpoints".

Ah, that's helpful. I found a repro, it's specific to the behavior of the ImmediateFuture pathway for non-clustered resolution of Conduit "calls". This should force it from any configuration/state, I think:

When implemented correctly, a Future should not throw until it is resolved (at least, for any kind of "result of trying to resolve the future" error), so the Iterator loop should never throw. Some older Futures were less careful about this, but I believe major futures were pulled in line after T11968. From cursory inspection, I think the behavior of ConduitClientFuture is correct, and I can't immediately get the iterator loop to throw an exception (see T13666#256232 for my test case and observed behavior).

Does that mean the ConduitClientFuture doesn't begin execution until resolve() is called or some other internal mechanism triggered by iterating a FutureIterator? I will definitely add this back in.

I'm able to reproduce this but I'll investigate further

The desired/intended approach is to implement ArcanistMercurialCommitSymbolCommitHardpointQuery, following ArcanistGitCommitSymbolCommitHardpointQuery. Alas, this is a significantly more complicated change (but might make life easier down the road).

I think I did conflate this partially - namely that this isn't directly caused by the issue discussed in T13365. Where this issue came up for me is while testing out D21715, I removed the head commit from the on-disk repo which caused the landing page to not render properly:

Thanks!

This construction:

I'll have some more detail in D21717 in a minute, but I can't immediately reproduce this, I think? If I intentionally break diffusion.historyquery like this:

In D21715#276590, @cspeckmim wrote:

I attempted to load the repository page and saw an exception due to a missing commit.

I don't think this is expected behavior but I'll make a new task for it since it's outside the scope of this change.

Created T13666

I attempted to load the repository page and saw an exception due to a missing commit.

I don't think this is expected behavior but I'll make a new task for it since it's outside the scope of this change.

In Mercurial, I'm not exactly sure how wide this window is. Since you normally can't strip commits in the remote by pushing (AFAIK?)

This is true, though it does change a bit if we enable evolve server-side. Then when commits/branches are pruned when pushing/pulling those obsolete markers will also be transferred, allowing similar disappearing commits (retrievable via —hidden). However I believe by default mercurial does not allow pruning public changesets (would require forcibly changing phases on client/server, or running a server/branch in non-publishing mode). Supporting evolve/topics server-side is probably large enough that it would be its own big project. So I think this assumption holds up well.

(D21715 is still a draft, but I left a couple of more specific comments there.)

Does this refer to when multiple dependent commits are pushed and then stripped from the on-disk state?

Another caveat here is that if you destroy commits directly in Phabricator's working copy in certain states, it's possible it won't be able to trace ancestry to find all the commits you destroyed in order to mark them unreachable. However, this window is narrow. For Git, you can bin/repository mark-reachable to rebuild reachability if you know you've done a bunch of dangerous mutations to the on-disk state.

Thank you for figuring out the appropriate change!

Thanks!

I think this is probably the cleanest fix, but I only tested event imports (which now appear to work). If you want to test Maniphest with subtypes configured (to make sure it doesn't break) and send me a revision with this change, I'll review it. Otherwise, I'll do that testing when I get a chance and land this if nothing crops up.

I found this channel incredibly helpful for repairing/replacing components on boards:

You can render remarkup using this rule without having a meaningful relative base URI, e.g. on some other object type or via remarkup.process in the API. This isn't necessarily a meaningful operation and the result may not be useful, but remarkup generally tries not to fail loudly: if you copy/paste a block of text from somewhere that happens to have some substrings which aren't valid, the desired behavior is generally for your text to be processed in some best-guess-at-sensible way -- and usually emitted unmodified (see also discussion in D21713).

This also fixes root document preview in lighttpd, which seems to have mandatory slash merging.

• For now, just return the literal input if we fail to evaluate an expression.

When intent is ambiguous (the user might or might not be trying to invoke a Remarkup rule), I try to make the output of an "invalid" input exactly the same as the input, so (for example) copy/pasting text into Phabricator doesn't mangle it into a big blob of nonsense just because you happened to have some magic words in there.

I think _() is the pht() of Gnu "gettext". Modern programers may mostly be more familiar with Python than with gettext, of course.

Oh yea that's right. A long time ago I worked on a python web application that used this was probably confusing it.

I'll plan to get this on a test instance and get some screenshots

Try escaping the ${{{

I suspect escaping things in PHP will be pretty rare and that the "collides with PHP strings" downside will be very small.

You can either escape ${{{strings.x.y}}} as \${{{strings.x.y}}} or suggest a different syntax for the "eval" rule --- I'm not married to ${{{...}}}.

I think this format makes sense to me. Nothing else really comes to mind. The squiggles are keeping consistency with other remarkup rules like figlet and cowsay. I think I've seen $ used in other places (and languages) with some relation to string substitution. I think python convention uses _, so another possibility could be _{{{...}}}.

Ah, yeah, the build issue is that ${...} in a PHP double-quoted string is semantic, so PHP is trying to do something with ${{{...}}} and failing with a syntax exception during parsing. You can either escape ${{{strings.x.y}}} as \${{{strings.x.y}}} or suggest a different syntax for the "eval" rule --- I'm not married to ${{{...}}}.

That build failure may be related to recent changes to T13072, or something else server-side -- I don't expect build failures to look like that in Harbormaster. Let me see if I can reproduce it locally.

I’ve used this a few times in the past week and the behavior is overall better for my own case. I had one other developer upgrade to master branch and also reported better behavior, though his setup is very similar to my own.

Adding some feedback regarding these changes (I should’ve made a task for this first). Last week I had two developers separately approach me with issues they ran into with arc diff. I had them upgrade to the master branch and this resolved their issues.

I'll investigate the build failures -- I suspect it's the use of the new remarkup rule. At the moment I'm working from a Windows system and haven't been able to get xhpast or unit tests running properly.

Minor corrections

Preferring "installation" as a generic term, where possible

This looks pretty cool and will be useful for maintaining existing text that goes through remarkup. Really useful doc on PhutilRemarkupBlockStorage btw.

These generally look reasonable to me, some thoughts inline. In some of these cases I think we can probably just remove extra references to "this install", "this install of Phabricator on this server", etc., and let it be implied by context.

This is just trying out some changes that modify the language to reduce instances of "Phabricator" from user-visible text. I went through the auth provider implementations to try out using "platform", "server", "service", and in one case I think just "instance".

argh