Page MenuHomePhabricator
Feed All Stories

Wed, Jul 21

cspeckmim added inline comments to D21700: Display informative message when arc launches an editor.
Wed, Jul 21, 12:31 AM

Tue, Jul 20

epriestley accepted D21700: Display informative message when arc launches an editor.

There a bit of fancier indent/formatting stuff in some of the newer code (e.g., in ArcanistRefView->newLines()) but it hasn't really generalized into something you could easily apply here yet. I think this is fine for now and we could revisit it to make it fancier later on if the newest display stuff gets generalized a bit.

Tue, Jul 20, 10:58 PM
cspeckmim added a comment to D21700: Display informative message when arc launches an editor.

Maybe the output could be two separate lines:

Launching editor ("nano")...
Supply commit message for uncommitted changes, then save and exit.

Then neither part needs to care about the other part, and things are likely more translatable and such.

Yea that works a lot better. Though I'm on the fence about the order of the two messages. Logically my brain wants the "Launching..." message to be second but I don't think it really matters

Tue, Jul 20, 10:30 PM
cspeckmim updated the diff for D21700: Display informative message when arc launches an editor.

Update the display format to not try and combine two things

Tue, Jul 20, 10:25 PM
epriestley added a comment to D21700: Display informative message when arc launches an editor.

Maybe the output could be two separate lines:

Tue, Jul 20, 10:12 PM
cspeckmim added inline comments to D21700: Display informative message when arc launches an editor.
Tue, Jul 20, 10:04 PM
cspeckmim requested review of D21700: Display informative message when arc launches an editor.
Tue, Jul 20, 9:59 PM
cspeckmim added a revision to T3271: Before launching $EDITOR from arc, print that we're doing it: D21700: Display informative message when arc launches an editor.
Tue, Jul 20, 9:58 PM · Arcanist
epriestley requested review of D21699: Rename "HarbormasterRestartException" to "HarbormasterMessageException".
Tue, Jul 20, 9:45 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21699: Rename "HarbormasterRestartException" to "HarbormasterMessageException".
Tue, Jul 20, 9:43 PM · Harbormaster
epriestley requested review of D21698: Allow "harbormaster.sendmessage" to send control command (pause, restart, abort, resume) to Builds/Buildables.
Tue, Jul 20, 9:42 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21698: Allow "harbormaster.sendmessage" to send control command (pause, restart, abort, resume) to Builds/Buildables.
Tue, Jul 20, 9:41 PM · Harbormaster
cspeckmim closed D21686: Update "arc diff" to amend non-head commits with Mercurial.
Tue, Jul 20, 8:55 PM
cspeckmim committed rARC41f6c6ecb2ec: Update "arc diff" to amend non-head commits with Mercurial (authored by cspeckmim).
Update "arc diff" to amend non-head commits with Mercurial
Tue, Jul 20, 8:55 PM
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

That all looks good to me.

Tue, Jul 20, 7:57 PM
cspeckmim added a revision to T13659: `arc land` may fail with missing rebase extension: D21697: Refactor how Mercurial runs commands that require extensions.
Tue, Jul 20, 4:36 AM · Arcanist, Mercurial
cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Remove the stdout text matching, I confirmed that using arc diff with uncommitted changes on a head changeset properly amended the change into the changeset.

Tue, Jul 20, 1:52 AM
cspeckmim added inline comments to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Tue, Jul 20, 1:39 AM
cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Revert changes to GitLocalState

Tue, Jul 20, 1:24 AM

Mon, Jul 19

cspeckmim added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

If the git changes are too much I can pull those out- I think a longer-term solution will be to do version-checking for whether git stash create is supported and use that instead but I can do that in a separate change.

Mon, Jul 19, 8:35 PM
cspeckmim added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

I really like that idea. I generalized the comments on the methods and then added a stack to GitLocalState. I ran two basic tests for this, running arc diff with uncommited changes and running arc land with uncommitted changes. For arc diff it properly amended the uncommitted changes into my diff (unsure if stash is used here), and for arc land it properly stashed my change during the land and restored it afterwards.

Mon, Jul 19, 8:33 PM
cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Update comments, include an internal stack in ArcanistGitLocalState for tracking the order of save/restore

Mon, Jul 19, 8:30 PM
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

We could conceivably prevent that error by making GitLocalState keep a trivial stack (just a stack of true) and return its current depth (0, 1, 2, ...), then throw if the passed $ref was not the current stack depth.

Mon, Jul 19, 7:18 PM
cspeckmim added inline comments to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Mon, Jul 19, 6:48 PM
cspeckmim added inline comments to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Mon, Jul 19, 6:01 PM
cspeckmim added inline comments to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Mon, Jul 19, 5:55 PM
cspeckmim added a comment to T13659: `arc land` may fail with missing rebase extension.

I'm also surprised that rebase extension isn't enabled by default. I guess I have been turning it on in my default setup.

Mon, Jul 19, 5:42 PM · Arcanist, Mercurial
cspeckmim claimed T13659: `arc land` may fail with missing rebase extension.

I like that idea. I'll take a look.

Mon, Jul 19, 5:41 PM · Arcanist, Mercurial
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

Since modern Arcanist usage with Mercurial has required the arc-ls-markers for a while now and nobody else has reported issues, I think it's fine to not try to address these issues for older versions.

Mon, Jul 19, 5:14 PM
epriestley accepted D21686: Update "arc diff" to amend non-head commits with Mercurial.
Mon, Jul 19, 5:12 PM
epriestley added a comment to T13659: `arc land` may fail with missing rebase extension.

...perhaps we should consider a way to do this automatically or at least less-manually.

Mon, Jul 19, 4:58 PM · Arcanist, Mercurial
epriestley added a comment to D21680: An assortment of fixes and updates to using arc-land with mercurial.

This makes me think that in this second scenario where running arc diff initially includes multiple commits that the revision should be updated to include the commit hashes for each, and then might result in having better detect this scenario if it's searching for revisions with associated commit hashes.

Mon, Jul 19, 4:33 PM
epriestley triaged T13659: `arc land` may fail with missing rebase extension as Low priority.
Mon, Jul 19, 4:32 PM · Arcanist, Mercurial
cspeckmim added inline comments to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Mon, Jul 19, 2:36 AM
cspeckmim added a comment to D21680: An assortment of fixes and updates to using arc-land with mercurial.

Still, I'd expect arc land <head2> to look at the ancestors between head2 and master, find the "Differential Revision" commit, and then figure out that "head2" can only be part of that revision, possibly prompting you ("Local range includes commits which don't match the revision, are you sure you want to land them?").

Mon, Jul 19, 2:10 AM

Sun, Jul 18

cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Fix up comments

Sun, Jul 18, 5:04 PM
cspeckmim added inline comments to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Sun, Jul 18, 5:03 PM
cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Updating arc-amend to support some older versions of Mercurial

Sun, Jul 18, 4:48 PM
cspeckmim added inline comments to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Sun, Jul 18, 5:32 AM
cspeckmim updated the test plan for D21686: Update "arc diff" to amend non-head commits with Mercurial.
Sun, Jul 18, 4:54 AM
cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Remove unused import from arc-hg.py

Sun, Jul 18, 4:46 AM
cspeckmim added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

I went back to test this with Mercurial 4.0 just as a test and the extension file fails to load properly. I traced the issue down to this issue
https://stackoverflow.com/questions/52117289/simple-mercurial-extension-fails-to-import

Sun, Jul 18, 4:44 AM
cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Use arc-amend instead of a long Mercurial dance

Sun, Jul 18, 4:34 AM

Sat, Jul 17

cspeckmim added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

Sounds good. I spent 15 seconds on trying to write hg arc-amend, and this does something and does appear to produce an amended commit that looks ballpark-correct (and can amend non-heads), but emits some warnings and I have no clue if it functions across Mercurial versions:

diff --git a/support/hg/arc-hg.py b/support/hg/arc-hg.py
index 01ac3907..82202767 100644
--- a/support/hg/arc-hg.py
+++ b/support/hg/arc-hg.py
@@ -28,6 +28,12 @@ _ = i18n._
 cmdtable = {}
 command = registrar.command(cmdtable)
 
+@command(
+  b'arc-amend')
+def amend(ui, repo, source=None, **opts):
+  cmdutil.amend(ui, repo, repo[b'.'], {}, [], opts)
+  return 0
+
 @command(
   b'arc-ls-markers',
   [(b'', b'output', b'',

Still, it seems possible that this is the least-messy pathway for non-evolve support since we need the extension anyway and all the "can't amend non-head" stuff seems advisory rather than technical, given that flags exist to disable the checks and the low-level call seems to produce the desired repository state.

Sat, Jul 17, 1:25 AM
cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Fix some typos and comments before @epriestley spots them

Sat, Jul 17, 12:00 AM

Fri, Jul 16

cspeckmim added inline comments to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Fri, Jul 16, 11:45 PM
cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Added a workflow argument to the amendCommit() path so it can be set on the local state for logging purposes.
Fixed the issue with the terminal, Ph'nglui mglw'nafh Cthulhu R'lyeh wgah'nagl fhtagn!

Fri, Jul 16, 11:40 PM
epriestley requested review of D21696: Add a side nav to Conduit API method console pages.
Fri, Jul 16, 10:16 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21696: Add a side nav to Conduit API method console pages.
Fri, Jul 16, 10:15 PM · Harbormaster
epriestley added a comment to T13652: Notes on Ardunio CNC drivers.
  • Cryptographically Random Orbit Sander
Fri, Jul 16, 9:43 PM
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

Sounds good. I spent 15 seconds on trying to write hg arc-amend, and this does something and does appear to produce an amended commit that looks ballpark-correct (and can amend non-heads), but emits some warnings and I have no clue if it functions across Mercurial versions:

Fri, Jul 16, 5:35 PM
cspeckmim added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

Given the mess of dealing with the non-evolve case, what do you think about splitting this into a smaller "make this work under evolve" change and landing that now (which should solve everything for your org, since everyone has evolve?) since all that code looks good to me, and then maybe revisiting the non-evolve case if it other users hit it and/or diff gets some work and it's easier, and/or we figure out a reasonable way to hg force-amend or do some other less-tortured operation here.

Unfortunately not everyone at my org uses evolve, in fact most don't. I've never gotten a solid reason for why but I suspect it's just "new" and frightening. I started looking at this change initially because someone ran into an error when trying to arc diff a non-head revision. I'll try to investigate what's happening - it totally hoses my console and I can't scroll up to view the history when using --trace so I'll have to pipe the output to a file or something. If I can't figure it out I might just throw an error stating that evolve is required here when trying to amend changes to a non-head commit. I'll extract out the non-evolve-local-changes amend to a separate function.

Fri, Jul 16, 4:54 PM
epriestley requested review of D21695: Add stub "harbormaster.build.edit" and "harbormaster.buildable.edit" API methods.
Fri, Jul 16, 4:44 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21695: Add stub "harbormaster.build.edit" and "harbormaster.buildable.edit" API methods.
Fri, Jul 16, 4:43 PM · Harbormaster
epriestley requested review of D21694: Modularize "HarbormasterBuildableTransaction".
Fri, Jul 16, 4:01 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21694: Modularize "HarbormasterBuildableTransaction".
Fri, Jul 16, 4:00 PM · Harbormaster
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

My only guess is you've delved into the Mercurial source base (I've poked around a little)

Fri, Jul 16, 3:55 PM
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

I don't think LocalState should z̵̰̲͚͖̊͗́̓̓a̷̛̱̯̠͐̿̅̅͛̓͗ĺ̵̨̻̠͓̼̗̥̥̻̲̳̎͜ͅg̴̛͙͗̀̄̈́̃̂̓̄͝o̶̥̘̻͙̬͇͚̥̪͌̔̃͛̆̀̂̚ͅ your terminal!

Fri, Jul 16, 3:54 PM
epriestley requested review of D21693: Remove "HarbormasterBuildableTransaction::TYPE_CREATE".
Fri, Jul 16, 3:35 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21693: Remove "HarbormasterBuildableTransaction::TYPE_CREATE".
Fri, Jul 16, 3:34 PM · Harbormaster
epriestley requested review of D21692: Remove "HarbormasterBuildCommand".
Fri, Jul 16, 3:11 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21692: Remove "HarbormasterBuildCommand".
Fri, Jul 16, 3:10 PM · Harbormaster
cspeckmim added inline comments to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Fri, Jul 16, 3:07 AM
Harbormaster failed remote builds in B25457: Diff 51654 for D21686: Update "arc diff" to amend non-head commits with Mercurial!
Fri, Jul 16, 3:04 AM
cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Attempt to stash unsaved changes before doing the rebase dance. Not currently in a working state.

Fri, Jul 16, 3:03 AM
cspeckmim added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

You can probably stash/restore by using ArcanistMercurialLocalState, although I'm not 100% sure it's reentrancy-safe and I'd imagine arc diff may already be holding a LocalState object somewhere above this callsite in the future.

Might that manifest in ways like this?

Screen Shot 2021-07-15 at 10.59.31 PM.png (816×1 px, 135 KB)

Fri, Jul 16, 3:02 AM
cspeckmim added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

Holy moly I'm not sure I would've found that out lol. My only guess is you've delved into the Mercurial source base (I've poked around a little)

Fri, Jul 16, 2:42 AM
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

This is probably not a great path to walk down, but there are secret flags that disable the "cannot amend changeset with children" check:

Fri, Jul 16, 2:15 AM
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

Another possible approach might be to copy the amend extension to create hg arc-amend which just lets you amend anything, assuming the "head only" rule is a safety guard rail rather than a fundamental issue with Mercurial (which it seems like it should be).

Fri, Jul 16, 2:07 AM
epriestley requested review of D21691: Modularize almost all Harbormaster build message workflows and UI/UX.
Fri, Jul 16, 1:32 AM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21691: Modularize almost all Harbormaster build message workflows and UI/UX.
Fri, Jul 16, 1:30 AM · Harbormaster
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

Ohhhhh, sorry, I didn't connect the dots and understand that you're only sometimes running the amend (since you can't amend for non-heads).

Fri, Jul 16, 1:28 AM
cspeckmim added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

The desired behavior is that you can not enter any interesting workflow in arc diff if your working copy is dirty, so amendCommit() "should" be able to safely assume that any working-copy dirtiness has been made by arc, likely in arc lint, and is appropriate to amend into repository state.

Ah the stashing I'm thinking needs done is due to how (I think) amending a non-head changeset without evolve has to happen.

C
|
B
|
A* (changes from lint, want to amend)
|
master

Create a copy of A with the new commit message

C
|
B
|
A
|
A*  A'
|  /
o  master

Rebase B:: onto A'

    C'
    |
    B'
    |
A*  A'
|  /
o  master

And then strip A*

Fri, Jul 16, 1:12 AM

Thu, Jul 15

epriestley added a comment to T13072: Merge Harbormaster BuildCommand into BuildMessage.

This ("Queued at") looks suspicious:

Thu, Jul 15, 11:03 PM · Harbormaster
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

I'm not catching on to what you're referring to. Are you saying there are other changes which will have already stashed unsaved state away before calling amendCommit()?

Thu, Jul 15, 11:02 PM
cspeckmim added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

Just for completeness for future readers: this behavior depends on history.immutable today, but arc would generally like to do this and Mercurial users broadly seem more open to treating local history as mutable today than they did in ~2011 (as does Mercurial itself, with changes like evolve).

Would this be good to comment on ArcanistMercurialAPI::amendCommit()?

Thu, Jul 15, 10:25 PM
epriestley requested review of D21690: Modularize individual Harbormaster build messages.
Thu, Jul 15, 10:25 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21690: Modularize individual Harbormaster build messages.
Thu, Jul 15, 10:24 PM · Harbormaster
epriestley requested review of D21689: Modularize HarbormasterBuildTransaction.
Thu, Jul 15, 9:07 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21689: Modularize HarbormasterBuildTransaction.
Thu, Jul 15, 9:05 PM · Harbormaster
epriestley requested review of D21688: Remove "HarbormasterBuildTransaction::TYPE_CREATE".
Thu, Jul 15, 6:10 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21688: Remove "HarbormasterBuildTransaction::TYPE_CREATE".
Thu, Jul 15, 6:09 PM · Harbormaster
epriestley requested review of D21687: Correct the flow of edit authority when sending messages to HarbormasterBuild objects.
Thu, Jul 15, 6:04 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21687: Correct the flow of edit authority when sending messages to HarbormasterBuild objects.
Thu, Jul 15, 6:02 PM · Harbormaster
epriestley added a comment to D21686: Update "arc diff" to amend non-head commits with Mercurial.

After arc diff creates a revision in Phabricator it amends the commit to include a link to the revision in the commit message.

Thu, Jul 15, 4:05 PM
cspeckmim planned changes to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Thu, Jul 15, 5:13 AM
cspeckmim updated the diff for D21686: Update "arc diff" to amend non-head commits with Mercurial.

Clean up the logic/nesting by returning early where possible. Add TODOs for work to be done still.

Thu, Jul 15, 5:00 AM
cspeckmim planned changes to D21686: Update "arc diff" to amend non-head commits with Mercurial.
Thu, Jul 15, 4:42 AM
cspeckmim requested review of D21686: Update "arc diff" to amend non-head commits with Mercurial.
Thu, Jul 15, 4:30 AM

Wed, Jul 14

cspeckmim added inline comments to D21682: Add a prompt to allow pruning merged branches when using --hold.
Wed, Jul 14, 4:19 AM
cspeckmim added a comment to D21682: Add a prompt to allow pruning merged branches when using --hold.

This is also shedding light on T13107 :)

Wed, Jul 14, 3:26 AM

Tue, Jul 13

epriestley added a comment to T13072: Merge Harbormaster BuildCommand into BuildMessage.

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.

Tue, Jul 13, 11:41 PM · Harbormaster
epriestley retitled D21685: Improve formality of "HarbormasterBuild" states from Improve formality of "HarbormasterBuild" messages to Improve formality of "HarbormasterBuild" states.
Tue, Jul 13, 11:39 PM
epriestley requested review of D21685: Improve formality of "HarbormasterBuild" states.
Tue, Jul 13, 11:38 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21685: Improve formality of "HarbormasterBuild" states.
Tue, Jul 13, 11:37 PM · Harbormaster
epriestley requested review of D21684: Merge the "HarbormasterBuildCommand" table into "HarbormasterBuildMessage".
Tue, Jul 13, 10:50 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21684: Merge the "HarbormasterBuildCommand" table into "HarbormasterBuildMessage".
Tue, Jul 13, 10:48 PM · Harbormaster
epriestley requested review of D21683: Rename "HarbormasterBuild" methods to prepare for use of the "BuildMessages" table.
Tue, Jul 13, 10:24 PM
epriestley added a revision to T13072: Merge Harbormaster BuildCommand into BuildMessage: D21683: Rename "HarbormasterBuild" methods to prepare for use of the "BuildMessages" table.
Tue, Jul 13, 10:23 PM · Harbormaster
epriestley updated the task description for T13072: Merge Harbormaster BuildCommand into BuildMessage.
Tue, Jul 13, 10:04 PM · Harbormaster