Summary:
See D2049, D2050. Identify reuses of locals as iterator variables. Before raising an error, we require:
- Variable is declared before the loop.
- Variable is used after the loop, ignoring uses as an iterator variable.
I think this identifies all problems with a very low false positive rate (the false positives are suspicious/unconventional code, but not necessarily errors).
Also fix an issue identified by the linter.
Test Plan:
- Verified this identified the bugs in D2049 and D2050.
- Ran linter against libphutil/, arcanist/ and phabricator/ (see D2051, this, and next diff).
- Ran unit tests.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley, jungejason
Differential Revision: https://secure.phabricator.com/D2052
Summary:
Addresses concerns in rARCefb8219196abf047f14b505959e54d078e1df6d3:
- As I recall, the intent of "generateFile" was that these warnings would replace all the other warnings for that file, under the theory that if one warning caused regeneration of
the file the other warnings were irrelevant.
- However, this code never had any effect and I haven't seen any issues with the behavior.
- So, just remove it.
Addresses concerns in rARC070e963d1c26879e47eab19a2377e388c2f166c5:
- Ran "arc liberate --all".
Test Plan: Ran "arc lint" after removing an "__init__.php" with and without a "fixed" generateFile block, there was no behavioral change. So I just nuked it.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2049
Summary:
D2016 changed the behavior of `phutil_console_wrap()`.
The new behavior is better so I am fixing callsites instead of the function.
Test Plan:
arc help help
Verify that the options descriptions is not indented with 28 spaces.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2047
Summary: Git can't apply filename case change patches on case-insensitive filesystems. Some day we could manually do this ourselves, but it's fairly rare and complicated -- just raise a useful warning.
Test Plan: Tried to apply a case-changing patch, got a good error.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T618
Differential Revision: https://secure.phabricator.com/D2015
Summary:
- We have "--merge", added similar "--squash" for completeness.
- Switched away from (cd .. && ...) pattern which is not functional on Windows.
- Allow a default --onto to be specified, if master isn't the best default for a project.
Test Plan:
- Did --hold --keep dry runs, changes landed properly.
- Set a default onto branch in .arcconfig.
- Ran --merge and --squash together to verify conflict configuration.
Reviewers: btrahan
CC: aran, epriestley
Maniphest Tasks: T124, T1033
Differential Revision: https://secure.phabricator.com/D2001
Summary:
In Mercurial, we figure out if the working copy is dirty with "hg diff", and do a somewhat expensive merge-base / outgoing operation if the relative commit isn't set. We can set the relative commit earlier and avoid some extra work.
We also may incorrectly cache this state (diff from merge-base of outgoing to tip) and pass the wrong rev and file dirty list to the linters.
Test Plan: Made commits which changed (A, B) and then (A). Ran "arc diff tip^". Before this change, observed full outgoing + merge base resolve and both "A" and "B" passed to lint. Observed execution of fewer commands and lint executing against "A" only after this change.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1998
Summary:
See task. Allow mercurial users to diff with uncommitted changes.
- By default, commit range is merge-base of `hg outgoing` to `.` (dirstate).
- You can get JUST dirstate with `arc diff tip` or similar.
- This ended up being a giant mess various other changes to deal with empty `hg outgoing` and empty dirstate.
Test Plan: Diffed with uncommitted changes, got sensible prompts and results.
Reviewers: Makinde, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T998
Differential Revision: https://secure.phabricator.com/D1954
Summary: We crash and burn right now, trying to launch an interactive editor against a non-tty stdin. Raise a helpful error message instead.
Test Plan: Will run "arc diff --raw --update".
Reviewers: btrahan, yairlivne
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1949
Summary:
- `git rev-parse --verify` "verifies" very valid-looking commit name, not just valid commit names.
- Currently, if we can't find the base rev we'll incorrectly "verify" it and then fail on "git checkout -b <branch> <some bogus commit>".
- Instead, use `git cat-file -t`.
- See similar fix in D1590.
Example:
$ git rev-parse --verify aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Test Plan: Ran "arc patch" in a mismatched local, hit "Y" to branch, got a branch off HEAD instead of an error.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1947
Summary:
In some workflows, we don't have Conduit at all, so we'll fail with a raw exception. Test for conduit presence before trying to make the encoding call.
Also move some "instanceof" logic for updates into RepositoryAPI (factoring, windows compat).
Test Plan: Ran "arc patch --patch some.patch".
Reviewers: 20after4, davidreuss, nh, btrahan
Reviewed By: 20after4
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1935
Summary:
In cases where a codebase is not UTF-8, we will attempt an conversion,
if an alternative encoding is given/configured.
This is now possible in two ways:
- by configuring one under repository tracking in diffusion
- by passing an --encoding option to the workflow
If the first is not available we will make a conduit call
to do an extra check and see if an encoding is configured directly with
phabricator.
Test Plan:
Tried various diffs with known encodings (mostly ISO-8859-1), and passed
it in, via stdin, or downloaded a known problematic revision from
phabricator, and they applied where they otherwise failed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D1880
Summary:
`svn rm $directory` removes the directory and its contents in svn 1.7, whereas
svn 1.6 only removes the directory's contents, so arc diff needs to not try
to read the directory.
Test Plan:
ran arc diff in a svn working copy that had a directory removed (with both
svn 1.6 and svn 1.7) and confirmed that the command did not throw an exception,
and that the removed directory was marked as directory (not a file).
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1889
Summary: @koolvin ran into this and was justifiably confused.
Test Plan: Put some random .php file in src/, ran arc liberate src/, got warned.
Reviewers: btrahan, Koolvin
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1878
Summary:
svn 1.7 reports a rev number of -1 for added files; previous versions
reported a rev number of 0. Interpret nonpositive rev numbers to mean
the file was added.
Test Plan: ran arc diff on an svn1.7 working copy with added files
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1867
Summary: Give "arc cover" --rev and paths parameters like "arc lint" and "arc unit".
Test Plan: Ran 'arc cover' with various rev/path things. Ran in SVN to verify it works/fails correctly.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T707
Differential Revision: https://secure.phabricator.com/D1860
Summary:
Make lint output look like gcc errors / grep output; a format used by
editors to jump from a compiler error to a file+line of code which
triggered the error.
Test Plan:
In vim, I ran:
:set makeprg=~/checkouts/devtools/arcanist/bin/arc\ lint\ --output\ compiler
Then:
:make
And was able to jump directly to problematic lines of code.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran, edwardspeyer, jungejason, codeblock, tuomaspelkonen, epriestley
Differential Revision: https://secure.phabricator.com/D550
Summary: `arc which` and related workflows have supplanted these now-unused mechanisms.
Test Plan: Grepped for chooseRevision() and DifferentialRevisionRef.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1841
Summary: Similar to rARC298ed9cda55d90b755e4dc45d202213e91631888, work around "differential.anonymous-access" issues until we get proper permissions up.
Test Plan: Ran "arc patch" against a "differential.anonymous-acccess" install with a revision argument
Reviewers: btrahan, mbautin
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T848
Differential Revision: https://secure.phabricator.com/D1835
Summary: good title
Test Plan: ran "arc patch DX" a bunch
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T892
Differential Revision: https://secure.phabricator.com/D1789
Summary: This may raise other types of exceptions, e.g. http/network exceptions, where getErrorCode() is not defined.
Test Plan: Reasoned about beahvior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1804
Summary:
`arc help` currently prints more than 500 lines and it is hardly usable without ##> arc.help##, ##| less##, ##| grep##, ##| wc -l## or such.
It is even worse with custom commands and options.
This diff changes `arc help` to only list commands with no description (under 100 lines).
It also adds `arc help --full` which maintains the original behavior.
It doesn't change `arc help <command>`.
NOTE: BC break on different levels.
Test Plan:
arc help
arc help --full
arc help help
arc help lint
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1769
Summary:
There's no reason this can't work now, we're just not quite careful enough about definitions.
NOTE: If you use a message which has a revision ID in it already, this will fail in an obtuse way when launching the update editor fails because stdin isn't a tty. I'll clean this all up after T614 is fully sorted.
Test Plan: Ran "arc diff --raw -C HEAD".
Reviewers: btrahan, yairlivne
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1765
Summary:
- When altering the include_path(), use PATH_SEPARATOR (";" on Windows, ":" elsewhere) instead of hard-coded ":".
- Detect missing php_curl.dll extension.
- Use APPDATA instead of HOME for storing .arcrc (the internet implies this is correct?)
- Don't try to do chmod() stuff on Windows; it's not critical and I don't want to figure out how it works.
Test Plan: Was able to run part of some arc commands on Windows.
Reviewers: btrahan, Makinde, Koolvin
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T124
Differential Revision: https://secure.phabricator.com/D1756
Summary:
Otherwise it says after hitting Tab everytime:
> Warning: Invalid argument supplied for foreach() in arcanist/src/workflow/shell-complete/ArcanistShellCompleteWorkflow.php on line 99
Test Plan: arc h<Tab>
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1766
Summary: Allow the user to pick a revision explicitly if they think they know what they're doing. Similar to "arc amend --revision", etc.
Test Plan: Obviously compells a meta-test.
Reviewers: btrahan, fratrik
Reviewed By: fratrik
CC: aran, epriestley
Maniphest Tasks: T928
Differential Revision: https://secure.phabricator.com/D1753
Summary:
When the user runs "arc diff --create" or triggers it via "arc diff
--auto", prefill the template as best we can.
Test Plan:
Ran "arc diff --auto" with a template commit message in the working
copy under various configurations, results seemed reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, dmi
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1719
Summary:
When a user invokes the update flow from Mercurial, prefill the update message
like we do for Git.
Also add a little caching to improve performance since "hg" execs cost ~100ms.
Test Plan: Updated a Mercurial diff repeatedly, got sensible default message
fills.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T28
Differential Revision: https://secure.phabricator.com/D1718
Summary:
Modified the arc diff workflow, introduced new diff properties -
arc:lint-excuse and arc:unit-excuse for respective errors/warnings.
The diff author inputs the explanation in an editor (similar to commit
message).
Task ID: #
Blame Rev:
Test Plan:
Ran sandbox arc on itself with some lint errors and verified that the
Diff property (== user explanation) is being set.
Revert Plan:
Tags:
Reviewers: epriestley, nh
CC: akramer, blair, aran, andreygoder, Girish, epriestley
Differential Revision: https://secure.phabricator.com/D1676
Summary:
When a Differential revision is updated in git, try to find all commit messages
in the range which we haven't already attached and combine them into a default
update message.
This won't work perfectly in a workflow where you rebase //and// stack local
commits, but worst case is that you just have to delete some lines, which is
still probably better overall than losing this information.
Test Plan: Meta-testing in progress.
Reviewers: btrahan, ptarjan
Reviewed By: btrahan
CC: aran, epriestley, davidreuss
Maniphest Tasks: T28
Differential Revision: https://secure.phabricator.com/D1671
Summary: Otherwise, "arc diff --auto" on a branch with the same name as some
other branch you previously used tries to update that revision.
Test Plan: Ran "arc diff --auto" on a "listdocs" branch; arc didn't try to
update D1639.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1662
Summary:
Allow users to easily define aliased arc commands. This is easier than making
them muck around with shell stuff, and lets me do stuff like "arc alias adiff
diff -- --auto" so I can test --auto more easily.
There are some limitations here (for example, you can't put --trace in an alias
because it gets parsed too early) but I think it's a reasonable starting point.
Test Plan: Set, listed, removed and used aliases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T896
Differential Revision: https://secure.phabricator.com/D1653
Summary:
Our failure behavior currently sucks. In particular:
- If lint or unit fail (expectedly or unexpectedly), we throw away your
message. omglol sux 2 b u
- If there's a parsing error, we dump the message to 'arc-commit-message' in
CWD and tell you to go deal with it.
This is awful. Improve the behavior:
- Once we read a message, save it in .arc/commit-message so we always have it
if anything goes wrong.
- If that file exists, prompt to read it without any "-F" nonsense.
- If there's a parsing error, tell the user and prompt them to just edit the
file again, showing what they need to fix.
Test Plan: Ran "arc diff --auto" and made a bunch of intentional errors; arc
never screwed me over and was generally fairly pleasant.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T614, T895
Differential Revision: https://secure.phabricator.com/D1656
Summary:
Our one Mercurial client doesn't use it (T614) and I moved git off to "arc
land".
This workflow can never work properly for Mercurial without extensions anyway
since it doesn't have --no-ff.
Test Plan: Ran "arc merge" in SVN, git and Mercurial repositories.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1650
Summary:
I think "arc land" is better than "arc merge" in every case? Make "arc merge"
hg-only (possibly nuke it later, see T614) and point users at "arc land".
Add an explicit "--merge" flag to force --no-ff behavior in mutable reposiories.
Test Plan:
Ran "arc land --hold --merge <feature>" on this branch, got a clean
merge.
Reviewers: fratrik, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1647
Summary:
For some workflows (all Mercurial, all SVN, some git) I eventually want arc to
pick between "arc --create" and "arc --update" automatically.
"arc which" laid the groundwork for this. For now, implement it as an optional
flag because I think there are still some rough edges on this pathway (for
example, handling of commit messages when errors happen).
When 'arc diff --auto' is invoked, guess whether the user meant --create or
--update.
Test Plan: Created this revision with 'arc diff --auto'.
Reviewers: Makinde, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1648
Summary: 'cuz the method would fail without authentication! see D1604 for some
discussion on approaches here
Test Plan: ran arc patch and it worked
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, johnduhart
Maniphest Tasks: T856
Differential Revision: https://secure.phabricator.com/D1645
Summary: T854 is hilarious. seriously, what are the odds?
Test Plan: ran arc patch a few times in a row with the same DX and verified
branches were made with expected, differing names
Reviewers: epriestley, fratrik
CC: aran, epriestley
Maniphest Tasks: T854
Differential Revision: https://secure.phabricator.com/D1605
Summary:
I want to use JSON which allows me to automatically open the files in editor.
I also want to apply patches, especially those modifying __init__.php.
I see no reason why these options should be mutually exclusive.
Also output nothing for okay result which is more standard and better parsable.
Test Plan:
arc lint --output json
arc lint --output json --never-apply-patches # same as above
arc lint --output json --apply-patches
Reviewers: aran, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1599
Summary: These are meaningless / unreachable after D1491 and nonsense after
D1582. Depends on D1579.
Test Plan: Grepped for 'sourcePath' references.
Reviewers: arice, btrahan
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T838
Differential Revision: https://secure.phabricator.com/D1584
Summary: Move toward deprecating "differntial.find". Also update the output to
be more inline with "arc branch".
Test Plan: Ran "arc list", "arc branch".
Reviewers: btrahan, arice
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T838
Differential Revision: https://secure.phabricator.com/D1579
Summary:
If you're on A and run "arc land B", run "git checkout A" after
everything's said and done.
Test Plan: Ran "arc land B" from A, got switched back to it.
Reviewers: davidreuss, kdeggelman, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1568
Summary:
- Renamed sanityCheckPatch to sanityCheck
- Move check for clean working copy into sanityCheck
- Execute sanityCheck before executing any other command
Test Plan:
- Run ##arc patch <revision id>## from a dirty working copy
and verify that a usage exception is thrown before the new branch is
created.
- Run ##arc patch <revision id> --force## and verify that it attempts
to create a new branch, apply, and commit the patch.
- Then clean your working copy and verify that ##arc patch <revision
id> works as expected
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T830, T479
Differential Revision: https://secure.phabricator.com/D1546
Summary:
- Show coverage for all files, not just files that have some coverage
information.
- Read file data off disk, under git the "current" data is the data at HEAD,
not the (possibly unstaged/uncommitted) file in the working copy which we
actually ran.
Test Plan: Ran "arc unit --detailed-coverage" on various files.
Reviewers: tuomaspelkonen, btrahan
Reviewed By: tuomaspelkonen
CC: aran, epriestley
Maniphest Tasks: T818
Differential Revision: https://secure.phabricator.com/D1542
Summary:
- Remove "new and experimental" because arc land is super awesome and great.
- Run the 'mark-committed' finalize workflow after pushing for untracked repo
setups.
Test Plan: Ran "arc mark-committed --quiet --finalize 123". Will run "arc land"
on this. so lazy
Reviewers: cpiro, btrahan
Reviewed By: cpiro
CC: aran, epriestley
Maniphest Tasks: T819
Differential Revision: https://secure.phabricator.com/D1538
Summary:
Mark all applicable Arcanist classes as "final", except PhutilLintEngine, which
needs a little finesse.
@jungejason / @nh, does this break any Facebook stuff?
Test Plan: Linter no longer raises warnings. Ran "testEverythingImplemented" in
Phabricator.
Reviewers: btrahan, jungejason, nh
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1519
Summary:
Check that the base revision is a valid git ref before trying to create a branch
starting at that rev. When arc patch is used in a git repo using the git-svn
bridge, the base reversion is a uri for the svn rev, not a git ref.
Test Plan:
run arc patch on a git-svn repo, run it on a pure git repo, and verify it works
for both
Reviewers: epriestley, btrahan
CC: jungejason, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1505
Summary: You can get this out of reflog, but we can easily just give you the
command in case you want to undo the -D.
Test Plan: Ran "arc land --hold" on this branch, used recovery command to
recover it.
Reviewers: cpiro, btrahan, jungejason
Reviewed By: cpiro
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1504
Summary: Better instructions in the 'git merge' failure case for 'arc land'.
Test Plan: no you test
Reviewers: fratrik, btrahan, jungejason
Reviewed By: fratrik
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1500
Summary:
- Use differential.query, not differential.find.
- Use loadWorkingCopyDifferentialRevisions ("arc which").
- Some general cleanup.
Test Plan:
oh man
$ arc commit --revision 999 # Does not exist
Usage Exception: Revision 'D999' does not exist.
$ arc commit --revision 1 # Exists, not accepted.
Revision 'D1: bleorp' has not been accepted. Commit this revision anyway?
[y/N] y
Committing 'D1: bleorp'...
A locally modified path is not included in this revision:
DERP
It will NOT be committed. Commit this revision anyway? [y/N] y
Done.
$ arc commit --revision 3 # Not mine, from a git repo
You are not the author of 'D3: bloop'. Commit this revision anyway? [y/N]
y
Revision 'D3: bloop' was generated from
'/INSECURE/repos/git-working-copy/', but current working copy root is
'/INSECURE/repos/svn-working-copy/'. Commit this revision anyway? [y/N] y
Committing 'D3: bloop'...
A locally modified path is not included in this revision:
DERP
It will NOT be committed. Commit this revision anyway? [y/N] y
svn: Commit failed (details follow):
svn: '/INSECURE/repos/svn-working-copy/derp' is not under version control
Exception:
Executing 'svn commit' failed!
(Run with --trace for a full exception trace.)
$ arc commit # Nothing accepted
Usage Exception: Unable to identify the revision in the working copy. Use
'--revision <revision_id>' to select a revision.
$ arc commit # Now accepted
Committing 'D1: bleorp'...
Marking revision D1 'bleorp' committed...
Done.
$ svn st # Complicated test for a bizarre SVN edge case
A svnderp
A svnderp/A
A svnderp/B
$ arc diff --create
Linting...
LINT OKAY No lint problems.
Running unit tests...
No unit test engine is configured for this project.
Created a new Differential revision:
Revision URI: http://local.aphront.com/D28
Included changes:
A (dir) svnderp
A svnderp/A
A svnderp/B
$ touch svnderp/C
$ svn add svnderp/C
A svnderp/C
$ arc commit
Usage Exception: Unable to identify the revision in the working copy. Use
'--revision <revision_id>' to select a revision.
$ arc commit --revision 28
Revision 'D28: derp' has not been accepted. Commit this revision anyway?
[y/N] y
Committing 'D28: derp'...
Usage Exception: This commit includes the directory 'svnderp', but it contains
a modified path ('svnderp/C') which is NOT included in the commit. Subversion
can not handle this operation and will commit the path anyway. You need to sort
out the working copy changes to 'svnderp/C' before you may proceed with the
commit.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1491
Summary: I removed the "--interactive" flag since it makes no sense with 'arc
merge --squash', and transposed a space.
Test Plan: Read text.
Reviewers: cpiro, btrahan, davidreuss
Reviewed By: cpiro
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1493
Summary:
This is a fancy version of "land.sh" that uses "git merge --squash" and
"arc which" to cover more cases.
Test Plan:
Ran "arc land" against various repository states (no such branch, not
accepted, valid, etc). Things seemed OK. There are basically an infinite number
of states here so it's hard to test exhaustively.
Reviewers: cpiro, btrahan, jungejason, davidreuss
Reviewed By: davidreuss
CC: zeeg, aran, epriestley, davidreuss
Maniphest Tasks: T787, T723
Differential Revision: https://secure.phabricator.com/D1488
Summary:
- Allow 'arc amend' to identify revisions by hash and branch, so it works with
"arc diff --create".
- Warn when the requested revision is not (apparently) in the working copy.
Test Plan:
Ran "arc amend" in working copies with different states (right
revision, wrong revision, hash/branch identification).
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T723
Differential Revision: https://secure.phabricator.com/D1480
ArcanistRepositoryAPI->loadWorkingCopyDifferentialRevisions()
Summary:
- See T787.
- @cpiro has an immediate use case for this, which is ##arc amend --revision
`arc which --id` --show master## for "git merge --autosquash" or similar.
- For T614, we need this to choose "--create" vs "--update".
- Other workflows should also use this to improve how often we automatically
get things right, particularly in Mercurial and SVN.
Test Plan:
Ran "arc which" in SVN, Git and HG working copies with various flags;
results seemed reasonable.
Reviewers: btrahan, cpiro, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T787
Differential Revision: https://secure.phabricator.com/D1478
Summary: ...basically by adding "getConduitURI" and then falling through to
that, rather than repeating work from the main arc wrapper that setConduitURI in
the first place. The explicit drawback here is the error message gets a little
more vague.
Test Plan:
- arc install-certifate --conduit-uri=https://secure.phabricator.com
// verified that the install flow was going for
https://secure.phabricator.com...!
- arc install-certificate https://secure.phabricator.com
// verified that the install flow was going for
https://secure.phabricator.com...!
- arc install-certificate
// verified that it reverted back to the .arcconfig conduit uri
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T794
Differential Revision: https://secure.phabricator.com/D1468
Summary:
under git, we now create a branch that is at the patch's base revision if we
know it or whatever the working copy happened to be at if we don't. This diff
also adds the --nobranch flag to disable this new behavior.
Also added an --update flag. When specified, we run the appropriate "update"
command in the VCS. By default this is off.
Finally, tried to give the user more information about what the heck arc just
did to their working copy.
Test Plan:
// verify --update flag works
// -- git
Assume we are at HEAD and we got to HEAD from HEAD^1 via DX
git reset --hard <THE BEGINNING>
arc patch --update DX
// ...versus svn
Assume we are at HEAD and we got to HEAD from HEAD^1 via DX
svn checkout -r 1
arc patch --update DX
// ...versus hg
Assume we are at HEAD and we got to HEAD from HEAD^1 via DX
hg update -r 1
arc patch --update DX
// verify under git a nice branch is made
// -- test where we should get a good name
// -- test where we have a base revision to check out the branch at
Assume we are at HEAD and we got to HEAD from HEAD^1 via DX
git reset --hard HEAD^1
arc patch DX
// verify under git an "okay" branch is made if we can't get "nice"
// -- test where we should get a "bad" name
// -- test where we DON'T have a base revision to check out the branch at
git diff HEAD^1 > ~/example.patch
git reset --hard HEAD^1
arc patch --patch ~/example.patch
// verify --nobranch flag skips the test for git
Assume we are at HEAD and we got to HEAD from HEAD^1 via DX
git reset --head HEAD^1
arc patch --nobranch DX
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D1459
Summary:
without "nocommit" we commit the patch to the working copy. add the nocommit
flag and this commit does not happen.
making this happen required adding revisionID to arc bundle to fetch the proper
commit message. if we can't get a commit message -- suppose the fetch fails or
the source is self::SOURCE_PATCH, we ask the user for the commit message on the
command line
Test Plan:
git reset --hard <SOME_REV>
arc patch DX, where this got us to <SOME_REV + 1>
observe correct patch committed with correct commit message in working copy
git reset --hard <SOME_REV>
arc patch --nocommit DX, where this got us to <SOME_REV + 1>
observe correct patch landed BUT NOT committed. note "commit message" does not
exist since there isn't a commit...!
git diff HEAD^1 > ~/file.patch
git reset --hard HEAD^1
arc patch --patch ~/file.patch
observe prompted for commit message and patch committed with commit message i
typed in
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D1450
Summary:
pretty easy stuff as mercurial accepts git style patches...!
also fixed two issues where we were 1) storing the short hash and 2) storing it
with a trailing "\n". This diff makes us store the full hash AND no trailing
return character
Test Plan:
in my mercurial repo
<note repo at revision $foo>
<did something dumb>
hg commit -m "something dumb"
arc diff
<go to web and fill out stuff for DX>
hg checkout $foo
arc patch DX
<verify patch DX successfully applied!>
use conduit console to verify a few diffs were returning the correct full
revision hash with a trailing \n
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T630
Differential Revision: https://secure.phabricator.com/D1431
Summary:
Extended the install-certificate workflow's timeout from 5
seconds to rely on the ConduitClient default (30 seconds), which matches the HTTP interface.
Test Plan: Run arc install-certificate
Reviewers: epriestley
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D1404
Summary:
Currently, we throw a fairly perplexing error when there are multiple valid
commit messages. Installs can also remove the "test plan" field entirely, which
is the only really strong discriminator here.
When the message to use is ambiguous, show the user all the valid messages and
prompt them to choose one.
Also add a -C flag like "git commit -C", so they can choose a message
explicitly.
Test Plan: Ran "arc diff HEAD^^^^^", "arc diff -C <rev>".
Reviewers: cpiro, btrahan, jungejason
Reviewed By: cpiro
CC: aran, cpiro
Differential Revision: https://secure.phabricator.com/D1385
revision is the correct base revision relative to the patch.
Summary: What the title says. If not correct, warn the user. This check
honors the --force flag to skip all these checks. This change also includes
moving some Differential constants into Arc so they can be used for both
projects. There is a corresponding phabricator diff (incoming) to address this
part of the change.
Test Plan:
For a project with actual diffs, a git repository tracked by phabricator, *AND*
development in master branch only, do some...
- git reset --hard HEAD^1
- arc patch DX, where X is what got us to HEAD in the first place
- verify successful patch
...then...
- git reset --hard HEAD^^
- arc patch DX, where X is what got us to HEAD in the first place
- verify warning
- verify Y versus N continues versus stops appropriately
Note if development were done outside the master branch this warning message
will fire early / often as git commit hashes are based on the commit *and* the
rest of the source code the commit is made against. This is (unfortunately) the
"typical" case so this warning is pretty active at the moment. T201 will
eventually land and when parsing a given commit update the corresponding diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1328
Summary: See T645. These commands take inconsistent and overly-magical arguments
right now. Instead, make them behave consistently and allow them both to operate
on "arc <workflow> path path2 path3 ...", which is a generally useful workflow.
Test Plan: Ran "arc lint <path>", "arc unit <path>", "arc lint --rev
HEAD^^^^^^", "arc unit --rev HEAD^^^^^^^^^^^^", etc. Ran "arc diff --trace" and
verified --rev argument to child workflows.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T645
Differential Revision: https://secure.phabricator.com/D1348
Summary:
See T614. This flag explicitly tells Arcanist to use the message for an existing
revision, and attach the change to it directly.
Next step is to have "arc diff" automatically choose "--create" or "--update" in
the absence of "--create", "--update", "--only", "--preview" or a template
commit message.
Test Plan:
Ran "arc diff --update <n>" for various revisions. Got updates or
errors as expected.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, jungejason
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1346
Summary:
Some Mercurial users at Dropbox have very specific diff preparation
needs. Allow "arc" to read an arbitrary diff off stdin. This disables most
features.
Test Plan:
Ran "git diff HEAD | arc diff --raw", "git show | arc diff --raw",
"hg diff --rev 8 | arc diff --raw".
Reviewers: btrahan, jungejason, Makinde
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T617
Differential Revision: https://secure.phabricator.com/D1323
Summary:
I want to make some changes to this workflow but it a huge mess right now. Try
to refactor a bit to make it a little more manageable.
Broadly:
- Moved lint/unit constant mapping into separate methods.
- Moved lint/unit/local properties into separate methods.
- Moved diff spec construction into a separate method.
- Moved some message stuff into separate methods and reorganized related
methods near to one another.
- Removed an unused findRevisionInformation() method.
I fixed a couple of small bugs, too:
- --create now conflicts with --only and --preview.
- --create now probably works in Mercurial.
- --create messages now have basic reviewer validation.
This should have not have any significant behavioral changes.
Test Plan:
- Created this revision.
- Ran "arc diff --create".
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1320
Test Plan:
Run ##arc lint## after changing a binary file
"Warning: Invalid argument supplied for foreach() in
src/workflow/cover/ArcanistCoverWorkflow.php on line 88" should not be displayed
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1314
Summary:
Creates a new hook API that can be used to interface with
SVN/Git/Mercurial in the context of a commit hook. Currently only adds a
function to read the modified file data in a Subversion commit hook.
An object of this API is created in the SvnHookPreCommitWorkflow and
passed on the Lint Engine which then uses it to access current file
data, of the way the APIs seem to be structured); linters use the
getData function which is essentially a wrapper around the engine's
call, with another layer of caching.
Task ID: #770556
Blame Rev:
Test Plan:
- Create a local svn repository and add a minimal hook to run the local
version of arc to test commits
(http://phabricator.com/docs/arcanist/article/Installing_Arcanist_SVN_Hooks.html)
- Create a temporary repository that can trigger any of the linters
available, and test against a temporary linter by committing against
the test repository: the linter should be able to access all required
files by using loadData/getData in the LintEngine and Linter.
Revert Plan:
Tags: lint, svn-hook-pre-commit
Reviewers: jungejason, asukhachev, epriestley, aran
Reviewed By: epriestley
CC: aran, jungejason, epriestley, kunalb, asukhachev
Differential Revision: https://secure.phabricator.com/D1256
Summary:
in arcanist we are using $_SERVER['PWD'], but in some cases it
is not returning the correct result. For example, when I'm using
PhpStorm (an php IDE) to launch the script, $_SERVER['PWD'] returns the
path of the binary of the IDE:
/home/jungejason/tools/PhpStorm-107.658/bin, not the path where arcanist
is at. getcwd() returns the correct value.
One difference between getcwd() and $_SERVER['PWD'] is that getcwd()
resolves symlink where $_SERVER['PWD'] does not. From what I can see,
using realpath should work.
Test Plan:
* ran arcanist as normal and it worked;
* run arcanist in PhpStorm and it worked.
* created a symlink pointing to the repository inside which
I ran the arc command, and it worked.
* created a symlink pointing to arcanist project, run `.
* resources/shell/bash-completion` and it worked
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1285
Summary:
Thinking about this, I think it's worthwhile to bump versions for T1249/T1250.
The problem is that if you have an old Arcanist, trying to update diffs against
a new Phabricator will create new diffs instead, and it probably won't be
obvious what's wrong.
Bump the versions so users will get a message like "oh, hey, you should
upgrade".
Test Plan:
- Tried to diff against a mismatched version.
- Diffed against a matching version.
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: aran
Differential Revision: https://secure.phabricator.com/D1257
Summary: We fatal confusingly if you specify a valid class that isn't of the
right subclass (like a linter rather than a lint engine). Improve the error
message.
Test Plan:
- Ran "arc lint --engine PhutilSymbolLoader", "arc unit --engine
PhutilSymbolLoader", got expected failures.
- Ran "arc diff" normally without errors.
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: aran
Differential Revision: https://secure.phabricator.com/D1259
Summary:
- Allow Arcanist to parse either "123", "D123" (existing behaviors) or
"http://phabricator.example.com/D123" (new behavior) values.
- Drop support for labeling this field "DiffCamp". This should only impact
people trying to update revisions that are more than ~a year old, which should
be very very few.
Test Plan: - Ran "arc diff" with values "74", "D74", "x74",
"http://local.aphront.com/D74", "http://local.aphront.com/x74". Got the expected
behaviors.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T54, T692
Differential Revision: 1249
Summary: --output json will be used for scripts, so support script writers a
little better.
Test Plan:
arc lint [--output {json, summary}]
arc lint --output json {--apply-patches, --never-apply-patches}
Reviewers: epriestley
Reviewed By: epriestley
CC: jack, aran, epriestley
Maniphest Tasks: T675
Differential Revision: 1220
Summary:
--revision doesn't allow the revision to be prefixed with 'D'. Also the error
message showed when specified revision doesn't exist is hard to understand.
Test Plan:
Used `arc amend --revision D123`, tried it without 'D' and with a non existing
revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1193
Summary: This adds parent directories of modified files to commit paths if
needed.
Test Plan:
Used https://reviews.facebook.net/D603 patch on Apache Hive repository, called
`arc commit` and SVN didn't complain about missing paths.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T667
Differential Revision: 1189
Summary:
See T614. This adds a "--create" flag which I think works properly, but doesn't
make it the default.
Once I add "--update" and am confident the flags actually work, I'll work on
some heuristics to make "arc diff" automatically choose "--udpate" or "--create"
as per T614.
Test Plan:
This revision was created with "--create" and a bogus commit message ("derp").
I intentionally goofed the message at first to test the fail + file workflow.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Differential Revision: 1162
Summary:
Commit templates are fully configurable on the server now, so we should be doing
validation there, since an install can add or remove fields and change
validation rules. Remove these outdated client validations, as per comment.
Also update the API call to use the 'errors' field, which allows us to show the
user all the parse errors at once. See:
https://secure.phabricator.com/diffusion/P/browse/origin:master/src/applications/conduit/method/differential/parsecommitmessage/ConduitAPI_differential_parsecommitmessage_Method.php;cfaab709df37739b$75
Test Plan:
Ran "arc diff" on a change with multiple errors:
```$ arc diff --conduit-uri=http://local.aphront.com/
Usage Exception: Commit message is not properly formatted:
Error parsing field 'Reviewers': Commit message references nonexistent users:
jjjderp.
Error parsing field 'CC': Commit message references nonexistent users and
mailing lists: jjjderp.
You should use the standard git commit template to provide a commit message. If
you only want to create a diff (not a revision), use --preview to ignore commit
messages.```
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1154
Summary: adds a little bit of sanity checking to the arc patch workflow. in
short, if the working copy project is not the same as the patch project, don't
apply the patch
Test Plan:
ran
arc patch DX
arc patch DX --force
in the top line directory for project A and proejct B. DX is for project A.
verified for project A that the patch was applied and for project B i was issued
warnings as expected. also verified in project B case that saying Y or N to the
warning had the desired effect.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1140
Summary:
If any hunks is detected as non-utf8, and you've never submitted diffs
for a certain project before, you would get a ERR-BAD-ARCANIST-PROJECT
exception. This makes it possible to submit the patch properly, so you
can set the encoding in the interface afterwards. Further this fixes
cases where you don't supply a diff but will result in hunks getting
treated as binary, but that still beats the exception behaviour.
Test Plan:
Ran `arc diff` with and without the new --encoding param and
got the expected results. Also ensured the diff (with non utf-8 hunks)
would be properly created even when no encoding is specified.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1135
Summary:
arc unit supports paths parameter
It is not documented in arc help
Test Plan:
arc help
Search for unit
Reviewers: slawekbiel, aran
Reviewed By: aran
CC: aran, vrana
Differential Revision: 1143
Summary:
Git works completely differently for commits zero and one than for 2..N so add
more special casing to handle them. See:
- {T206}
- {T596}
The getCommitRange() block is also fatal land, although I wasn't able to reach
it. I'll follow up with @s on T596.
Test Plan:
- Created a new, empty repository ("mkdir x; cd x; git init").
- Ran "arc lint", "arc unit", "arc diff" against it with no commits (the first
two work, the third fails helpfully).
- Made an initial commit.
- Ran "arc lint", "arc unit", "arc diff" against it (all work correctly).
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: s, aran
Differential Revision: 1142
Test Plan:
Wrote an event listener modifying the commit message and the message was
successfully changed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 1103
Summary:
just a little try / catch action in ArcanistDiffWorkflow.
"Ideally, it would be good to flag these changes somehow so that "arc patch" can
issue the inverse warning (e.g., "This patch could not be completely applied
because some binary data was not uploaded.") but that hasn't come up in a real
use case yet."
I think a change w/ filetype of FILE_IMAGE || FILE_BINARY *and* no phid means
that the upload failed so there's no additional flag needed. (True?) however,
it would be easy enough to store metadata that explicilty stated whether or not
the file upload succeeded.
(also / related - i looked through the arc patch workflow a bit and i don't
understand how the svn codepath loads up the actual binary files... for git,
toGitPatch => buildBinaryChange => getBlob is the right path )
Test Plan:
- set 'storage.mysql-engine.max-size' to 0 in my conf, uploaded diffs with
files
- noted in differential that it correctly detected images versus binary despite
file upload failing
- noted in differential that images had some empty UI
- reverted conf change, uploaded diffs with files
- noted in differential file showed up
- ran arc patch with DX, where DX had broken files
- noted "Downloading binary data...
Exception:
ERR-BAD-PHID: No such file exists."
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1100
Summary:
If arc amend is run on a rev that isn't accepted, it runs arc mark-committed
--finalize. The rev shouldn't be marked as committed if it hasn't been accepted,
so this diff adds in that check.
Test Plan:
Ran arc amend on a rev that hasn't been accepted, checked that it didn't get
marked as completed.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, epriestley, nh
Differential Revision: 1104
Summary:
Adds a secret, undoucmented "encoding" key to ".arcconfig" which makes a very
half-hearted effort to convert encodings. This is probably good enough that
Differential can be used for code review, but there will be issues with 'arc
patch', 'arc export', paste, maybe conduit stuff, Diffusion, and whatever else I
haven't thought of.
This also doesn't store the original encoding so anything converted like this
won't reasonably be able to be made to work with all that stuff in the future.
See T452 for a broader discussion of the issues involved.
Test Plan:
Short circuited the UTF-8 detection to always fail, had my files "converted"
from ISO-8859-1 to UTF-8.
@davidreuss: you can test this by applying this patch to arcanist/, adding
'"encoding" : "ISO-8859-1"' to your .arcconfig, touching some non-ASCII file,
and then running "arc diff".
Reviewers: davidreuss, jungejason, tuomaspelkonen, aran
Reviewed By: davidreuss
CC: aran, davidreuss, epriestley, nshamg123
Differential Revision: 812
Summary:
Save commited revision id in workflow, so it can be used in Arcanist
didRunWorkflow hook.
Test Plan:
Running getRevisionID on the workflow should return id of the committed
revision.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley, mareksapota
Differential Revision: 1078
Summary:
D1061 introduced a 'text file' check, but it fails under SVN for new
directories.
- Revert D1061 (This reverts commit b2cd18252701be2093b52652fb3d1d94c5df571e.)
- Make getChangedLines() return null to indicate that the operation doesn't
make sense. I think this was the intent of the code in the lint engine.
- Fix a bug where running "arc lint" on a change in an SVN working copy from a
subdirectory would fail.
- Fix a bug where warnings with no line information were incorrectly
discarded.
Test Plan:
- Ran "arc lint" in an SVN working copy with a new directory (no failure).
- Forced FilenameLinter to always raise a warning. Added a binary file and ran
"arc lint". The warning was reported for the new binary file, a new text file,
and a new directory.
Reviewers: jungejason, andrewjcg, nh, tuomaspelkonen, aran
Reviewed By: andrewjcg
CC: aran, andrewjcg, epriestley
Differential Revision: 1076
Summary:
Allow `arc patch` without authentication if Phabricator instance has
'differential.anonymous-access' set to true.
Test Plan:
Set 'differential.anonymous-access' in Phabricator to true and run `arc patch`
without installing a certificate. `arc patch` should work as expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1069
Summary:
Currently, lint messages are only included if they are errors or
if they affect lines which the diff changed. The implementation
of this caused issues for non-text files (e.g. binaries), as line
change information is not available, and the corresponding lint
messages were dropped (for non-errors).
In this diff, only lint messages concerning text files are dropped
based on this line filtering.
Test Plan: arc lint with binary file
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, aravindn, andrewjcg, liat, epriestley
Differential Revision: 1061
Test Plan:
Using SVN make some changes to the repo, run `arc diff`. As other user accept
the revision, add the changes to your repo using `arc patch` and then run `arc
commit --revision revisionID` to commit them. Arcanist should ask if you are
sure that you want to commit this revision and if you answer `Y` it should
commit to SNV repo.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, mareksapota, epriestley
Differential Revision: 1055
Test Plan:
Run `arc mark-committed` on a revision that you don't own, you should see a
prompt asking you if you really want to do that and if you answer `Y` it should
mark the revision as committed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota
Differential Revision: 1052
Summary: See T587. Reduce the strictness of working copy checks when using
"--show", since there's a reasonable workflow where you 'arc patch' and then
'arc amend --revision X --show | git commit -a -F -' that currently won't work.
There are other ways to accomplish the same thing but this increases flexibility
overall.
Test Plan: Ran 'arc amend --show' with a dirty working copy, didn't get yelled
at.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1033
Summary:
D935 missed one place of parseGitRelativeCommit() in
ArcanistExportWorkflow.
Test Plan: ran arc export and verify that it worked.
Reviewers: epriestley, tuomaspelkonen, aran
Reviewed By: aran
CC: aran
Differential Revision: 1015
Summary:
See D945. We have this kludgy "remote_hooks_installed" mess right now, but we
have enough information on the server nowadays to figure this out without it.
Also reduce code duplication by sharing the "mark-committed" workflow.
This causes "arc merge" to effect commit marks.
Test Plan:
In Git, Mercurial and SVN working copies ran like a million
amend/merge/commit/mark-committed commands with and without --finalize in
various states of revision completion.
This change is really hard to exhaustively test because of the number of
combinations of VCS, revision state, command, command flags, repository state
and tracking state. All the reasonable tests I could come up with worked
correctly, though.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 967
Summary: When a project uses a conservative history mutability doctrine, never
try to amend history.
Test Plan: Ran "arc amend" in an "immutable_history" working copy.
Reviewers: fratrik, Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde, epriestley
Differential Revision: 862
Summary:
This should support conservative rewrite policies in git fairly well, under an
assumed workflow of:
- Develop in local branches, never rewrite history.
- Commit with "-m" or by typing a brief, non-template commit message
describing the checkpoint.
- Provide rich information in the web console (reviewers, etc.)
- Finalize with "git checkout master && arc merge branch && git push" or some
flavor thereof.
This supports Mercurial somewhat. The major problem is that "hg merge" fails if
the local is a fastforward of the remote, at which point there's nowhere we can
throw the commit message. Oh well. Just push it and we'll do our best to link
them up based on local commit info.
I am increasingly forming an opinion that Mercurial is "saftey-scissors git".
But also maybe I have no clue what I'm doing. I just don't understand why anyone
would think it's a good idea to have a trunk consisting of ~50% known-broken
revisions, random checkpoint parts, whitespace changes, typo fixes, etc. If you
use git with branching you can avoid this by making a trunk out of merges or
with rebase/amend, but there seems to be no way to have "one commit = one idea"
in any real sense in Mercurial.
Test Plan: Execute "arc merge" in git and mercurial.
Reviewers: fratrik, Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, epriestley, Makinde
Differential Revision: 860
Summary:
We have some git-specific logic on main pathways which should be in the API
class, move it around so "arc lint" with an engine works under Mercurial. This
resovles the error @makinde reported:
> PHP Catchable fatal error: Argument 1 passed to
ArcanistBaseWorkflow::parseGitRelativeCommit() must be an instance of
ArcanistGitAPI, instance of ArcanistMercurialAPI given, called in
/home/makinde/.arc_install/arcanist/src/workflow/lint/ArcanistLintWorkflow.php
on line 131 and defined in
/home/makinde/.arc_install/arcanist/src/workflow/base/ArcanistBaseWorkflow.php
on line 830
Test Plan: Ran "arc diff" in git and hg working copies, plus "arc lint" with a
configured "lint_engine".
Reviewers: Makinde, aran, jungejason, nh, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 935
Summary:
1) unit engine getter method in the unit workflow
we store some information (unit test results) in the engine that we
need to access in the "arc unit" post hook
2) make requireCleanWorkingCopy() public
"arc unit" doesn't need to be clean in general, but we'd like the
option to upgrade the workflow to require it if necessary. We use
this for ensuring a repo is clean before updating unit test
results.
Test Plan: Used both features in a custom post hook in arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, epriestley
Differential Revision: 917
Summary: When a revision is created, attach relevant information about the local
commits which it came from if applicable. This supports T473, for DCVSes and
DCVS workflows with immutable history where we can't just amend commit messages.
It will also allow us to enrich the web interface.
Test Plan: Will verify this info shows up for this very diff.
Reviewers: fratrik, aran, jungejason, tuomaspelkonen
Reviewed By: fratrik
CC: aran, epriestley, fratrik
Differential Revision: 857
Summary:
- Build the manifest of file changes so unit and lint workflows work.
- Default to creating a diff between the parent of the first outgoing change
and the tip.
Test Plan:
- Ran "arc diff" in a dirty mercurial repo, got warned about
untracked/uncommitted changes.
- Ran "arc diff" in a clean mercurial repo, got a diff of everything I'd done
locally.
Reviewed By: aran
Reviewers: Makinde, aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 796
Summary:
There's a lot of ground left to cover but this makes "arc diff" work (on one
trivial diff) in my sandbox, at least, and supports parsing of Mercurial native
diffs (which are unified + a custom header). Piles of missing features, still.
Some of this is blocked by me not understanding the mercurial model well yet.
This is also a really good opportunity for cleanup (especially, reducing the
level of "instanceof" in the diff workflow), I'll try to do a bunch of that in
followup diffs.
Test Plan: Ran "arc diff" in a mercurial repository, got a diff out of it.
Reviewed By: aran
Reviewers: Makinde, jungejason, tuomaspelkonen, aran, codeblock
CC: aran, epriestley, codeblock, fratrik
Differential Revision: 792
Summary: This stopped being available in scope when I refactored this
recentlyish.
Test Plan: Got error, saw useful message.
Reviewed By: jungejason
Reviewers: mgummelt, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 787
Summary:
Requires
https://secure.phabricator.com/rPHU0acf708b9b0fdbf59e4399f14dd8295b6a96972c
from libphutil, which allows a backslash prefix to control escape
sequences such as bold, underline, and invert.
Test Plan: "arc help liberate"
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 767
Summary:
- Provide a "--json" flag for "arc upload"
- Unify some of the stderr stuff across upload/download/paste.
Test Plan:
- Ran "arc upload" and "arc upload --json", piped stderr away with 2>/dev/null
to verify only JSON got emitted to stdout
- Ran "arc paste"
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock
Differential Revision: 749
Summary: Read and write in the same workflow! Dogs and cats living together!
Test Plan: - Performed a bunch of paste reads and writes and they looked ok?
Reviewed By: aran
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 748
Summary: Mechanisms for interacting with Files via Arcanist.
Test Plan:
- Ran 'arc upload x', 'arc upload x y z'
- Ran 'arc download' with --as and --show.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock, epriestley
Differential Revision: 742
Summary:
remove the size check. The conduit call will fail later if the
size is too large, and we will get better error message.
Test Plan:
run arc diff with files smaller and larger than the size
limit.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 729
Summary: One day all "GUID" references will be gone, maybe.
Test Plan: grep, ran workflow before changing and got a warning about
deprecation
Reviewed By: aran
Reviewers: mgummelt, tuomaspelkonen, aran, jungejason
CC: aran
Differential Revision: 671
Summary:
The primary goal of this is to allow pre/post workflow hooks to upgrade a
workflow which doesn't require conduit into one which does, or one which doesn't
require authentication into one which does. They do this by calling
$workflow->establishConduit() or $workflow->authenticateConduit() respectively.
It also removes a bunch of dead code and a bunch of now-unnecessary public
interfaces.
Test Plan:
Broke my certificate and ran "arc list", "arc unit", "arc help", "arc
call-conduit".
Restored my certificate and re-ran the commands.
Reviewed By: mgummelt
Reviewers: mgummelt, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, mgummelt
Differential Revision: 664
Summary: This allows us to detect and complain about mismatched client and
server host identities. It causes some subtle and not-so-subtle problems if the
client and server don't agree on the install's primary URI.
Test Plan: Ran "arc install-certificate" and "arc list" against my local host
with intentionally bogus configs and was warned. Ran with normal configs and
everything worked.
Reviewed By: tuomaspelkonen
Reviewers: jungejason, llorca, tuomaspelkonen, aran
CC: aran, tuomaspelkonen
Differential Revision: 591
Summary:
Keeping unit tests speedy keeps them useful since people actually won't mind
running them. This diff records the time taken by each test and displays it nice
and colorized. Really, I just want to discourage non-unit tests from making
their way into ##__tests__##.
Some thoughts:
- The "acceptableness" times are subjective but if dependencies are properly
mocked the times seem to be ok. Integration tests that make network requests to
third-party endpoints and pull in megabytes of data will not survive. This is a
good thing.
- Fast tests get a gold star, encouraging small tests. I am sorry that the star
does not sparkle.
- There is no way for a programmer to admit that their test is going to be slow
in some cases. They will be shamed with red text for the life of their test.
- It might be confusing that fast but failing tests get green text and maybe a
gold star.
Test Plan: Ran some of the unit tests within Arcanist and libphutil. See
https://secure.phabricator.com/file/view/PHID-FILE-cdd3c94c219e0fd7470b/ for
sample output.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 588
Summary:
We do this check on the web interface but not from the CLI.
Also clean up some GUID/PHID stuff (eventually all GUID references should be
replaced with PHID, although they mean the same thing).
Test Plan: Tried to create a revision with myself on the reviewer line, got
called out.
Reviewed By: jungejason
Reviewers: moskov, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 564
Summary:
If you checkout some commit you end up on "(no branch)" which currently breaks
the parser. Ignore that, and add revision IDs to the output. They aren't very
big and I hate flags so I didn't add a flag for this. You can add a flag to turn
them off if you really want.
Test Plan:
Ran "arc branch" and "arc branch --by-status" from a "(no branch)" working copy.
Reviewed By: slawekbiel
Reviewers: slawekbiel, ahupp, jungejason, tuomaspelkonen, aran, schrockn
CC: aran, slawekbiel
Differential Revision: 555
Summary:
This diff adds a '--by-status' argument to arc branch that sorts the
output by status. Example output:
Accepted
my-branch Amazing change
Needs Revision
nah-nah Not so good change
Needs Review
in-progress-change I have no idea
No Revision
etc etc
Blame Rev:
Task ID: #
Reviewers: epriestley, slawekbiel
Test Plan:
Ran it with and without --by-status, saw expected output in both cases.
DiffCamp Revision:
Revert Plan:
Database Impact:
Memcache Impact:
Other Notes:
EImportant:
- begin *PUBLIC* platform impact section -
Bugzilla: #
- end platform impact -
Differential Revision: 512
Summary:
Currently, when arc-lint processes lint warnings and errors, it
accumulates the warnings into an "unresolvedMessages" array. As
soon as it sees an errors, it breaks out of the loop that does the
collection and returns, causing individual lint errors messages to
not show up in differential.
This diff collects all lint warning and error messages into the
unresolved array.
Test Plan:
arc-diff on patch that had lint errors. Verified that,
after this change, linte errors messages showed in differential.
Reviewed By: epriestley
Reviewers: jungejason, epriestley
Commenters: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 543
Summary:
Since this has auth information in it now, we should prevent other users on the
system from reading it. Detect readable files and prompt the user to fix them.
Test Plan:
Did "o+r" on my ~/.arcrc, ran "arc list", got prompted, hit "Y", verified it set
perms to 600, ran "arc list" again and wasn't prompted.
Reviewed By: jungejason
Reviewers: fratrik, jungejason, aran, tuomaspelkonen
CC: aran, jungejason, epriestley
Differential Revision: 532
Summary:
This should be implemented more elegantly, but this is a mostly-reasonable
attempt at it.
Test Plan:
Ran 'arc diff --only --json' with this diff.
Reviewed By: gc3
Reviewers: gc3
CC: aran, epriestley, gc3
Differential Revision: 509
Summary:
This documentation isn't terribly clear and it isn't obvious how to use the
workflow. Make it more clear and provide examples.
Test Plan:
Ran "arc help call-conduit"
Reviewed By: gc3
Reviewers: gc3, aran, jungejason
CC: aran, gc3, epriestley
Differential Revision: 502
Summary:
Appending differential status, sorting, filtering and coloring git
branches.
I think it turned out rather nicely. On my repository with 70 branches
it takes 1.6s, not terrible, though 1.2s is in the conduit call - seems
like there is potential for optimization.
I didn't end up changing 'arc list', as their semmantics are slightly
different, but I'm open to ideas of consolidating them
Test Plan:
- Tested on both facebook www and arcanist repositories.
- Validated that view-all flag works
- Validated that the ordering is correct
- Validated that the statuses match the differential status.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley, slawekbiel
Revert Plan:
sure
Other Notes:
Differential Revision: 497
Summary:
When installing a certificate, it's being written to ~/.arcrc not ~/.arcconfig
Test Plan:
Installed a certificate it said the right thing.
Reviewed By: aran
Reviewers: epriestley, aran
Commenters: epriestley
CC: aran, epriestley
Differential Revision: 491
Summary:
Provide an "install-certificate" workflow to simplify ~/.arcrc edits. See also
D460.
Test Plan:
Installed certificates via "arc install-certificate".
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 465
Summary:
I looked at this quickly to see what was involved and there were only a couple
of issues so here's a patch:
- On OSX, the "-i" flag does not mean "--mime". Use "--mime" explicitly
instead. This is a minor fix which affects only OS X.
- I wasn't able to repro the "crazy executables" behavior and think it might
have been me messing something up, so nuke it until we see an issue.
- Some guid vs phid shenanigans. Differential reads "phid" but arcanist set
"guid". We should move toward "phid"; I started using "guid" before I realized
it was an overloaded term that also refers to a specific GUID implementation
(Microsoft's UUID).
Test Plan:
Uploaded an image diff, saw images in Differential.
Reviewed By: aran
Reviewers: jianfeng, jungejason, aran, tuomaspelkonen
CC: aran
Differential Revision: 466
commit template
Summary:
When users run into this, point them at the documentation explicitly.
Test Plan:
Tried to "arc diff" with a commit message of 'derp', got a live link to
documentation instead of a vague set of general instructions.
Reviewed By: aran
Reviewers: moskov, aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 468
unit'
Summary:
We want to handle 'arc unit' and 'arc diff' differently in our test
framework.
Test Plan:
Tested that with 'arc unit' the value was set to 'unit' and with 'arc diff'
the value was set to 'diff'.
Reviewed By: epriestley
Reviewers: slawekbiel, epriestley
CC: jungejason, grglr, aran, tuomaspelkonen, epriestley
Differential Revision: 430
Summary:
Differential showed 'okay' as the arc unit status even when there were
postponed tests.
Test Plan:
Tested that test results were pushed to differential when there were
postponed tests.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
CC: slawekbiel, aran, jungejason
Differential Revision: 417
Summary:
We changed our Facebook implementation to echo test results while the
tests are running. We do not want to echo the test results twice.
Test Plan:
Tested that implementing the function in PhutilUnitTestEngine and
returning true showed the results and returning false didn't show the
results.
Reviewed By: epriestley
Reviewers: jungejason, epriestley, grglr, slawekbiel
Commenters: slawekbiel, aran
CC: sgrimm, slawekbiel, aran, tuomaspelkonen, epriestley
Differential Revision: 400
Summary:
I typed the wrong number of spaces into some of these.
Test Plan:
Visual inspection, ran 'arc unit'
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 402
Summary:
Test Engine classes might need the differential Diff ID to be
able to attach postponed test results to diffs.
Test Plan:
Added setDifferentialDiff function to PhutilUnitTestEngine class
and made sure it was called with the correct diff ID when running
'arc diff --preview'
Reviewed By: jungejason
Reviewers: jungejason, grglr
Commenters: sgrimm
CC: epriestley, sgrimm, slawekbiel, aran, tuomaspelkonen, jungejason
Differential Revision: 395
Summary:
There are a bunch of different ways we could approach this, but practically I
think this is probably the best one even though it's kind of yucky.
A big part of my motivation here is that if we just reject UTF-8 outright, users
are going to end up in a situation they don't understand how to resolve.
UPDATE: after discussion, going with a more conservative approach until such
time as we have a more compelling case for the less strict functionality.
Test Plan:
Created a diff with a text file that contained invalid UTF-8 subsequences.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 325
Summary:
I'm probably missing some edge cases but it took me almost 3 hours to get this
far and I think it only makes things work that didn't work before. Some stuff
like SVN binary patches still won't work, although they should be far easier to
implement.
Most of the magic here just comes from reading the git source code. It appears
to work correctly; I sprinkled printf() around git liberally and recompiled it
during development. Took me about 45 minutes to figure out that "Index" vs
"index" causes git to silently fail in a confusing way. :/
Git has a diff mode for binary changes but I don't think we lose much by always
using the full binaries. We can enhance it later if we want.
Test Plan:
Exported and patched binary changes (a picture of a duck) into a working copy.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: simpkins, aran, epriestley
Differential Revision: 327
Summary:
passthru() doesn't show up on --trace, and one time a while ago someone had an
issue which was harder than necessary to debug because the command wasn't
avialable in the log. Use the logged version.
Also fix a locale issue I ran into on my local machine, with "en_US.utf8" not
being a valid locale.
Test Plan:
Created an SVN repo and used "arc commit" to make commits against it.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 306
Summary:
The --only flag implies --nounit and --nolint, so there's really no
conflict if these flags are specified together.
(My wrapper around arc diff automatically specifies --nounit in some
cases. It is useful to be able to manually add --only in some cases,
and not have it fail because it conflicts with the automatically
specified --nounit flag.)
Test Plan:
Ran "arc diff --only --nounit".
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley, simpkins
Differential Revision: 298
Summary:
Update the lint workflow to exit immediately it there is no lint engine
configured. Previously it scanned the repository to find the paths to
check before failing in this case. Scanning the repository can be
relatively slow on large repositories.
Test Plan:
Ran "arc lint" in a large repository with no lint engine configured.
Verified that it failed quickly, without scanning the repository first.
Reviewed By: jungejason
Reviewers: jungejason, epriestley
CC: aran, jungejason
Differential Revision: 297
Summary:
The story for creating and maintaining libphutil libraries and modules
is pretty terrible right now: you need to know a bunch of secret scripts and
dark magic. Provide 'arc liberate' which endeavors to always do the right thing
and put a library in the correct state.
Test Plan:
Ran liberate on libphutil, arcanist, phabricator; created new
libphutil libraries, added classes to them, liberated everything, introduced
errors etc and liberated that stuff, nothing was obviously broken in a terrible
way..?
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 269
