The Future stuff is used on both the client and server, so it lives in Arcanist rather than Phabricator. The method definition should be here:

See also T12404#256288 for a note on each removal.

each is usually easy to replace and I'm happy to accept a change to replace it if someone wants to reproduce/test it. I believe this (totally ridiculous) construction:

From https://discourse.phabricator-community.org/t/sending-emails-causes-an-exception/4966, looks like the PHPMailer uses each which is removed in php 8.

From https://discourse.phabricator-community.org/t/sending-emails-causes-an-exception/4966, it appears that both class.smtp.php and class.phpmailer-lite.php have calls to each which is removed in PHP 8.

Marking Plan Changes until I test this out in Git

Remove the $obsolete_map and $rebasedActiveCommit and just use $rebasedCommitMap

The second baby has arrived

The second baby has arrived so I have about 17 seconds per day to look at my computer nowadays, but I think it would also be reasonable to backfill str_starts_with() if you run into more of this -- the strncmp() syntax has always felt pretty hard to read to me. You can do that in PHP like this:

Switch to using strncmp()

I played around with this some more and this feels pretty solid for resolving most common symbols. I'll poke around some of the mercurial source a bit later this week and see if I can find an easy way for resolving revset/symbols easily though a custom extension.

Don't bother trying to lookup bookmark/tag names when the symbol is a number as Mercurial disallows this. Also fix the revision number mapping which somehow worked? But still does now...

Fix not being able to lookup commit by symbol 0

Include support for revision number and hash-prefix symbols

The "symbol" here could, in the absolute-most-general case, be a short commit hash (abcd1234) or symbolic commit (tip^^^) -- not just a bookmark or tag name -- so I think this trick (though quite clever) may run into trouble some day.

Oh yea... hmm I think it's possible to include support for commit hashes (or prefixes) as well as revision ID -- I'll try out a change real quick to see if it's simple to include support for those symbols at least. Any relativity modifiers ^^ I think are effectively impossible to handle without resorting to individual hg log commands or an extension

The "symbol" here could, in the absolute-most-general case, be a short commit hash (abcd1234) or symbolic commit (tip^^^) -- not just a bookmark or tag name -- so I think this trick (though quite clever) may run into trouble some day. This looks like a reasonable fix to the immediate issue, though, and we can cross this bridge if/when we come to it.

More-correct formatting for the revset

Update the revsets used to work around potentially invalid symbols from preventing all symbols from resolving

Ooh actually it looks like this will work

$ hg log --revset "bookmark('re:^symbol$') or tag('re:^symbol$')"

Doing that doesn't fail if the symbol doesn't exist

Hmm could maybe do something like hg log --rev 'bookmark() or tag() or .' to just get every commit that has a potential symbol and then process those. It would end up getting more results than necessary in every case. The query runs relatively quickly (~230ms) on our ~128k commit repository but it only has ~550 tags which I'm guessing is low -- if we tagged every build we'd have ~15k.

Not yet handling the case where an invalid symbol results in failing to lookup any symbols

I didn't see any obvious way of doing this through revsets but I'll try some more experiments. An extension that adds a template keyword, or just a new command for this, might also be an option

Not yet handling the case where an invalid symbol results in failing to lookup any symbols

Provide implementation of ArcanistMercurialCommitSymbolCommitHardpointQuery to enable hardpoint queries for commit symbols.

So that part is correct, but swapping the commit and branch was incidental, and I overlooked it because we usually get the same result: in common cases, this is moot because ancestors(<branch name>) is equivalent to ancestors(<heads of that branch>), and branch(<commit hash>) is equivalent to branch(<branch of that commit>). The latter is always equivalent; the former is equivalent when commit is the single head of branch, which it almost always is in regular use.

But this behavior (on this install) is unambiguously wrong:

...since b7be5c961297 (the parameter) is an ancestor of 162a9c1e8e44 (the first commit in the result set). I'll send you a diff.

Thanks for checking this out

Err, I updated the error message after running those cases so it no longer says "Failed liberating:" but "Failed to update library:"

Sorry I made a side-stop checking out the error-handling behavior from ArcanistLiberateWorkflow - I made some updates so it can detect the failure to avoid indicating success.

Update error handling of ArcanistLiberateWorkflow to detect failure from running rebuild-map.php in a separate process.

Thanks!

Remove log() entirely, let filesystem errors escape outwardly.

I think it's reasonable to remove the try/catch and just let the exception escape. It (the try-catch + log) isn't consistent with the approach to error handling in the rest of the codebase, and I can't think of any valid reason to continue here after a write failure.

Good catch!

Change approach - remove --verbose, remove --quiet, remove most instances of log() being used, keep the instance when an error is encountered.

Thanks!

I didn't make a task for this as it's a fairly small change and the reproduction/test case is also fairly simple.

One tangential thing I noticed from the error messages in these cases, in DiffusionHistoryQueryConduitAPIMethod the getMercurialResult() function executes this hg command

$path_args = array();
if (strlen($path)) {
  $path_args[] = $path;
  $revset_arg = hgsprintf(
    'reverse(ancestors(%s))',
    $commit_hash);
} else {
  $revset_arg = hgsprintf(
    'reverse(ancestors(%s)) and branch(%s)',
    $drequest->getBranch(),
    $commit_hash);
}

With just the updates to the future handling this is now the landing page I see with missing commits:

I tested this out and it works properly, in that there's no noticeable difference between the behavior with this change and without but including the new future updates

Sigh maybe update arcanist too. I ran arc lint before arc diff and it said no lints...

Try again but with arcanist on master

Rebased onto changes (no merge needed tho)

I'm working on testing out the issue before/after this change

I'll do that

This looks great to me now, as long as I didn't miss anything with the Future stuff and it does what it's supposed to on top of D21720 + D21721.

I'd favor this change:

I hadn't considered that having this still use http requests could be done in parallel vs. the clustered running them in serial being faster (theoretically)