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 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)

Yeah -- isReady() should really be called something else nowadays (it's, uh, "doTheThingThisFutureDoes()" pretty much), but it would be a backward-compatibility break and I shuffled enough stuff around in T11968 that I didn't want to press my luck.

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

Right: creating a future doesn't start it (it doesn't make any network connections, launch any subprocesses, etc). Technically, you can start a future explicitly by calling $future->start(), but there's little/no reason to ever do so.

Even if we started futures on creation, the process is ultimately still single-threaded and we don't get any kind of I/O handling "for free" in PHP, so execution must pass into most Futures regularly to make meaningful progress on them: for example, if you just start() an HTTP future and don't return to it, it will probably get about as far as opening a network connection but not actually send any bytes over the wire, so it isn't really executing fully in parallel. Subprocess futures usually don't need as much upkeep as network futures, but they can still stall if the stderr buffer on the child process fills up and the parent process hasn't returned execution into the Future to read the buffer.

This might or might not be helpful, but the rough model here is that "hardpoints" are an open slot that some particular data may be loaded into. The physical analogy is a "hardpoint" on fighter aircraft where a particular missile or bomb may be attached.

oops

I looked at Future to see how isReady() plays into other parts and this looks good! Thank you!