Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2084
Summary:
The default of "arc diff" to "arc diff HEAD^" in git is universally confusing to everyone not at Facebook.
Drive the default with configuration instead. Even at Facebook, "origin/trunk" (or whatever) is probably a better default than "HEAD^".
See D863 for the last attempt at this.
NOTE: This is contentious!
Test Plan: Ran "arc diff", got prompted to set a default. Ran "arc diff" from a zero-commit repo, got sensible behavior
Reviewers: btrahan, vrana, nh, jungejason, tuomaspelkonen
Reviewed By: btrahan
CC: aran, epriestley, zeeg, davidreuss
Maniphest Tasks: T651
Differential Revision: https://secure.phabricator.com/D1861
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
