- Queries
- All Stories
- Search
- Advanced Search
- Transactions
- Transaction Logs
All Stories
Nov 18 2021
Nov 17 2021
Closing this in favor of T13630, which covers the same ground.
Nov 16 2021
Nov 15 2021
I'm planning to simply delete the Discourse forum without preserving any content.
Oct 29 2021
Oct 23 2021
Thank you for the answer, appreciate it, and your effort that goes into arcanist.
Oct 21 2021
The non-public parts of Phage are currently very specific to Phacility's cluster and probably not generally useful. The current version of PhageRemoteWorkflow is similar to P2107 and depends on particular Phacility services and hosts to enumerate valid remotes and negotiate a connection to them through a bastion pool. These service-listing and bastion-host components are not generalized and not trivially generalizable.
It seem to me like certain parts of phage are not published in a public repo. Are there plans to open up missing pieces?
Oct 19 2021
- As Alice, commandeer a revision authored by Baliey and reviewed by Claire. Edit it locally to do arbitrary bad things, then git push it.
- Make a commit, edit the commit message to say Differential Revision: D1234, where D1234 is a current, valid, accepted revision authored by anyone, then git push it.
I believe it is extremely difficult to configure Phabricator to provide the assurance you describe, particularly if arc land does anything. If you are actually providing this guarantee ("an attacker needs two machines"), you can likely add a clause to the large amount of custom code you've written to prevent self-foisting while still supporting other foisting use cases. If you haven't written a large amount of custom code, I suspect an attacker can fairly easily deploy with one machine without using "Foist Upon".
Is there a way to disable this feature? Our security team has noticed that with this feature we can land code with just a single person's machine being compromised (we rely on an attacker needing two machines to deploy code as a safety mechanism). I.e. You make a revision, Foist it on someone, Approve it, then arc land it as the other person (saying y to the prompt).
Just making a note that I did test git a while back and saw behavior that I wasn't expecting; I haven't had a chance to dig further into this. I'm not super familiar with how to visualize the commit graph in git to confirm this but I believe what happened is a graph that looked like this
A B | / C | D (master)
Oct 3 2021
Oct 1 2021
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:
Sep 28 2021
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.
Sep 24 2021
Marking Plan Changes until I test this out in Git
Sep 23 2021
Remove the $obsolete_map and $rebasedActiveCommit and just use $rebasedCommitMap
Sep 22 2021
Sep 18 2021
Sep 17 2021
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:
Sep 16 2021
Switch to using strncmp()
Sep 14 2021
Sep 13 2021
Sep 7 2021
Sep 5 2021
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!
Sep 4 2021
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