After arc diff creates a revision in Phabricator it amends the commit to include a link to the revision in the commit message.
- Queries
- All Stories
- Search
- Advanced Search
- Transactions
- Transaction Logs
All Stories
Jul 15 2021
Clean up the logic/nesting by returning early where possible. Add TODOs for work to be done still.
Jul 14 2021
This is also shedding light on T13107 :)
Jul 13 2021
In D21685, I've imposed stricter rules for which state transitions are allowed: for example, you can't issue a "pause" command to a build that is already pausing.
I take it these disparate trees have some limitations? Do they always apply to the working directory or something?
In Git, we can just create new commits floating out in the middle of nowhere -- not on any branch, and not the ancestor of any labeled commit -- that you can retrieve if you know the hash but aren't otherwise reachable or visible to users without using very low-level commands. This isn't normally useful for human users, but it's great for reducing side effects in arc land, since we can do all our work in a sort of temporary scratch area that automatically gets cleaned up later.
Woah, yea I don't think there's anything like this in Mercurial. I take it these disparate trees have some limitations? Do they always apply to the working directory or something?
Jul 12 2021
I'm still piecing together Git's handling of branches in my mind as I have used Mercurial for so long.
Just thinking aloud to myself
Off-hand I don't know of a solution that could provide that same guarantee for cherry-picking. Our current process also requires that the engineer who authored the fix is the one responsible for merging it, rather than having release engineers or an unlucky engineer responsible for performing all merges into the subsequent versions.
This could probably be handled by having the server-side hook check for commits with the same task number, as we require all commits to start with a task number. It's not as "comfortable" as ensuring the same changeset exists in the branch but since the changesets require merge commits which can potentially introduce incorrect merges that comfort might be just as misleading.
The commits created as a side effect of arc land --hold aren't actually deleted explicitly, but they're effectively gone in Git as soon as you update your working copy to any other state. I'm not sure how closely this holds (or can hold) in Mercurial.
Ah yea this is one difference with Mercurial and Git's treatment of branches. In Mercurial there is no automatic cleanup, so things must be stripped/pruned manually before they will go away -- regardless of whether they're tagged with a bookmark or not. I'm still piecing together Git's handling of branches in my mind as I have used Mercurial for so long. Conceptually I know how I want to manipulate the repository but then things happen which reveal that how I'm thinking the repository is laid out isn't what Git's doing (Arcanist has been very useful in this respect).
Would the expectation then be that running arc land again would remove the rebased/squashed commits created by --hold, so the result would be as if they hadn't run --hold previously?
Remove TODO from ArcanistMercurialWorkingCopyRevisionHardpointQuery. Add some comments to functions in ArcanistLandEngine.
Oh just noticed your comment on this, marking for review so emails go out (I don't plan to make further changes here).
Jul 11 2021
There are a bunch of libraries to help with this that I obviously won't be using because I didn't write them...
If I want to go spelunking later to see about improving/adding this behavior is this the right place to do this or is there another place this TODO should go?
Made updates from comments. Re-ran the "Landing a non-tip revision" test and everything still looks good.
I'll make these last few minor updates and do a quick few tests to make sure everything is still in working order
If there was a way to run the rebase --abort and discard the result of that it would probably be best.
I think not all versions of Windows + PHP require adjusting php.ini, but some nontrivial subset do -- enough that arc itself emits some flavor of guidance on this:
This is no longer my fight.
One thought here is that updating variables_order is fine on the server, but I really don't want to force arc users to muck with php.ini.
This seems to be resolved already but just wanted to add a note - I believe Windows users currently need to modify php.ini to enable the curl extension for arcanist to work. This is from our internal documentation for installing PHP for arcanist:
Browse to C:\tools\php[some version number]. Edit php.ini as listed below.
- Note: The following modifications remove the semi-colon at the start of the line. Semi-colons are comments in .ini files and these changes are uncommenting and updating the lines.
- Find the line ;extension=curl and remove the semicolon.
- Locate the line ;date.timezone = and replace it with date.timezone = UTC to prevent PHP from yelling at you.
Jul 10 2021
Sorry for the churn on this, it sorta became a whackamole for an assortment of issues I ran into. I think most of the test cases I've tried out now indicate that the behavior is pretty solid at this point. For now I don't have any remaining test cases to cover and I think this is in a state for review.
Uncovered an issue when using the evolve extension, in which landing a non-tip revision would leave behind a set of commits in a hideous state.
Don't actually unset the found rebased active commit. Not sure why I thought that was a good idea.
Just did another pass-through of all these changes to do some cleanup.
- Removed an old line of code I was testing out trying to handle multiple onto-markers when restoring the working state
- Updated some comments and formatting of marker ref queries
- Found some logic which didn't quite carry over from refactoring of newPushCommands() in handling of cleaning up markers to be deleted
Whoops - that last change results in the same original behavior. It turns out confirmCommits() reverses the order that selectCommits() creates.
To confirm the min/max change I reviewed the revset x..y documentation along with the expected ordering of commits in the ArcanistLandCommitSet, then renamed + added some comments to clarify this behavior.
I normally use arc:bookmark however I recently switched it to arc:this so it's also compatible with git. All of my tests I've run currently were using arc:this.
Went back through https://secure.phabricator.com/book/phabricator/article/arcanist_commit_ranges/ and realized I can specify a ruleset of arc:bookmark, arc:this to get the behavior I want in both mercurial and git. Huzzah!
I'm guessing the --base argument would affect what commits get snagged up during landing (or is that captured during diff phase only?)
Ah it looks like the --base flag isn't part of diff workflow
$ arc land test --base arc:bookmark Usage Exception: Argument "--base" is unrecognized. Use "--" to indicate the end of flags.
I'm going to do a few more tests -- I'm guessing the --base argument would affect what commits get snagged up during landing (or is that captured during diff phase only?). I normally use arc:bookmark however I recently switched it to arc:this so it's also compatible with git. All of my tests I've run currently were using arc:this.
Add back in the rebase --abort for now
When landing from a working directory that isn't in the set of changes being landed, restore the working directory to that commit after the land.
Not-working state test
- I created a diff with an inactive bookmark test but working directory on an older commit not related to the one being landed
- I landed that diff
- I verified the ending resulting working state was on the older commit I was on prior to landing
Doing a basic test this behavior looks good
$ hg wip @ 19:823c7457b7ef cspeck (30 seconds ago) tip test <- draft commit to be landed, working state when running `arc land` | test commit | o 17:33c3ae4d1277 cspeck (8 minutes ago) master |/ test commit o 14:82892d507c28 cspeck (24 minutes ago) | test commit ~
Determine whether the working directory was rebased, and if so update to it during reconcileLocalState()
Jul 9 2021
For now, this has been working fine as a simple CLI flow.
When using --template I think I was not properly escaping quotes for how they are eventually invoked when launching the process
To sum up what I think the issues I ran into last night were
- I was not purging caches when switching versions of mercurial or when making changes to DiffusionMercurialBlameQuery
- I had improperly inverted strpos(':', $line) instead of strpos($line, ':')
- When using --template I think I was not properly escaping quotes for how they are eventually invoked when launching the process
There's one minor bug with annotations using mercurial 4.2 for file lines that contain : not being annotated but it works on 5.8.
This was because I had made modifications to the resolveBlameFuture() in my testing. I undid these changes and things are working again. I tested both mercurial 5.8 and 4.4, purging all caches in between and verifying that annotations and history appear correctly, including annotations of files with code lines including ':'.
Update the handling of template arguments to avoid using quotes around the template being used
I've been incorporating ./bin/cache purge --all into my test process and things are making a lot more sense now. I'll hopefully have some changes up here shortly. There's one minor bug with annotations using mercurial 4.2 for file lines that contain : not being annotated but it works on 5.8.
Ah, yes; DiffusionBlameQuery has a cache.
Is it possible the annotate results are cached somewhere and might be causing some of the inconsistency I’m seeing?
I'll take another look tomorrow. Somehow I managed to get the code to emit output and found the issue with strpos() but then after fixing that it now seems busted again for both versions of mercurial. No exceptions are happening but it's not actually building up the annotation data properly (which is just the ordered list of the commit hashes I think?)
Fix strpos()
Best guess is that it's maaaaaaybe because we pass HGPLAIN=1 in the environment (DiffusionMercurialCommandEngine) and your version of Mercurial has annotate set up as an extension? That seems like a big stretch, but wouldn't show up.
I don't believe annotate is setup as an extension. This is our global .hgrc
# Ansible managed
I added some logs and then removed them but functionality nothing else should have changed (except the --template quoting I mention) and now when running hg 4.4 it no longer errors, but it the blame doesn't list changesets (but does put color next to the lines which I think is just a default if nothing was found?)
is there an easy way to log the command that will run from the CommandFuture?
Buh there must be something going on in the environment that's causing issues. I logged out the command it's running and tried running it directly in the repo
[cspeck@repo-testing1 85]$ hg --version Mercurial Distributed SCM (version 4.4.2) (see https://mercurial-scm.org for more information)
Oh that error happens when resolveBlameFuture() returns null because there was a problem invoking the command... looking a little more into what issue is happening
Hmm this is the error I'm getting when using mercurial 4.4
2021/07/08 22:35:31 [error] 9693#9693: *1 FastCGI sent in stderr: "
I setup mercurial to run version 4.4, created a new repository, added some commits, and verified all the above behavior still works properly.
This isn't quite true -- annotations don't appear to be showing up. I'll take a quick look
More updates based on comments. Tested out history view and annotation view and things look good.
Fix a regex typo and add back in the trim() on the hash
%C ("raw, unescaped comment")
Something isn't quite right here. Looking at the history of a repo, the graph rendering doesn't match what it was previously doing
😎
Run arc liberate for the new test file
forgot to submit my comments~
Do capability-check for using hg annotate --template vs --debug, set the required version to 2.4, address other comments
Adding a capability check for hg annotate --template
Jul 8 2021
Ah okay, I'll do some other testing. I was also wondering if there would be cases in Phabricator where something is doing something like if (get_class($a) == 'PhabricatorEnv') {... which if so I'm guessing would cause failures.