HomePhabricator

Arcanist - normalize question about unstaged files across workflows (diff vs…

Description

Arcanist - normalize question about unstaged files across workflows (diff vs land)

Summary:
Fixes T4163. Re-word the question so its the same as in "arc diff". Draw the line though and don't automagically do anything if the user says "Y", 'cuz amending / adding files last minute in 'arc land' and other workflows is batshit insane. Assuming infinite growth in the future, I think its best to get this language consistent now.

I changed the shouldAmend member variable to a shouldAmend() function with a static inside. Previously shouldAmend was getting set as a side effect and it was kind of weird. I thought maybe it was written this way because the calls to the vcs are a little slow or something. As such, I figured caching it in the static was a good idea? Didn't seem awful but maybe a premature optimization with whatever the performance reality turns out to be.

Also a modest amount of bonus pht.

Test Plan: this very diff. i'm going to arc land it laters too.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: chad, epriestley, Korvin

Maniphest Tasks: T4163

Differential Revision: https://secure.phabricator.com/D8753

Event Timeline

/src/workflow/ArcanistBaseWorkflow.php
930–933

i find this really weird... i understand that D8753#33914 was the motivation for it, but the code has changed since and this extra method doesn't seem useful. cc @epriestley

I'll send a follow up diff; I should have implemented exactly what @epriestley asked for.

D8753#33914 to me had one main bit of feedback - get rid of the static. I didn't really understand the member variable bit given the rest of the comment - it seemed to me it would still be caching it if I did the === null thing on a member variable - and the mistake I made here was forgetting that this is the BASE workflow so any child workflows would be fresh instantiations.