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:
We have a linter that needs to read data from the repo that isn't in the commit,
so that data isn't part of the svn transaction, thus not available in svnlook
cat --transaction. This change provides a method to get data from a committed
file.
Test Plan:
Successfully ran arc svn-hook-pre-commit with a linter that needs something
not in the transaction.
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2582
Summary:
there are two problems making the ArcanistSvnHookPreCommitWorkflow not working.
- pathExists($path) will always return false because it hasn't loaded the path yet. Because of this, PhutilLintEngine will unset the path at https://secure.phabricator.com/diffusion/ARC/browse/master/src/lint/engine/phutil/PhutilLintEngine.php;032b9b30b0721c3f$46, so ArcanistSvnHookPreCommitWorkflow will have no path to lint against and never detects a problem.
- In ArcanistSubversionHookAPI::getCurrentFileData(), the $path is already the full path in svnroot (path got from https://secure.phabricator.com/diffusion/ARC/browse/master/src/workflow/svn-hook-pre-commit/ArcanistSvnHookPreCommitWorkflow.php;032b9b30b0721c3f$72). Prepending the project root to it will make the file path to be wrong so that the file content is empty. Again, ArcanistSvnHookPreCommitWorkflow never detects a problem.
Test Plan:
changed --transaction to --revision in related code and manually ran the following command. It detected the syntaxt error correctly. 558937 is a revision with syntax error.
/usr/local/bin/php -f /home/jungejason/nfs_devtools/arcanist/bin/arc svn-hook-pre-commit --load-phutil-library=/tmp/testwww/www/lib/arcanist --load-phutil-library=/home/jungejason/nfs_devtools/facebook/src /svnroot 558937 2>&1
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, Girish, Koolvin
Differential Revision: https://secure.phabricator.com/D2485
Summary:
- When you run "arc diff" in Mercurial, we currently give you an empty template. Instead, prefill a likely template by parsing messages, as we do for Git.
- Unify Git and Mercurial logic for acquiring messages, since `getLocalCommitInformation()` now provides this information. This should improve Git performance, and allows us to delete some code.
- Merge messages more intelligently. Currently, we just overwrite fields. Instead, merge arrays (like ccs, reviewers, tasks) and concatenate strings (like summary and test plan). We need to special case "title", see inline.
- @csilvers: this is a precursor to implementing "--reviewers", etc.
Test Plan: See next post, test plan includes giant output.
Reviewers: btrahan, csilvers, Makinde, jungejason, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2536
Summary:
- "git log" still includes "\n", so we're currently generating nonsense hashes like "\nabd9879bab86ad78ab...".
- Correct the log range we use in Git. See comment. When users perform merges, their expectations about what the "included commits" and what the included changes are are different. Represent them with two different ranges.
- Same deal for Mercurial
Test Plan: Ran "arc which" in various contexts.
Reviewers: btrahan, aurelijus, Makinde
Reviewed By: Makinde
CC: aran, nh, jungejason
Maniphest Tasks: T873
Differential Revision: https://secure.phabricator.com/D2460
Summary: Provide far more information about what "arc diff" intends to do.
Test Plan: Ran "arc which" in a variety of circumstances.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran
Maniphest Tasks: T1183
Differential Revision: https://secure.phabricator.com/D2454
Summary: Currently, we ship only the summary, but we need to ship the whole thing for T1189.
Test Plan: Added var_dump() + die, ran in git and hg working copies, verified 'message' included the whole message.
Reviewers: csilvers, btrahan, Makinde
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1189
Differential Revision: https://secure.phabricator.com/D2437
Summary:
This is mostly an onboarding thing, but also allows "arc upload", "arc download", and "arc paste" to work anywhere on the system.
- Try to read the Phabricator install URI from arc global config if we can't find ".arcconfig".
- Build a WorkingCopy anyway if we can't find ".arcconfig", as long as we can find ".svn", ".git", or ".hg".
- Make all the workflows handle "no project ID" at least somewhat gracefully.
Test Plan:
- Ran "arc diff" in .arcconfig-less Mercurial, Git, and Subversion working copies.
- Ran "arc upload" and "arc download" from my desktop.
- Ran "arc paste" from somewhere random.
- Cleared my config and hit the error, got useful instructions.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2424
Summary:
Always use the value in git.default-relative-commit when getting the relative
commit in git.
Test Plan:
Ran arc diff in a repo with git.default-relative-commit set to HEAD^ on a
branch tracking a remote (that is different from HEAD^), and checked that
the diff against HEAD^, not the remote, was published.
Reviewers: jungejason, epriestley, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2409
Summary:
- We no longer need color options since we fake our way through parsing ANSI colorized diffs and use HGPLAIN (on Windows, too!). Drop 'em.
- In the case where you have nothing outgoing, we don't cache the relative commit and thus run "hg outgoing" too many times, which is fairly slow (even if you have nothing outgoing). Cache it.
Test Plan: Ran "arc diff --trace" in a mercurial working copy with nothing outgoing; verified we run "hg outgoing" only once.
Reviewers: Makinde, csilvers, btrahan
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2399
Summary: In Windows, you can't use `X=y cmd` syntax to set variables. Use "set X=y & cmd" instead.
Test Plan:
- Ran "arc diff" in a Mercurial repo in Windows, created D2367.
- Verified this does //not// cause 'HGPLAIN' to be set in the outer shell (where you type "arc diff").
Reviewers: Makinde, tido, indiefan, btrahan
Reviewed By: tido
CC: aran
Maniphest Tasks: T1179
Differential Revision: https://secure.phabricator.com/D2368
Test Plan: Run arc diff locally, verify via git log that the commit is amended afterwards (using the mutable history paradigm)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2369
Summary: Apparently my advice here was terrible, and `--no-decorate` and `--decorate=no` are both very recent additions to Git which a bunch of users don't have. Get rid of them since D2344 allows us to parse all decorate levels anyway.
Test Plan: Tried to google "git changelog", got a bunch of pages about managing changelogs with git.
Reviewers: zeeg, ehren
Reviewed By: ehren
CC: ehren, aran, NorthIsUp
Differential Revision: https://secure.phabricator.com/D2354
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:
- Add branch name tab completion to "arc land".
- Default to landing the current branch.
- This is a little bit hacky but not too terrible. I'm planning to move the whole thing to PhutilArgumentParser at some point so that'll be an opportunity for a big refactor.
Test Plan: Hit tab, landed this branch.
Reviewers: zeeg, btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran, kdeggelman
Differential Revision: https://secure.phabricator.com/D2293
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:
When people create the .arc/default-relative-commit scratchfile with $EDITOR of
choice, their editor usually puts a newline at the end, which breaks arc diff.
We should trim the newline before using the contents of the scratchfile.
Test Plan:
ran arc diff in a working copy that contained a .arc/default-relative-commit
with a newline
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2209
Summary: If you have two projects (say, libphutil and arcanist) and you prepare a patch for one of them on branch "master", run "arc diff", and then prepare a patch for the other one on the same branch, "arc diff" will try to update the first revision when you run it. Instead, make it smart enough to stay within arc projects.
Test Plan: Ran "arc which" in circumstances where it previously generated a false positive, no false positive.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T1100
Differential Revision: https://secure.phabricator.com/D2199
Summary: See discussion in D1861.
Test Plan: Ran "arc diff" on master, got an upstream-based relative commit. Ran "arc diff" on a feature branch, got a config-based relative commit. Ran "arc diff x", got an argument-based relative commit.
Reviewers: btrahan, vrana, davidreuss, elgenie
Reviewed By: davidreuss
CC: aran
Differential Revision: https://secure.phabricator.com/D2192
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:
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:
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: 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:
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:
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:
`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:
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:
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: good title
Test Plan: ran "arc patch DX" a bunch
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T892
Differential Revision: https://secure.phabricator.com/D1789
Summary:
See <https://github.com/facebook/phabricator/issues/97>. Mercurial branch names may contain any characters, but we currently reject branch names with spaces.
NOTE: Mercurial allows you to name branches things like ` ` (five spaces). We do not parse this correctly. Die in a well fire.
Test Plan:
- Added some horrible-but-possible test cases for crazy branch names.
- Unit tests now pass.
Reviewers: btrahan, Makinde, killermonk
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1795
Summary:
On Windows, the PHP function escapeshellarg() replaces '%' with ' ' (space). This is apparently because there is no safe way to escape % inside of strings.
cmd.exe does use "^" as an escape character, so I think replacing "xyz" with "^x^y^z" might work for arbitrary strings (maybe?), or at least some subset of strings, but I don't know cmd.exe well enough to make that call without being concerned I'm introducing a security issue.
Although this patch is dumb, it's certinaly safe, and can only do something wrong if the user has environmental variables like H, P, T, or x01, in which case they're sort of asking for it.
cmd.exe also truncates output on \0, so use \1 as a delimiter instead.
Seriously it's like this thing was written in 1982 and never ever changed.
Test Plan: Created D1783 successfully after applying this patch.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T124
Differential Revision: https://secure.phabricator.com/D1785
Summary: This use of "(cd ..)" is outside of the RepositoryAPI proper (it's when we're trying to figure out which VCS a directory uses) and explodes on Windows.
Test Plan: @koolvin applied this patch manually and got farther than before.
Reviewers: btrahan, Koolvin, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T124
Differential Revision: https://secure.phabricator.com/D1779
Summary:
- This is simpler and reuses more code than doing "(cd %s && ...)" every time.
- If we have an issue like HGPLAIN, we have one place to fix it now.
- On Windows, the construct "(cd %s && ...)" does not mean what we'd like it to.
Test Plan: Ran "arc diff" in a git repo with this change.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T124
Differential Revision: https://secure.phabricator.com/D1761
Summary:
If a user reverts HEAD in the index or working copy, "git diff" shows no changes
and we explode.
Instead, we should simply return no changes; the rest of the lint pipeline
accommodates this correctly.
Test Plan: Reverted HEAD in a working copy, ran "arc lint", got an error.
Applied patch, ran "arc lint", got accurate lint.
Reviewers: Koolvin, btrahan
Reviewed By: Koolvin
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1729
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: See comments. Skip [defaults] in .hgrc when executing commands.
Test Plan: Ran "arc diff" in a mercurial working copy.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T922
Differential Revision: https://secure.phabricator.com/D1707
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
ArcanistRepositoryAPI->loadWorkingCopyDifferentialRevisions()
Summary:
- See T787.
- @cpiro has an immediate use case for this, which is ##arc amend --revision
`arc which --id` --show master## for "git merge --autosquash" or similar.
- For T614, we need this to choose "--create" vs "--update".
- Other workflows should also use this to improve how often we automatically
get things right, particularly in Mercurial and SVN.
Test Plan:
Ran "arc which" in SVN, Git and HG working copies with various flags;
results seemed reasonable.
Reviewers: btrahan, cpiro, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T787
Differential Revision: https://secure.phabricator.com/D1478
Summary:
pretty easy stuff as mercurial accepts git style patches...!
also fixed two issues where we were 1) storing the short hash and 2) storing it
with a trailing "\n". This diff makes us store the full hash AND no trailing
return character
Test Plan:
in my mercurial repo
<note repo at revision $foo>
<did something dumb>
hg commit -m "something dumb"
arc diff
<go to web and fill out stuff for DX>
hg checkout $foo
arc patch DX
<verify patch DX successfully applied!>
use conduit console to verify a few diffs were returning the correct full
revision hash with a trailing \n
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T630
Differential Revision: https://secure.phabricator.com/D1431
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
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: Find the relative commit by finding the first non-outgoing commit, so
we don't show changes caused by merges we've performed since the last time we
pushed.
Test Plan: Checked out two hg working copies, A and B. Made a change in A. Made
a change in B. Pushed B. Merged in A. Made another change in A. Ran "arc diff"
in A. Got only changes I made in A in the diff, not the change from B.
Reviewers: Makinde, btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1339
Summary:
Creates a new hook API that can be used to interface with
SVN/Git/Mercurial in the context of a commit hook. Currently only adds a
function to read the modified file data in a Subversion commit hook.
An object of this API is created in the SvnHookPreCommitWorkflow and
passed on the Lint Engine which then uses it to access current file
data, of the way the APIs seem to be structured); linters use the
getData function which is essentially a wrapper around the engine's
call, with another layer of caching.
Task ID: #770556
Blame Rev:
Test Plan:
- Create a local svn repository and add a minimal hook to run the local
version of arc to test commits
(http://phabricator.com/docs/arcanist/article/Installing_Arcanist_SVN_Hooks.html)
- Create a temporary repository that can trigger any of the linters
available, and test against a temporary linter by committing against
the test repository: the linter should be able to access all required
files by using loadData/getData in the LintEngine and Linter.
Revert Plan:
Tags: lint, svn-hook-pre-commit
Reviewers: jungejason, asukhachev, epriestley, aran
Reviewed By: epriestley
CC: aran, jungejason, epriestley, kunalb, asukhachev
Differential Revision: https://secure.phabricator.com/D1256
Summary: ... in case the head commit is empty. Empty commits are useful for
injecting an Arcanist commit message in a branch then sending the whole thing
off for review. As far as I know there is no situation in which an empty commit
would exist unintentionally and using ##--allow-empty## would suppress an error.
Test Plan: works fine for a branch I just sent for review
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1241
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:
In D1079, I added "--color never", but this flag is provided by the "color"
extension, which is why I missed it originally, because it doesn't show up until
you enable that extension. Providing it causes installs which don't have it
enabled (disabled is the default) to fail.
Use "--config" to disable color instead. This sets a configuration setting and
works regardless of whether the color extension is present.
Test Plan: Ran "arc diff" in a mercurial working copy with the color extension
enabled and disabled.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1092
Summary: We ostensibly now support Mercurial at least mostly, so don't
underpromise quite so much.
Test Plan: Ran "arc diff" in a Mercurial working copy without being warned.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1085
Summary: Mercurial has a --color flag which disables the use of ANSI colors. Use
it to prevent "hg diff" from giving us colorized diff output.
Test Plan: Ran "hg diff --color never", verified it disabled ANSI color in diff
output.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran
Differential Revision: 1079
Summary:
D1061 introduced a 'text file' check, but it fails under SVN for new
directories.
- Revert D1061 (This reverts commit b2cd18252701be2093b52652fb3d1d94c5df571e.)
- Make getChangedLines() return null to indicate that the operation doesn't
make sense. I think this was the intent of the code in the lint engine.
- Fix a bug where running "arc lint" on a change in an SVN working copy from a
subdirectory would fail.
- Fix a bug where warnings with no line information were incorrectly
discarded.
Test Plan:
- Ran "arc lint" in an SVN working copy with a new directory (no failure).
- Forced FilenameLinter to always raise a warning. Added a binary file and ran
"arc lint". The warning was reported for the new binary file, a new text file,
and a new directory.
Reviewers: jungejason, andrewjcg, nh, tuomaspelkonen, aran
Reviewed By: andrewjcg
CC: aran, andrewjcg, epriestley
Differential Revision: 1076
Summary: Apparently I just never tested this or something. Make it work
correctly.
Test Plan: Ran "arc diff" in a Mercurial working copy with a binary file in
outgoing.
Reviewers: Makinde
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1074
Summary: We incorrectly add the "Uncommitted" flag to untracked files, which
causes them to raise various prompts and not respect "--allow-untracked".
Test Plan: Ran "arc diff --allow-untracked" in a clean working copy with
untracked files, was not fatally error'd.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde, epriestley
Differential Revision: 1073
Summary:
In D962, I switched us from using "hg summary" to "hg id" to find the working
copy revision. However, "hg id" reports the revision with a trailing "+" if the
working copy is dirty, so we fail before hitting the explicit check for this
later.
Trim off the trailing '+' if it is present.
Test Plan:
Ran "arc diff" in a dirty Mercurial working copy and got a good error message
about uncommitted changes.
~/repos/hg-working-copy $ arc diff
WARNING: Mercurial support is largely imaginary right now.
Usage Exception: You have uncommitted changes in this branch. Commit (or
revert) them before proceeding.
Working copy: /INSECURE/repos/hg-working-copy/
Uncommitted changes in working copy
hello.c
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1070
Summary:
See D945. We have this kludgy "remote_hooks_installed" mess right now, but we
have enough information on the server nowadays to figure this out without it.
Also reduce code duplication by sharing the "mark-committed" workflow.
This causes "arc merge" to effect commit marks.
Test Plan:
In Git, Mercurial and SVN working copies ran like a million
amend/merge/commit/mark-committed commands with and without --finalize in
various states of revision completion.
This change is really hard to exhaustively test because of the number of
combinations of VCS, revision state, command, command flags, repository state
and tracking state. All the reasonable tests I could come up with worked
correctly, though.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 967
Summary:
Expand 12-character hashes to 40-character hashes so other things will work
properly.
Also use "hg id" instead of "hg summary" to figure out where the working copy
is, since it's substantially simpler.
Test Plan: Ran "arc diff" and got properly 40-character hashes.
Reviewers: Makinde, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 962
Summary: For Mercurial parsing in Diffusion, it looks like the only command we
can use to get move/copy information is "hg status -C --rev <rev>", so add a
parser for it.
Test Plan: Ran unit tests.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 958
Summary: I renamed this at some point but missed a callsite. See T519.
Test Plan: Ran "arc patch" in an SVN working copy.
Reviewers: Girish, jungejason
Reviewed By: Girish
CC: aran, Girish
Differential Revision: 959
Summary: Move code to actually parse "hg" output into a separate class with some
tests, so I can reuse it in the import scripts. We should probably do this for
Git/SVN at some point, too.
Test Plan: Ran unit tests, used this class in Phabricator importers, grepped for
calls to removed private methods.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 942
Summary:
This should support conservative rewrite policies in git fairly well, under an
assumed workflow of:
- Develop in local branches, never rewrite history.
- Commit with "-m" or by typing a brief, non-template commit message
describing the checkpoint.
- Provide rich information in the web console (reviewers, etc.)
- Finalize with "git checkout master && arc merge branch && git push" or some
flavor thereof.
This supports Mercurial somewhat. The major problem is that "hg merge" fails if
the local is a fastforward of the remote, at which point there's nowhere we can
throw the commit message. Oh well. Just push it and we'll do our best to link
them up based on local commit info.
I am increasingly forming an opinion that Mercurial is "saftey-scissors git".
But also maybe I have no clue what I'm doing. I just don't understand why anyone
would think it's a good idea to have a trunk consisting of ~50% known-broken
revisions, random checkpoint parts, whitespace changes, typo fixes, etc. If you
use git with branching you can avoid this by making a trunk out of merges or
with rebase/amend, but there seems to be no way to have "one commit = one idea"
in any real sense in Mercurial.
Test Plan: Execute "arc merge" in git and mercurial.
Reviewers: fratrik, Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, epriestley, Makinde
Differential Revision: 860
Summary:
We have some git-specific logic on main pathways which should be in the API
class, move it around so "arc lint" with an engine works under Mercurial. This
resovles the error @makinde reported:
> PHP Catchable fatal error: Argument 1 passed to
ArcanistBaseWorkflow::parseGitRelativeCommit() must be an instance of
ArcanistGitAPI, instance of ArcanistMercurialAPI given, called in
/home/makinde/.arc_install/arcanist/src/workflow/lint/ArcanistLintWorkflow.php
on line 131 and defined in
/home/makinde/.arc_install/arcanist/src/workflow/base/ArcanistBaseWorkflow.php
on line 830
Test Plan: Ran "arc diff" in git and hg working copies, plus "arc lint" with a
configured "lint_engine".
Reviewers: Makinde, aran, jungejason, nh, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 935
Summary:
- Use "hg parents" to figure out where outgoing changes originate from, not
"~1", since that's a new feature in Mercurial.
- Add "--style default" all over the place since hg config can override this
(similar to the date config issues we saw in git).
- Cache working copy status so we don't run full hg diffs like 30 times
(similar to git/svn APIs).
- Use full "--git" flag instead of rather cryptic "-g".
Test Plan: Ran "arc diff" in my hg working copy, got the diff I expected.
Reviewers: Makinde, aran, jungejason, nh, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 934
Summary:
- Noncontentious Mercurial stuff from D863.
- Use "hg outgoing --branch" to make sure we aren't including outgoing
revisions on other branches.
- Use "hg summary" to figure out where the working copy is, so "hg up <older
rev> && arc diff" works like expected.
Test Plan: Ran "arc diff" from multiple branches with the working copy in
various states.
Reviewers: Makinde, aran, jungejason, nh, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 936
Summary: Parse some other fields which I hadn't seen yet from "hg log". Select
the right "hg log" range for local commit info.
Test Plan: Ran "arc diff" in my test mercurial repository from a branch with
tags. Looked at "local commits" in Differential.
Reviewers: Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 880
Summary: When a revision is created, attach relevant information about the local
commits which it came from if applicable. This supports T473, for DCVSes and
DCVS workflows with immutable history where we can't just amend commit messages.
It will also allow us to enrich the web interface.
Test Plan: Will verify this info shows up for this very diff.
Reviewers: fratrik, aran, jungejason, tuomaspelkonen
Reviewed By: fratrik
CC: aran, epriestley, fratrik
Differential Revision: 857
Summary:
- Build the manifest of file changes so unit and lint workflows work.
- Default to creating a diff between the parent of the first outgoing change
and the tip.
Test Plan:
- Ran "arc diff" in a dirty mercurial repo, got warned about
untracked/uncommitted changes.
- Ran "arc diff" in a clean mercurial repo, got a diff of everything I'd done
locally.
Reviewed By: aran
Reviewers: Makinde, aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 796
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:
right now 'arc blame' uses the user's blame.date setting, but
it will only work when the format is iso. So it will fail for user who
has customized it.
Test Plan:
run arc cover with 'blame.date' set to relative and verified
that 'arc cover' still works
Reviewed By: epriestley
Reviewers: epriestley, codeblock
CC: hwang, aran, epriestley
Differential Revision: 745
Summary:
executes the calls to git in parallel to improve startup performance of
arc lint and possibly other commands
Gets about a 2~3x speedup when repo is in buffer cache, with 4 cores.
Test Plan:
arc linted a repo with unstaged, untracked, staged, and committed
changes, see that the same files were linted.
Reviewed By: jungejason
Reviewers: jungejason, epriestley
Commenters: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 594
Summary:
If you checkout some commit you end up on "(no branch)" which currently breaks
the parser. Ignore that, and add revision IDs to the output. They aren't very
big and I hate flags so I didn't add a flag for this. You can add a flag to turn
them off if you really want.
Test Plan:
Ran "arc branch" and "arc branch --by-status" from a "(no branch)" working copy.
Reviewed By: slawekbiel
Reviewers: slawekbiel, ahupp, jungejason, tuomaspelkonen, aran, schrockn
CC: aran, slawekbiel
Differential Revision: 555
Summary:
Turns out you can hava a branch named |foo.
(Good that '&& rm -fR /usr' is not a valid name)
Test Plan:
created a branch |test, run arc branch
Reviewed By: epriestley
Reviewers: epriestley, dschafer
CC: aran, epriestley, slawekbiel
Revert Plan:
sure
Other Notes:
Differential Revision: 507
Summary:
Appending differential status, sorting, filtering and coloring git
branches.
I think it turned out rather nicely. On my repository with 70 branches
it takes 1.6s, not terrible, though 1.2s is in the conduit call - seems
like there is potential for optimization.
I didn't end up changing 'arc list', as their semmantics are slightly
different, but I'm open to ideas of consolidating them
Test Plan:
- Tested on both facebook www and arcanist repositories.
- Validated that view-all flag works
- Validated that the ordering is correct
- Validated that the statuses match the differential status.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley, slawekbiel
Revert Plan:
sure
Other Notes:
Differential Revision: 497
Summary:
The amend process used "git log HEAD^..HEAD" to get log for the
commit being amended. When run on a merge commit this can return
any number of commits from the non-first parents. Since only a
single commit was expected, arc fails here.
This diff changes the amend process to use the '--first-parent' flag
to be consistent with using '^', which references the first parent.
This should guarantee a single commit log every time.
Test Plan:
arc amend on a merge commit
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley, andrewjcg
Differential Revision: 415
Summary:
Adds "--no-textconv" to all 'git diff' commands so we don't invoke textconv. See
T178 for discussion.
Test Plan:
Added something like this to .gitattributes:
*.txt diff=uppercase
And then this to .git/config:
[diff "uppercase"]
textconv = /path/to/uppercase
...where "uppercase" is a script which takes a file and emits an uppercase
version of it.
Then I added a "wisdom.txt" text file:
The cow goes "moo".
The duck goes "quack".
Without this patch, the file appears in uppercase in Differential, i.e. textconv
runs. With this patch, it appears as the original text.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: elgenie, aran, tuomaspelkonen
Differential Revision: 372
Summary:
Sometimes we need to show abbreviated hashes, passing --short is more
reliable than substring of fixed number of characters.
Test Plan:
- called with and without the parameter, got correct results
-
Reviewed By: jungejason
Reviewers: jungejason
CC: jungejason
Revert Plan:
sure
Other Notes:
Differential Revision: 170
Summary:
Modifying a new file and running 'arc cover' before committing the
modifications confused arc. The problem was that 'unstaged' status
erased 'added' status and this caused problems.
Test Plan:
Tested that arc cover ignores added files when they are modified, but
old files that are modified are still handled correctly by arc cover.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 122
Summary:
I simplified this code at some point and broke it horribly
in the process. Mask in the right flag here and combine the maps
correctly. This solves the great mystery of increased developer
clowniness.
Test Plan:
Ran 'arc diff' in a working copy with staged but
uncommitted files, got an error message (previously, I didn't).
Reviewed By: aran
Reviewers: mroch, aran
CC: aran
Differential Revision: 104
Summary:
SVN has a special 'replaced' status which was not being parsed
correctly. Parse it correctly.
Test Plan:
Replaced a file, ran arc diff, got a sensible diff.
Reviewed By: aran
Reviewers: aran
CC: aran
Differential Revision: 60
Summary:
Git (the world's hardest revision control system) allows you to change
output formats by accident and/or without your direct knowledge. Protect users
from themselves.
Test Plan:
Changed "pretty" in [format] to "format:quack" so every log just
outputs the word "quack". Ran "arc diff" successfully.
Reviewed By: aran
Reviewers: aran
CC: epriestley, aran
Differential Revision: 56
Summary: Haiping is getting a pretty confusing error message when trying to
commit.
Test Plan: Created a mock repository, installed the hook, made commits against
directories with bad .arcconfigs.
Reviewers:
CC:
Summary: When running "arc diff" in a mixed-base-revision working copy, we
prevent the operation. Relax this restriction so that having a different root
revision is okay, so long as all affected files share the same revision. This
facilitates multiple similar edits without updates.
Test Plan: Reverted a file to an older revision in an SVN working copy, ran
"arc diff", was rejected. Applied patch, ran "arc diff", diff went through and
was recorded with the right SVN base revision in the database.
Reviewers:
CC:
