Paths

Table of Contentst

Differential D8753

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

Authored by btrahan on Apr 10 2014, 11:05 PM.

Details

Reviewers

epriestley

Maniphest Tasks

T4163: "arc diff" and "arc land" use two different worded questions about untracked files

Commits

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

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.

Diff Detail

Repository

rARC Arcanist

Branch

T4163

Lint

Lint Passed

Unit

No Test Coverage

Event Timeline

btrahan updated this revision to Diff 20759.Apr 10 2014, 11:05 PM

btrahan added a task: T4163: "arc diff" and "arc land" use two different worded questions about untracked files.

btrahan retitled this revision from to Arcanist - normalize question about unstaged files across workflows (diff vs land).

btrahan updated this object.

btrahan edited the test plan for this revision. (Show Details)

btrahan added a reviewer: epriestley.

Herald added subscribers: Korvin, epriestley.Apr 10 2014, 11:05 PM

@chad - FYI I am throwing the gauntlet down here.

What gauntlet is that?

This is fairly contentious change overall I guess. I think its best for new users / looking long term. For careful readers its basically the same. For people who have lots of untracked files and muscle reflex type "Y" or "N" -- well, they're broked, sorry.

i just use the web ui

Couple of tiny nits, but overall everything here looks great to me.

src/workflow/ArcanistBaseWorkflow.php
785	For consistency, prefer `.` over `,`
826–830	Maybe call this some thing like `askToAddFiles()` or similar, to make it clear that it returns answer, not, e.g., an actual prompt text.
930	I think this cache should still be a normal property, but the new implementation is otherwise better. It's possible for a command to end up running multiple workflows, and we shouldn't carry the answer to this question from one workflow to another -- although it can't happen today, some day "arc diff" could make us run workflows in different working copies (maybe, for example, Git submodules or SVN externals) which might have a different answer to this question. So we'd end up with, e.g.: private $shouldAmend; private function getShouldAmend() { ... } private function calculateShouldAmend() { ... }

This revision is now accepted and ready to land.Apr 11 2014, 11:17 AM

updates

Closed by commit rARC77a9c1814063 (authored by @btrahan).

Revision Contents
Changeset List

Path

Size

src/

workflow/

ArcanistBaseWorkflow.php

50 lines

Diff 20775

View Options

src/workflow/ArcanistBaseWorkflow.php

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