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:
When phutil_analyzer builds the dependency graph I convert all class names w.r.t
XHP's internal naming scheme. It actually wouldn't be a terrible idea to do this
munging when phutil loads the symbols. I guess it doesn't really matter at the
moment since only Arcanist generates phutil maps and only libphutil reads them,
but it'd be nice to do this munging in as few places as possible in the future.
The XHP grammar does not allow elements to be interfaces
(dafff2cc18/xhp/parser.y (L1688))
so I've only applied this to classes.
Test Plan:
Created a sample project:
./__phutil_library_init__.php
hopping/__init__.php
/hophophop.php
where that last file contains
<?php
class :bunny:hop-hop-hop extends ❌element {
protected function render() {
return <p>bunny goes hop hop hop</p>;
}
}
Ran phutil_mapper.php and generated __phutil_library_map__.php:
<?php
/**
* This file is automatically generated. Use 'phutil_mapper.php' to rebuild i\
t.
* @generated
*/
phutil_register_library_map(array(
'class' =>
array(
'xhp_bunny__hop_hop_hop' => 'hopping',
),
'function' =>
array(
),
'requires_class' =>
array(
'xhp_bunny__hop_hop_hop' => 'xhp_x__element',
),
'requires_interface' =>
array(
),
));
Reviewed By: epriestley
Reviewers: epriestley
Commenters: aran
CC: aran, ide, epriestley
Differential Revision: 715
Summary:
In hphp's version of idx, it uses debug_rlog() which is not
available in phabricator's code. It will be executed if null is passed
as the second value to idx.
Test Plan:
ran arc lint which still report error message, and php
./scripts/arcanist.php unit src/lint/linter/xhpast/__test__ doesn't
throw.
Reviewed By: epriestley
Reviewers: tuomaspelkonen, epriestley
CC: aran, jungejason, epriestley
Differential Revision: 666
Summary: See D589. Some environments configure output buffering with a prepend script. This is rather silly, but we can at least recover from it.
Test Plan: Started multiple output buffers with ob_start() and then used phutil_console_confirm() to create a prompt. Verified that 'arc' was broken under output buffering prior to the patch, but now works correctly.
Reviewed By: dreuss
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:
In order to support more flexible post hooks. Also send the
error code to the hook for use by client code.
Test Plan: None
Reviewed By: epriestley
Reviewers: epriestley
CC: dpepper, aran, epriestley
Differential Revision: 656
Summary:
in https://secure.phabricator.com/rARC36de84ee, we changed to
use 'binary-phid' from 'binary-guid'. Several places are still using
'binary-guid'. Fix them.
Test Plan:
run 'arc patch' against a revision which was throwing
exception because of this issue and it worked.
Reviewed By: epriestley
Reviewers: epriestley, sgrimm
CC: aran, epriestley
Differential Revision: 624
Summary:
executes the calls to git in parallel to improve startup performance of
arc lint and possibly other commands
Gets about a 2~3x speedup when repo is in buffer cache, with 4 cores.
Test Plan:
arc linted a repo with unstaged, untracked, staged, and committed
changes, see that the same files were linted.
Reviewed By: jungejason
Reviewers: jungejason, epriestley
Commenters: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 594
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:
We currently look:
- In the working copy
- Alongside libphutil/
- In PHP include_path
Before checking next to libphutil/, check next to the working copy. The problem
right now is that javelin/ references diviner/, but it won't find it next to
javelin/, only next to the libphutil/ instance which is loaded off
/home/engshare/devtools.
Test Plan: Moved javelin to tmp/javelin and diviner to tmp/divinerx. Ran 'arc
list' and got a library load error. Moved 'divinerx' to 'diviner' and ran 'arc
list'. It loaded correctly. Ran '--trace' to verify load location.
Reviewed By: cpojer
Reviewers: cpojer, jungejason
CC: aran, cpojer
Differential Revision: 568
Summary:
Document the relationship between lint engines and linters. Provide an example
linter. Improve the documentation of PyLintLinter, which has a bunch of
configuration stuff which you had to dig into the code to get.
Test Plan:
Ran "arc lint --engine ExampleLintEngine --lintall derp.py" on a file with a
Python syntax error in it. Read documentation.
Reviewed By: j3kuntz
Reviewers: j3kuntz, andrewjcg
CC: aran, j3kuntz
Differential Revision: 557
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:
See task. This is a bizarre edge case which we incorrectly reject but should do
our best with.
Test Plan:
Created an added copy of a file and diffed it successfully.
Reviewed By: alex
Reviewers: alex, jungejason, tuomaspelkonen, aran
CC: aran, alex
Differential Revision: 547
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:
We give you a worthless message that doesn't point at "arc install-certificate"
if you don't have an .arcrc right now. Instead, tell the user to run "arc
install-certificate".
Test Plan:
Nuked my ~/.arcrc and tried to run "arc list".
Reviewed By: llorca
Reviewers: codeblock, llorca, jungejason, aran, tuomaspelkonen
CC: aran, llorca
Differential Revision: 538
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 is sort of cheating, but just have this feature disable itself if the input contains multibyte UTF-8 characters. We can clean it up in the future, maybe when we have better utf8 tools.
This means that all the algorithms are safe to pass utf8 to, so we can get rid of all the "<?>" silliness.
Test Plan: Added a UTF8 character to a line, diffed it out, and got the entire line highlighted as changed. See: https://secure.phabricator.com/file/view/PHID-FILE-70fb54eb3f88dc057ab3/
Reviewers: jungejason, aran, tuomaspelkonen
CC:
Differential Revision: 514
Summary:
W293 reports a lint warning for the same problem that TXT6 reports a
lint error for. This isn't so terrible in and of itself.
However, when you are then prompted to apply a patch to fix TXT6, lint
doesn't realize that the W293 warning was also handled, and still
prompts you about ignoring unresolved warnings. This is misleading.
Test Plan:
> python pep8.py hasBlankLineWS.py
hasBlankLineWS.py:3:1: W293 blank line contains whitespace
> python pep8.py --ignore=W293 hasBlankLineWS.py
(no output)
Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: epriestley, aran
Differential Revision: 525
Summary:
Turns out you can hava a branch named |foo.
(Good that '&& rm -fR /usr' is not a valid name)
Test Plan:
created a branch |test, run arc branch
Reviewed By: epriestley
Reviewers: epriestley, dschafer
CC: aran, epriestley, slawekbiel
Revert Plan:
sure
Other Notes:
Differential Revision: 507
Summary:
Right now, if you specify "/api" instead of "/api/" the entire world breaks in a
horrible way. Generally, this whole thing is silly. Just use the right path.
Test Plan:
Edited .arcconfig to use the "wrong" path, so this diff demonstrates the change
works.
Reviewed By: llorca
Reviewers: jungejason, llorca, aran, tuomaspelkonen
CC: aran, llorca, epriestley
Differential Revision: 506
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:
cpojer fixed a JS instance of this in D481, but we can catch it in PHP with
XHPAST. Add a lint rule to fail if nested loops use the same iterator.
Test Plan:
Ran unit test.
Reviewed By: tomo
Reviewers: aran, pad, cpojer, jungejason, tuomaspelkonen, tomo
Commenters: cpojer
CC: aran, cpojer, epriestley, tomo
Differential Revision: 489
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: This once made sense but hasn't been a reasonable default for a long time, get rid of it.
Test Plan: Ran "arc list".
Reviewers: llorca, toulouse
CC:
Differential Revision: 496
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
Summary:
We have some "@generated" files of these types now, hit them with the text
linters
Test Plan:
Ran 'arc lint' on D444
Reviewed By: aran
Reviewers: aran
CC: aran
Differential Revision: 451
Summary:
This is pretty coarse and could be refined, but I often do this when testing:
lang=diff
- if (some_complicated_condition())
+ if (true || some_complicated_condition())
aran has caught me not only doing it but sending out diffs with it like 30
times. Catch it in lint instead.
Test Plan:
Unit test, added a "true || $junk" to the code and linted it.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, pad
CC: aran
Differential Revision: 447
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:
The amend process used "git log HEAD^..HEAD" to get log for the
commit being amended. When run on a merge commit this can return
any number of commits from the non-first parents. Since only a
single commit was expected, arc fails here.
This diff changes the amend process to use the '--first-parent' flag
to be consistent with using '^', which references the first parent.
This should guarantee a single commit log every time.
Test Plan:
arc amend on a merge commit
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley, andrewjcg
Differential Revision: 415
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:
Adds "--no-textconv" to all 'git diff' commands so we don't invoke textconv. See
T178 for discussion.
Test Plan:
Added something like this to .gitattributes:
*.txt diff=uppercase
And then this to .git/config:
[diff "uppercase"]
textconv = /path/to/uppercase
...where "uppercase" is a script which takes a file and emits an uppercase
version of it.
Then I added a "wisdom.txt" text file:
The cow goes "moo".
The duck goes "quack".
Without this patch, the file appears in uppercase in Differential, i.e. textconv
runs. With this patch, it appears as the original text.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: elgenie, aran, tuomaspelkonen
Differential Revision: 372
Summary:
This is sort of a guess. In D357, arc has gone crazy and removed a bunch of
'utils' dependencies for gc3. I think this is because his HPHP/i has them marked
as builtins.
However, I don't have an HPHP/i build (and probably don't have the fortitude to
build it outside of a Facebook environment, I spent about two hours on it at one
point and got maybe 25% of the way through the build process before running into
things I didn't know how to resolve) so I'm not sure this is the issue. No one
else's diffs have exhibited this problem eitehr, so I'm not confident this is
actually the problem or solution.
gc3, can you apply this locally to a copy of arcanist and then run 'arc lint'
(or 'arc lint --apply-patches') on your commit and see if it restores all the
libphutil/utils requirements?
Test Plan:
unable, see summary
Added 'array_keys' to this blacklist and verified the technical behavior of the
patch is correct by var_dump()'ing builtins.
Reviewed By: gc3
Reviewers: gc3, tuomaspelkonen, jungejason, aran
CC: aran, gc3, epriestley
Differential Revision: 361
