Test Plan:
Cancel `arc diff`.
Verify that the message is created.
Run `arc diff --verbatim` and see no reuse message question.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2390
Summary: With `--verbatim` flag, notes created from parse errors are never displayed to user resulting in blank fields.
Test Plan:
- `arc diff --verbatim` with invalid Cc
- `arc diff --verbatim` with all fields correct
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2395
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: Missed this when getting rid of all the 'file' calls.
Test Plan: Meta.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1159
Differential Revision: https://secure.phabricator.com/D2327
Summary: See rage in T1117. Don't use the <project + branch> heuristic anymore..
Test Plan: Ran "arc diff --strict HEAD^" on a commit stacked on top of this one, got no matches.
Reviewers: btrahan, vrana, simpkins, beng
Reviewed By: btrahan
CC: aran, avive
Maniphest Tasks: T1117
Differential Revision: https://secure.phabricator.com/D2221
Summary:
- Historically, "--preview" was forbidden under SVN. No reason for that now.
- The "--auto" patch moved the "--preview" / "--only" checks later than they should be.
- Fix an issue with Conduit query construction in SVN.
Test Plan: Ran "arc diff --preview" in an SVN working copy. Ran "arc diff" in an SVN working copy.
Reviewers: svemir, btrahan, vrana, jungejason
Reviewed By: svemir
CC: aran
Differential Revision: https://secure.phabricator.com/D2218
Summary:
Xcode (a popular code editor on Mac OS X) has no facility
to trim trailing whitespace automatically.
This adds a new lint severity "AUTOFIX" that's between
WARNING and ERROR. When running the linter, any lint message
whose severity is AUTOFIX will automatically be patched.
Furthermore, if all lint messages returned from the engine are
AUTOFIX, we'll automatically amend HEAD with the patch.
Test Plan:
arc lint on files with and without trailing whitespace,
with and without UTF-8 contents to confirm those still error
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2125
Summary:
--auto doesn't work right now on the implicit --create pathway in SVN and HG because we hit these conditions.
Also improve a message.
Test Plan: Ran "arc diff" in unaffiliated working copies in HG and SVN.
Reviewers: svemir, btrahan, vrana, jungejason
Reviewed By: svemir
CC: aran
Differential Revision: https://secure.phabricator.com/D2187
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:
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:
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:
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:
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: 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:
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:
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:
There's no reason this can't work now, we're just not quite careful enough about definitions.
NOTE: If you use a message which has a revision ID in it already, this will fail in an obtuse way when launching the update editor fails because stdin isn't a tty. I'll clean this all up after T614 is fully sorted.
Test Plan: Ran "arc diff --raw -C HEAD".
Reviewers: btrahan, yairlivne
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1765
Summary:
- When altering the include_path(), use PATH_SEPARATOR (";" on Windows, ":" elsewhere) instead of hard-coded ":".
- Detect missing php_curl.dll extension.
- Use APPDATA instead of HOME for storing .arcrc (the internet implies this is correct?)
- Don't try to do chmod() stuff on Windows; it's not critical and I don't want to figure out how it works.
Test Plan: Was able to run part of some arc commands on Windows.
Reviewers: btrahan, Makinde, Koolvin
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T124
Differential Revision: https://secure.phabricator.com/D1756
Summary:
When the user runs "arc diff --create" or triggers it via "arc diff
--auto", prefill the template as best we can.
Test Plan:
Ran "arc diff --auto" with a template commit message in the working
copy under various configurations, results seemed reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, dmi
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1719
Summary:
When a user invokes the update flow from Mercurial, prefill the update message
like we do for Git.
Also add a little caching to improve performance since "hg" execs cost ~100ms.
Test Plan: Updated a Mercurial diff repeatedly, got sensible default message
fills.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T28
Differential Revision: https://secure.phabricator.com/D1718
Summary:
Modified the arc diff workflow, introduced new diff properties -
arc:lint-excuse and arc:unit-excuse for respective errors/warnings.
The diff author inputs the explanation in an editor (similar to commit
message).
Task ID: #
Blame Rev:
Test Plan:
Ran sandbox arc on itself with some lint errors and verified that the
Diff property (== user explanation) is being set.
Revert Plan:
Tags:
Reviewers: epriestley, nh
CC: akramer, blair, aran, andreygoder, Girish, epriestley
Differential Revision: https://secure.phabricator.com/D1676
Summary:
When a Differential revision is updated in git, try to find all commit messages
in the range which we haven't already attached and combine them into a default
update message.
This won't work perfectly in a workflow where you rebase //and// stack local
commits, but worst case is that you just have to delete some lines, which is
still probably better overall than losing this information.
Test Plan: Meta-testing in progress.
Reviewers: btrahan, ptarjan
Reviewed By: btrahan
CC: aran, epriestley, davidreuss
Maniphest Tasks: T28
Differential Revision: https://secure.phabricator.com/D1671
Summary: Otherwise, "arc diff --auto" on a branch with the same name as some
other branch you previously used tries to update that revision.
Test Plan: Ran "arc diff --auto" on a "listdocs" branch; arc didn't try to
update D1639.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1662
Summary:
Our failure behavior currently sucks. In particular:
- If lint or unit fail (expectedly or unexpectedly), we throw away your
message. omglol sux 2 b u
- If there's a parsing error, we dump the message to 'arc-commit-message' in
CWD and tell you to go deal with it.
This is awful. Improve the behavior:
- Once we read a message, save it in .arc/commit-message so we always have it
if anything goes wrong.
- If that file exists, prompt to read it without any "-F" nonsense.
- If there's a parsing error, tell the user and prompt them to just edit the
file again, showing what they need to fix.
Test Plan: Ran "arc diff --auto" and made a bunch of intentional errors; arc
never screwed me over and was generally fairly pleasant.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T614, T895
Differential Revision: https://secure.phabricator.com/D1656
Summary:
For some workflows (all Mercurial, all SVN, some git) I eventually want arc to
pick between "arc --create" and "arc --update" automatically.
"arc which" laid the groundwork for this. For now, implement it as an optional
flag because I think there are still some rough edges on this pathway (for
example, handling of commit messages when errors happen).
When 'arc diff --auto' is invoked, guess whether the user meant --create or
--update.
Test Plan: Created this revision with 'arc diff --auto'.
Reviewers: Makinde, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1648
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:
Currently, we throw a fairly perplexing error when there are multiple valid
commit messages. Installs can also remove the "test plan" field entirely, which
is the only really strong discriminator here.
When the message to use is ambiguous, show the user all the valid messages and
prompt them to choose one.
Also add a -C flag like "git commit -C", so they can choose a message
explicitly.
Test Plan: Ran "arc diff HEAD^^^^^", "arc diff -C <rev>".
Reviewers: cpiro, btrahan, jungejason
Reviewed By: cpiro
CC: aran, cpiro
Differential Revision: https://secure.phabricator.com/D1385
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 T614. This flag explicitly tells Arcanist to use the message for an existing
revision, and attach the change to it directly.
Next step is to have "arc diff" automatically choose "--create" or "--update" in
the absence of "--create", "--update", "--only", "--preview" or a template
commit message.
Test Plan:
Ran "arc diff --update <n>" for various revisions. Got updates or
errors as expected.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, jungejason
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1346
Summary:
Some Mercurial users at Dropbox have very specific diff preparation
needs. Allow "arc" to read an arbitrary diff off stdin. This disables most
features.
Test Plan:
Ran "git diff HEAD | arc diff --raw", "git show | arc diff --raw",
"hg diff --rev 8 | arc diff --raw".
Reviewers: btrahan, jungejason, Makinde
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T617
Differential Revision: https://secure.phabricator.com/D1323
Summary:
I want to make some changes to this workflow but it a huge mess right now. Try
to refactor a bit to make it a little more manageable.
Broadly:
- Moved lint/unit constant mapping into separate methods.
- Moved lint/unit/local properties into separate methods.
- Moved diff spec construction into a separate method.
- Moved some message stuff into separate methods and reorganized related
methods near to one another.
- Removed an unused findRevisionInformation() method.
I fixed a couple of small bugs, too:
- --create now conflicts with --only and --preview.
- --create now probably works in Mercurial.
- --create messages now have basic reviewer validation.
This should have not have any significant behavioral changes.
Test Plan:
- Created this revision.
- Ran "arc diff --create".
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1320
Summary:
- Allow Arcanist to parse either "123", "D123" (existing behaviors) or
"http://phabricator.example.com/D123" (new behavior) values.
- Drop support for labeling this field "DiffCamp". This should only impact
people trying to update revisions that are more than ~a year old, which should
be very very few.
Test Plan: - Ran "arc diff" with values "74", "D74", "x74",
"http://local.aphront.com/D74", "http://local.aphront.com/x74". Got the expected
behaviors.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T54, T692
Differential Revision: 1249
Summary:
See T614. This adds a "--create" flag which I think works properly, but doesn't
make it the default.
Once I add "--update" and am confident the flags actually work, I'll work on
some heuristics to make "arc diff" automatically choose "--udpate" or "--create"
as per T614.
Test Plan:
This revision was created with "--create" and a bogus commit message ("derp").
I intentionally goofed the message at first to test the fail + file workflow.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Differential Revision: 1162
Summary:
Commit templates are fully configurable on the server now, so we should be doing
validation there, since an install can add or remove fields and change
validation rules. Remove these outdated client validations, as per comment.
Also update the API call to use the 'errors' field, which allows us to show the
user all the parse errors at once. See:
https://secure.phabricator.com/diffusion/P/browse/origin:master/src/applications/conduit/method/differential/parsecommitmessage/ConduitAPI_differential_parsecommitmessage_Method.php;cfaab709df37739b$75
Test Plan:
Ran "arc diff" on a change with multiple errors:
```$ arc diff --conduit-uri=http://local.aphront.com/
Usage Exception: Commit message is not properly formatted:
Error parsing field 'Reviewers': Commit message references nonexistent users:
jjjderp.
Error parsing field 'CC': Commit message references nonexistent users and
mailing lists: jjjderp.
You should use the standard git commit template to provide a commit message. If
you only want to create a diff (not a revision), use --preview to ignore commit
messages.```
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1154
Summary:
If any hunks is detected as non-utf8, and you've never submitted diffs
for a certain project before, you would get a ERR-BAD-ARCANIST-PROJECT
exception. This makes it possible to submit the patch properly, so you
can set the encoding in the interface afterwards. Further this fixes
cases where you don't supply a diff but will result in hunks getting
treated as binary, but that still beats the exception behaviour.
Test Plan:
Ran `arc diff` with and without the new --encoding param and
got the expected results. Also ensured the diff (with non utf-8 hunks)
would be properly created even when no encoding is specified.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1135
Summary:
Git works completely differently for commits zero and one than for 2..N so add
more special casing to handle them. See:
- {T206}
- {T596}
The getCommitRange() block is also fatal land, although I wasn't able to reach
it. I'll follow up with @s on T596.
Test Plan:
- Created a new, empty repository ("mkdir x; cd x; git init").
- Ran "arc lint", "arc unit", "arc diff" against it with no commits (the first
two work, the third fails helpfully).
- Made an initial commit.
- Ran "arc lint", "arc unit", "arc diff" against it (all work correctly).
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: s, aran
Differential Revision: 1142
Summary:
just a little try / catch action in ArcanistDiffWorkflow.
"Ideally, it would be good to flag these changes somehow so that "arc patch" can
issue the inverse warning (e.g., "This patch could not be completely applied
because some binary data was not uploaded.") but that hasn't come up in a real
use case yet."
I think a change w/ filetype of FILE_IMAGE || FILE_BINARY *and* no phid means
that the upload failed so there's no additional flag needed. (True?) however,
it would be easy enough to store metadata that explicilty stated whether or not
the file upload succeeded.
(also / related - i looked through the arc patch workflow a bit and i don't
understand how the svn codepath loads up the actual binary files... for git,
toGitPatch => buildBinaryChange => getBlob is the right path )
Test Plan:
- set 'storage.mysql-engine.max-size' to 0 in my conf, uploaded diffs with
files
- noted in differential that it correctly detected images versus binary despite
file upload failing
- noted in differential that images had some empty UI
- reverted conf change, uploaded diffs with files
- noted in differential file showed up
- ran arc patch with DX, where DX had broken files
- noted "Downloading binary data...
Exception:
ERR-BAD-PHID: No such file exists."
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1100
Summary:
Adds a secret, undoucmented "encoding" key to ".arcconfig" which makes a very
half-hearted effort to convert encodings. This is probably good enough that
Differential can be used for code review, but there will be issues with 'arc
patch', 'arc export', paste, maybe conduit stuff, Diffusion, and whatever else I
haven't thought of.
This also doesn't store the original encoding so anything converted like this
won't reasonably be able to be made to work with all that stuff in the future.
See T452 for a broader discussion of the issues involved.
Test Plan:
Short circuited the UTF-8 detection to always fail, had my files "converted"
from ISO-8859-1 to UTF-8.
@davidreuss: you can test this by applying this patch to arcanist/, adding
'"encoding" : "ISO-8859-1"' to your .arcconfig, touching some non-ASCII file,
and then running "arc diff".
Reviewers: davidreuss, jungejason, tuomaspelkonen, aran
Reviewed By: davidreuss
CC: aran, davidreuss, epriestley, nshamg123
Differential Revision: 812
Summary: When a project uses a conservative history mutability doctrine, never
try to amend history.
Test Plan: Ran "arc amend" in an "immutable_history" working copy.
Reviewers: fratrik, Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde, epriestley
Differential Revision: 862
