Summary:
NOTE: This is a disruptive change to how 'arc diff' behaves by default.
Many years ago, Differential (then DiffCamp) supported SVN. Someone added a "-i" mode to the "diffcamp" script so you could "git show | diffcamp -i", and thus we were
forever bound to storing metadata in commit messages.
But this isn't a common use case outside of Facebook + git-svn, and isn't very flexible. We now have a great deal of flexibility to identify revisions based on
hashes, branch names, etc, and to sync metdata from web to CLI and back. I want to jettison the commit-message-bound artifacts of the tool's history, and move to a
more flexible, modern workflow.
I added "--auto" a while ago, which figures out if you want to create or update a diff automatically, and then prompts you for whatever data it needs, reading
information if it can from commit messages in the range. This is a vastly better workflow in general, especially for SVN and Mercurial users (who currently need to
jump to the web UI to create revisions). It's better for git users too, since they don't need to use template commits and don't have to muck with or configure
templates. However, it's a nontrivial change to git users' core workflow that is clearly different in more ways than it is clearly better.
- This might be contentious, but probably not toooo much, I hope?
- I also deleted the (fairly ridiculous) workflow where we sync commit message changes from the working copy. I think two or three people will be REALLY upset about
this but I have no sympathy. "--edit" covers this and has been around for like two years now, and making the commit message and web dual-authoritative was always a bad
idea that we only ever half-accommodated anyway (see giant swaths of removed TOOD nonsense).
Test Plan:
- I've been using "--auto" exclusively for like a month, it seems to work well.
- Created/updated SVN, Git and HG diffs with "arc diff" under this workflow.
Reviewers: btrahan, jungejason, nh
Reviewed By: btrahan
CC: Makinde, vrana, zeeg, mbautin, aran, yairlivne, 20after4
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D2080
Summary:
This should be a raw text block, not a parsed message object.
I broke this in rARC1f13e022cdc9bf4859274a83784bd615caf62ef9 when I improved Mercurial support.
See: https://github.com/facebook/arcanist/issues/23
Test Plan: (Will update this diff...)
Reviewers: vrana, jungejason, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2136
Summary: Allows you to set a default in .arcconfig. This default is overriden by any .arc/ setting.
Test Plan: Ran "arc diff" with a .arcconfig setting but no .arc/ setting, got a diff against the specified relative commit.
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D2093
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:
The new wrapper shell script that is bin/arc does not correctly
handle arguments with spaces in them.
Test Plan:
added a print_r($argv) to scripts/arcanist.php and ran bin/arc -m
"testing something" to see that "testing something" got passed in as
one argument instead of two.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2066
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: When tests have a lot of output, show a diff of the expected/actual.
Test Plan: Used this when developing D2016 to examine large output usefully.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2017
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
Reviewed By: btrahan
CC: aran, epriestley, gschmidt
Maniphest Tasks: T124, T1033
Differential Revision: https://secure.phabricator.com/D2001
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:
Missed this when I moved the document a while ago. See <https://github.com/facebook/arcanist/issues/21>.
NOTE: The actual document isn't very helpful in figuring out the error. I'll address this in a followup.
Test Plan: Hit error, clicked link, got docs.
Reviewers: btrahan
Reviewed By: btrahan
CC: jmhsieh, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1989
Summary: Obvious error in refactoring code around getCanonicalRevisionName() in D1954.
Test Plan: Ran "arc diff" without looping. Can you verify this fixes your case?
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T1025
Differential Revision: https://secure.phabricator.com/D1970
Summary: Currently, we use a symlink. Under git bash, it tries to run the symlink. This is not a recipe for success. Instead, use a trivial wrapper script.
Test Plan: Ran "arc" from git bash.
Reviewers: Makinde, btrahan, Koolvin
Reviewed By: Koolvin
CC: aran, epriestley
Maniphest Tasks: T124
Differential Revision: https://secure.phabricator.com/D1939
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:
See D1950, same patch for arc/.
NOTE: I'm moving the ob_get_level() stuff to libphuil.
Test Plan: Ran "arc".
Reviewers: btrahan, killermonk
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1951
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:
We currently detect the "\" as a change, and may generate a hunk like this, without changes.
@@ -97,4 +98,4 @@
mmm
mmm
mmm
\ No newline at end of file
While "git apply" is OK with this, "patch" is not. Instead, don't detect this as a change (it is always accompanied by - / + if it's a real change).
Test Plan: Successfully applied a previously-failing SVN patch to a file without a terminal newline that had nonlocal changes.
Reviewers: 20after4, nh, btrahan
Reviewed By: 20after4
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1934
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:
- When users pipe in colorized diffs, strip the colors instead of failing.
- When showing context, show line numbers (we do show 3 lines around the failure, the failure was just on the first line).
- Remove an irrelevant TODO comment (we handle this elsewhere now).
Test Plan: Unit tests.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1887
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: svn changed the format of property changes for svn1.7.
Test Plan: arc diff on a working copy with svn property changes
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1885
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: Wanted to double-check that this works properly; it does, but might as well keep the test case.
Test Plan: Ran tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T970
Differential Revision: https://secure.phabricator.com/D1869
Summary: We incorrectly merge array() into empty string, which is later interpreted as "this file is entirely not-executable". Instead, show no coverage information in the UI.
Test Plan: Looked at a README diff with no coverage information, got no UI render.
Reviewers: tuomaspelkonen, btrahan, zeeg
Reviewed By: tuomaspelkonen
CC: aran, epriestley
Maniphest Tasks: T965
Differential Revision: https://secure.phabricator.com/D1863
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:
Resolves crazy stuff like 'HEAD' or '{2001-01-01T01:01:01}' into an
actual revision, with a provided implementation for git.
Test Plan: Tested in a hacky script.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1849
Summary:
Although we currently forbid anonymous functions, some day we will likely permit them, and our behavior for them is wrong.
Handle them correctly rather than throwing an exception because we should be covered by D1846 against accidental use, and we won't be on PHP 5.2 forever.
Test Plan: Ran against a test file with anonymous functions. Before, it emitted a declaration of a nameless function; afterward, it did not.
Reviewers: btrahan, edward
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T963
Differential Revision: https://secure.phabricator.com/D1847
Summary:
svn 1.7 no longer has a .svn directory in every subdirectory of the working
copy, only one in the root of the working copy (like git, except you can still
check out subtrees). Thus, we can't check whether we're in an svn repo by
looking for a .svn directory alongside the .arcconfig file.
Test Plan: ran arc diff in an svn 1.7 working copy where it previously wasn't working
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1848
Summary:
- Add a lint check for PHP 5.3 features (namespaces, anonymous functions).
- Add unit tests.
- Disable it by default.
- Enable it for us.
Test Plan: Ran unit tests.
Reviewers: btrahan, edward
Reviewed By: edward
CC: aran, epriestley
Maniphest Tasks: T962
Differential Revision: https://secure.phabricator.com/D1846
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
