Summary: The way we represent some move/copy stuff is a bit messed up, but it mostly works, so add coverage before I mess with it.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T866
Differential Revision: https://secure.phabricator.com/D3752
Summary:
We have coverage for generating patches, applying them, and making sure they reproduce the original repository state. However:
- The code uses simplified patch generation which omits some flags like `-M` and `-C`, and generally produces less rich patches than we really produce. Instead, produce patches the same way `arc diff` does.
- We don't test the intermediate change representation. In theory it's not too important because if we get it wrong the output should be wrong, but in practice it makes it easier to nail down issues. We can also generate less-rich patches which still apply correctly, but would prefer not to.
- Similarly, we don't test the intermediate patch representation. This is almost entirely redundant with simply applying the patch, but easier to visualize.
Add coverage for all that stuff and fix some bugs with `-M` / `-C` patch generation that weren't caught under the simpler patch generation.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T866
Differential Revision: https://secure.phabricator.com/D3751
Summary: Make this harder to get wrong. Instead of requiring a separate call for synthetic data, automatically load it if we can.
Test Plan: Unit tests; `arc diff`.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T866
Differential Revision: https://secure.phabricator.com/D3750
Summary:
- I caused $parser to be reused in D3732 which I belived was safe, but actually isn't. We end up writing to the same changes. We should make it safe but there's some mess in Phabricator that needs to be cleaned up first.
- One minor error code thing, variable is undefined.
Test Plan: Ran `arc export --git` on a moved file, got a better result. Ran some command which made me hit the other case and didn't get a fatal anymore.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3749
Summary:
After a reintegration merge, "Copied From URL" will be different and current approach will result in a wrong path. If the path does not match, just mark it as a new file.
moved the comment before if so lines stay at 80 chars
Test Plan: in trunk, svn merge --reintegrate ^/branches/foo and arc diff - without this change it will say "Copied from es/foo/..."
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3748
Summary: See T1675 for discussion. We currently strip `[abicwo12]/` from Git patches, but the user can provide arbitrary prefixes with `--src-prefix` and `--dst-prefix`, or strip prefixes entirely with `--no-prefix`. In these cases, trust they know what they're doing rather than rejecting the diff.
Test Plan: Added a bunch of tests. We have existing tests for `diff.mnemonicprefix` and normal prefixes.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1675
Differential Revision: https://secure.phabricator.com/D3744
Summary:
Hunk may be missing newline at end of file. It produces exports like this:
lang=diff
--- a/third-party
+++ b/third-party
@@ -1 +1 @@
-/mnt/gvfs/third-party/90cb1654197e56261b1733c704b387285f36208e
\ No newline at end of file
+/mnt/gvfs/third-party/7097083d10d37251218531da398545658872a47a
\ No newline at end of filediff --git a/ti/proxygen/TARGETS b/ti/proxygen/TARGETS
Test Plan:
$ arc export --git --diff 1
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, lesliepc16
Differential Revision: https://secure.phabricator.com/D3672
Summary:
Adds a test which goes through a git repository commit by commit and applies them (in effect) via "arc patch", verifying that the results match the actual commit.
See D3439 for the fixture stuff.
The git repo archive has a couple of trivial commits in it:
$ ../libphutil/scripts/utils/directory_fixture.php src/parser/__tests__/bundle.git.tgz
Spawning an interactive shell. Exit when complete.
sh-3.2$ git log
commit f19fb9fa1385c01b53bdb6d8842dd154e47151ec
Author: epriestley <git@epriestley.com>
Date: Wed Sep 5 14:30:28 2012 -0700
Edit a text file.
commit 228d7be4840313ed805c25c15bba0f7b188af3e6
Author: epriestley <git@epriestley.com>
Date: Wed Sep 5 14:30:11 2012 -0700
Add a text file.
Test Plan: Ran tests.
Reviewers: edward
Reviewed By: edward
CC: aran
Maniphest Tasks: T866
Differential Revision: https://secure.phabricator.com/D3440
Summary:
If the file has no changes (probably because it has been moved or copied) then we fail.
Fixes T1708, fixes T1559.
Test Plan:
$ svn mv a b
$ arc diff --only
$ svn revert a b
$ arc patch --diff # of created diff
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Maniphest Tasks: T1559, T1708
Differential Revision: https://secure.phabricator.com/D3526
Summary: We don't store old file PHID for binary file moves here.
Test Plan: `arc patch` on revision with binary file moves which was failing earlier.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3521
Summary:
- Add zlib functions to extension functions.
- Provide a better error if the extension is actually missing.
Test Plan: Eyeballed it.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D3321
Summary: See T1635 and the giant inline comment.
Test Plan: Ran unit tests on 32-bit and 64-bit machines.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1635
Differential Revision: https://secure.phabricator.com/D3250
Summary: See T1635. I'm going to make an effort to rewrite this in a way that's safe in 32-bit PHP, i.e. without bcmath. Add test coverage to limit the chance I screw it up.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1635
Differential Revision: https://secure.phabricator.com/D3249
Summary: See D3252. Reduces code duplication a little bit. Also remove some dire warnings about impending doom -- this has been in use in the wild for a long time.
Test Plan: Added a file in ISO-8859-1, ran `arc diff --encoding ISO-8859-1` to generate this revision, got an encoding note in output.
Reviewers: davidreuss, vrana, btrahan
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D3253
Summary: This diff format is used by de-facto mercurial GUI called "TortoiseHG".
It is available as the only way to copy diff into clipboard (right click commit,
select "export", select "copy patch". I added this format support into arcanist
so revisions in Differential can be created from TortoiseHG via simple copy-
paste. Unit test added, manually tested.
See: https://github.com/facebook/arcanist/pull/46
Reviewed by: epriestley
Summary: When you run `hg diff -r x:y`, we get two "-r" arguments in the diff header. Currently, we parse this incorrectly.
Test Plan: Added unit test which previously failed; test now passes.
Reviewers: dschleimer, btrahan
Reviewed By: dschleimer
CC: aran, cakoose
Maniphest Tasks: T1550
Differential Revision: https://secure.phabricator.com/D3061
Summary:
On Windows, a diff may have "\n" newlines (from the file itself) but "\r\n" blocks (from svn).
NOTE: indents are funky since I edited this with Notepad++, I'll fix before landing.
Test Plan: Diffed an edit to a "\n" newline file on Windows in SVN.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2998
Summary: I'm trying to get a repro for a Windows + SVN patch issue. Dump patches which fail to a temp file so there's less bewilderment in getting the right patch handed over for analysis.
Test Plan: Forced a parse failure, ran "arc diff", inspected temp file.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2997
Summary: From "cmd.exe" with, e.g. SilkSVN, there are some issues getting arc to do anything useful. Resolve enough of them so that it's at least usable.
Test Plan: Created a revision from Windows / cmd.exe / arc / SVN.
Reviewers: btrahan, jungejason, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2984
Summary:
- Implement "arc:amended", a base commit DSL rule which always selects HEAD (git) or `.` (hg) if it has "differential revision:" in the commit message. This is unambiguously correct in amend workflows, and can cover holes in other rules like "git:branch-unique(*)".
- Fix a bunch of Mercurial stuff:
- Our use of '.' is wrong, and based on a misunderstanding on my part of the behavior of `hg diff --rev . --rev .`, which means "ignore the second --rev flag", not ". means working directory state". As far as I know there's no explicit way to say "the working copy plus all its changes".
- The `--prune` argument to "hg log" does not support symbolic names like ".^". Use revsets instead.
- Reduce the number of times we need to run `hg branch`.
- We can safely use "." to mean "the working copy revision", and do not need to do "hg --debug id" or similar.
- Generally simplify some of the nonsense in the implementation left over from me having no idea how Mercurial works.
Test Plan:
Ran "arc which" in various scenarios in a mercurial working copy. I //think// I exercised all the changes.
Ran "arc which --base arc:amended" in hg and git working copies without "Differential Revision:" in head/. (no match) and with it (matched head/.).
Reviewers: dschleimer
Reviewed By: dschleimer
CC: Makinde, tido, phleet, aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2876
Summary:
This adds a new base option that Does the Right Thing (or as close to
it as I can get) for someone using Mercurial bookmarks as lightweight
branches, and where each branch should be one differential revision.
Specifically, it walks backwards through the ancestors of the working
copy until it finds a revision that is either not outgoing (ie already
in the remote) or that is bookmarked. This means that bookmarks
effectively act as delimiters, and point at the last revision that
will be included in an arc diff.
Test Plan:
[14:40:54 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1226 $ hg log -r '(first(outgoing())^)::' --template='node: {node}\nbookmark: {bookmarks}\n\n' -G
@ node: c8379ef32b0d0e6cf94fe636751ea4fe1353e157
| bookmark:
|
| o node: 14f03139049cbda339190b814e52f4ec8b05c431
| | bookmark: more
| |
| o node: 6970e9263ab8c6da428420606d1f15c9980da183
| | bookmark: something
| |
| o node: 433a93023f03d5f3eddaa243fa973d32a1566aee
|/ bookmark:
|
o node: f47ccfe34267592dd2e336174a3a311b8783c024
| bookmark:
|
[14:41:00 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1226 $ ~/devtools/arcanist/bin/arc which --show-base --base 'arc:bookmark'
f47ccfe34267592dd2e336174a3a311b8783c024
[14:41:05 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1226 $ hg up 14f03139049cbda339190b814e52f4ec8b05c431
3 files updated, 0 files merged, 1 files removed, 0 files unresolved
[14:41:10 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1226 $ ~/devtools/arcanist/bin/arc which --show-base --base 'arc:bookmark'
6970e9263ab8c6da428420606d1f15c9980da183
[14:41:14 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1226 $ hg up 6970e9263ab8c6da428420606d1f15c9980da183
0 files updated, 0 files merged, 1 files removed, 0 files unresolved
[14:41:44 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1227 $ ~/devtools/arcanist/bin/arc which --show-base --base 'arc:bookmark'
f47ccfe34267592dd2e336174a3a311b8783c024
Reviewers: epriestley, bos
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1331
Differential Revision: https://secure.phabricator.com/D2863
Summary: Allow users to run a command to determine the base revision of the commit range.
Test Plan: Ran stuff like `arc which --show-base --base 'arc:exec(ls)'` and got the expected results.
Reviewers: dschleimer, csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2830
Summary:
New optional mode. If you set 'base' in local, project or global config or pass '--base' to 'arc diff' or 'arc which', it switches to DSL mode.
In DSL mode, lists of rules from args, local, project and global config are resolved, in that order. Rules can manipulate the rule machine or resolve into actual commits. Provides support for some 'arc' rules (mostly machine manipulation) and 'git' rules (symbolic ref and merge-base).
Test Plan:
Ran unit tests. Also:
```$ arc which --show-base --base 'arc:prompt'
Against which commit? HEAD
HEAD
$ arc which --show-base --base 'git:HEAD'
HEAD
$ arc which --show-base --base 'git:fake'
Usage Exception: None of the rules in your 'base' configuration matched a valid commit. Adjust rules or specify which commit you want to use explicitly.
$ arc which --show-base --base 'git:origin/master'
origin/master
$ arc which --show-base --base 'git:upstream'
Usage Exception: None of the rules in your 'base' configuration matched a valid commit. Adjust rules or specify which commit you want to use explicitly.
$ arc which --show-base --base 'literal:derp'
derp
$ arc which --show-base --base 'arc:halt'
Usage Exception: None of the rules in your 'base' configuration matched a valid commit. Adjust rules or specify which commit you want to use explicitly.
$ arc set-config --local base git:origin/master
Set key 'base' = 'git:origin/master' in local config.
$ arc which --show-base
origin/master
$ arc which --show-base --base 'git:HEAD^'
HEAD^
$ arc which --show-base --base 'arc:yield, git:HEAD^'
origin/master
$ arc which --show-base --base 'arc:global, git:HEAD^'
HEAD^
$ arc which --show-base --base 'arc:global, git:merge-base(origin/master)'
3f4f8992fba8d1f142974da36a82bae900e247c0```
Reviewers: dschleimer, vrana
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2748
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
