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:
