Summary: Mechanical changes from D2588. No "Class.php" moves yet because they aren't necessary for libraries to function.
Test Plan: See D2588.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2589
Summary:
"git diff -M -C" generates useful metadata (moves/copies) but (for a pure move) no diff text. Synthetically build the diff text after the fact so this information is available in Differential.
This patch is kind of nasty but I couldn't see a cleaner way to do it. :/
This also needs some UI changes in Differential: we get a full-green new file right now, but it would be better to default-hide it with "This file was moved. Show More" or similar.
Test Plan: Moved a file, ran "arc diff", got textual diff.
Reviewers: aran, tuomaspelkonen, jungejason, btrahan, vrana
Reviewed By: vrana
CC: aran, epriestley, vrana
Maniphest Tasks: T230
Differential Revision: https://secure.phabricator.com/D479
Summary:
Arcanist fails to find git's 'commit <hash>' header when log.decorate is
set in one of git's config files. git adds the named refs that point to
the commit in parentheses following the hash. This changes the regex
used by arcanist to match git commit headers to optionally match the
branch names in parentheses following the hash.
Test Plan:
Run `git config --global log.decorate short` and check that `arc diff`
runs successfully.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2344
Summary: See next diff for an explanation of this issue.
Test Plan: See next diff.
Reviewers: Makinde, btrahan, vrana, jungejason
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2174
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:
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:
- When you try to "arc patch" a change which adds or removes a trailing newline or alters a file with a trailing newline near the newline, the patch fails because we fail to reconstuct the "\ No newline at end of file" line.
- We don't currently record enough information about these diffs to reconstruct them with a sensible amount of effort. Store the "\ No newline at end of file" line instead of just a flag.
- There are some special rules for these lines; implement them.
NOTE: This causes some display glitching in Differential, but nothing major; I'll fix that in a followup patch.
Test Plan:
- Created a diff which added a trailing newline, removed a trailing newline, and altered a file without a trailing newline near the end of the file.
- Verified that the git version of this patch and the 'arc export --git' version of this patch are (essentially) identical.
- Verified that "arc diff" + "arc patch" can apply these changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T533
Differential Revision: https://secure.phabricator.com/D1810
Summary: This is probably just the first step down a long road of not handling
exotic filenames correctly, but if you diff a file named "∆.jpg" it currently
chokes while parsing the diff.
Test Plan: Added minimal failing unit test, fixed parser, test passed.
Reviewers: zeeg, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1592
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:
Revert of D715, which allowed you to put xhp classes into libphutil libraries.
We're removing support for XHP in resolving T635.
According to @ide, removing this won't break anything.
Test Plan: Straight revert. Grepped for methods.
Reviewers: btrahan, ide, nh, jungejason
Reviewed By: ide
CC: aran, epriestley
Maniphest Tasks: T635
Differential Revision: https://secure.phabricator.com/D1511
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
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 T419 for some philosophical musing on this. The "ideal" way to represent
these changes is two or more COPY_HERE plus a DELETE, but we can't build the
DELETE patch yet.
Just turn one of the COPY_HERE changes into a MOVE_HERE. While less idealistic,
I think this should always work.
Test Plan: This seemed to cleanly apply TenXer D265. TenXer guys, can you
confirm that this makes it actually patch correctly?
Reviewers: jonathanhester, bizrad6, kdeggelman, jungejason, btrahan
Reviewed By: jungejason
CC: aran, jonathanhester, jungejason, epriestley
Maniphest Tasks: T419
Differential Revision: https://secure.phabricator.com/D1270
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
removed
Summary: Mercurial output diverges from git output when binary files are
removed. Parse the Mercurial flavor.
Test Plan:
- Unit test fails before this change and passes afterward.
- Removed a binary file in my hg local and successfully "arc diff --only"'d
the change.
NOTE: Git can't apply these patches, and reports:
$ git apply < ~/Desktop/patch.txt
error: removal patch leaves file contents
error: level.png: patch does not apply
Reviewers: Makinde, btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, Makinde
Differential Revision: 1126
Summary: 'patch' chokes on hunks with too much trailing stuff. 'git apply'
chokes on overlapping hunks. Make them both happy. Write some test cases so I
stop breaking this stuff.
Test Plan:
- Applied a previously-failing patch via SVN.
- Applied a previously-failing-then-succeeding patch via Git.
- Ran unit tests.
Reviewers: jungejason, btrahan, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1099
Summary: We currently may generate patches which have hunks that include overlapping context. Git was okay with this until at least 1.7.3.4, but started rejecting these patches some time later.
We already attempt to detect and prevent this condition, we just don't do a very good job of it. Fix the check so that we avoid generating overlapping hunks.
Test Plan:
- Cleanly applied troublesome patches under Git 1.7.7.2.
- Used "arc export --git" to verify patches generate without overlapping sections.
- Locally changed $context to 2 and 1, verified patch behavior was reasonable.
Reviewers: kdeggelman, bizrad6, jonathanhester, jungejason, nh, tuomaspelkonen, aran
CC:
Differential Revision: 1084
Test Plan:
Tried viewing a diff detected as binary in diffusion with and without
setting my desired encoding. Worked as expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1008
mercurial changesets
Summary: Makes the parsing slightly more liberal so a test case from @Makinde
passes.
Test Plan: Ran unit tests.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1054
Summary:
See T584. Git renders these types of changes somewhat unusually. Ensure they are
rendered as changes.
The fact that a symlink was replaced is not explicitly rendered (e.g., "This
path was changed from a symlink to a regular file."), but can be inferred from
the 'unix:filemode' property change that will accompany the changeset. We could
also make it explicit but this type of change is rare enough that it's probably
good enough as-is.
Test Plan:
- Ran unit test, which previously failed and now passes.
- Created a diff which replaces a file with a symlink, verified it rendered a
little more sensibly.
Reviewers: aravindn, dschleimer, jungejason, nh, tuomaspelkonen
Reviewed By: nh
CC: aran, goldshlager, nh
Differential Revision: 1030
changes which add or alter binary files
Summary: See test cases, derived from "hg diff --git" and "git diff --binary".
Test Plan: Test cases work now.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1047
Test Plan:
Go to /differential/diff/create and try to upload a patch that was created with
`git diff` when diff.mnemonicprefix was set to true.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1022
Summary: A minor bug in the parser prevented it from handling git diffs where an
empty file appears at the end of the diff. Since git appends an extra newline,
we failed to jump into the '$line === null' block.
Test Plan: Ran unit tests. Generated a revision which only deleted an empty
file.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh, epriestley
Differential Revision: 998
Summary:
The unix utility `patch` will not move a file (unless it is in or out of
/dev/null). If a diff copies or moves a file and also makes changes to the file,
the generated patch needs to list the new filename as the path to both the
original and new files so the changes get written to the new file.
(When arc applies this patch, it copies or moves the original as needed before
running `patch`.)
(This only matters for svn repos, since arc uses git commands for git repos
instead of using `patch`.)
Test Plan:
Created an arc bundle of a diff that copied a file to a new location and made
changes in both locations, and then ran arc patch with this bundle (in an svn
repo) to see that it correctly patches.
Reviewed By: epriestley
Reviewers: epriestley, jungejason, tuomaspelkonen
CC: aran, epriestley, nh
Differential Revision: 813
Summary:
A better definition of "binary" is "not utf-8", instead of "has some characters
not in this arbitrary regexp". Principally, this makes files with windows
newlines not autodetect as binary.
This might fix some of the issues in T365.
Test Plan: @egillth applied this patch and verified that Diffusion now shows
file content instead of detecting everything as binary in his repo full of
Windows newlines.
Reviewed By: jungejason
Reviewers: egillth, tuomaspelkonen, jungejason, aran
CC: aran, jungejason
Differential Revision: 799
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:
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 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:
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:
Arc patch for image files failed, because they were treated like text
files.
Test Plan:
Tested that 'arc patch' worked for a png file.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
CC: aran, jungejason
Differential Revision: 341
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:
When an arcbundle includes a symlink, we fail to apply it correctly
when applying to a subversion working copy.
Test Plan:
created a diff which added a symlink, removed it locally, bundled
and applied the patch, got a good symlink out
Reviewed By: aran
Reviewers: aran
CC: epriestley, aran
Differential Revision: 63
Summary: We have some diffs which predate property tracking and are shipping
down sketch data, work around it in the client for now.
Test Plan:
Reviewers:
CC:
svn.
Summary: when you "svn cp" with a revision, we end up with some path collision
problems. This isn't a complete fix but it gets the change to Differential,
at least.
Test Plan:
Reviewers:
CC:
