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:
We currently use the language "relative commit" or "relative local commit" to refer to the head of a commit range. I want to move toward callign this a "base commit", since I think that makes more sense. This moves us a small step in that direction.
The DSL stuff in T1233 also needs access to this stuff, so move it up to the base. This is mostly a bite-sized piece of the change in that diff.
Test Plan: Ran "arc which" in a couple of circumstances, read explanations.
Reviewers: dschleimer, vrana
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2747
Summary:
Using .arc as the scratch and per-repository configuration directory
has some unfortunate consequenses in the real world. Among other
things, people forget to .gitignore it so it gets checked in.
Test Plan:
the only thing that seems to use this is the relative commit setting
for git. This diff consists of 2 commits, one for the .gitignore and
one for everything else.
Comment out the portion of my .git/config that defines the upstream
for the branch. Run arc diff --only with HEAD^ in
.arc/default-relative-commit. See that .gitignore is not included in
the resultant diff, that .arc no longer exists, and that .git/arc
exists and has HEAD^ in .git/arc/default-relative-commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2725
Summary:
This adds basic support for mutable history for arc diff and arc amend for
mercurial. This is purely opt-in (so it shouldn't affect anyone who doesn't want
this feature) by explicitly setting
"immutable_history" : false
in arc configuration.
This also fixes another instance of weird behaviour for multiple heads - the
first instance was fixed here:
https://github.com/facebook/arcanist/pull/28
Test Plan:
without "immutable_history" turned on)>
When ##arc diff## produces an update diff, it should list only commits that are
ancestors of the current revision - not ones from other heads.
Reviewers: epriestley
CC: csilvers, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2654
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
