Summary:
Fixes T2266. Motivation:
- The lint cache does not always invalidate correctly. Because of the nature of the cache, this is a hard problem (right after "naming things").
- We already have a fair amount of complexity in trying to invalidate it, and are still discovering new places where it doesn't work (e.g., Windows with "/" vs "\" paths).
- One invalidation failure is when linter code changes, which seems unresolvable in the general case (e.g., changes to external linters).
- It's not obvious what's happening when the lint cache causes some kind of issue.
- Particularly while developing or debugging linters, your changes often won't be reflected in the lint output. Some of this is theoretically tractable but the external linter case probably isn't.
- When someone reports a problem with the lint cache in IRC or elsewhere, there is essentially never a way for me to fix it. The lint cache can't be debugged effectively without access to a working copy where the problem reproduces.
- The cache provides limited benefit outside of Facebook's install.
To remedy these issues:
- Introduce configuration which controls cache usage.
- Default it off.
- Print a message when the cache is in use.
(I'd tentatively support removing the cache entirely, but I don't know how @vrana and Facebook feel about that.)
Test Plan: Ran `arc set-config --show`, `arc lint --cache 0`, `arc lint --cache 1`.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, mbishopim3, nh, edward, wez
Maniphest Tasks: T2266
Differential Revision: https://secure.phabricator.com/D5766
named the same as a file path.
Summary: (In fbcode and www at least, the only places I tried) if a
branch happens to be named the same as a file path (from root) (like if
a branch is called `spec` and there is a directory in `www` called
`spec`) then `arc land` will fail with git complaining that the argument
(to `git log ...`) is ambiguous as it could refer to both a path and a
revision; so this diff adds a `--` to the end so that git knows that
both are revisions and not paths.
Test Plan: Try and land something from www (I did, see D752219) where
the branch is named `spec`.
Reviewed by: epriestley
Test Plan:
$ set # on Windows
Reviewers: epriestley, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5733
Summary:
Arc lint for git is currently adding all untracked files when it
amends the commit with lint fixes. This changes the git add -A to be
git add -u. This only adds files that were already tracked. -A was adding
untracked files as well which was not the desired behavior here.
Test Plan:
Create an untracked file.
Commit a lint failure another file.
arc diff and choose to amend the lint patches.
Verify that the untracked file was not added but the tracked file was amended.
Reviewers: epriestley, wez, chad
Reviewed By: chad
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5731
Summary:
arc lint was hardcoded for git for amending commits with lint
patches. This enables the same functionality for mercurial.
Test Plan:
Made some changes that would result in a lint patch.
arc diff
Verify that the patches it produces were amended into the commit.
Verified it still works in git as well.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5716
Summary:
@ender is reporting a parsing issue in SVN, but we don't build a parser with setWriteDiffOnFailure() set in this workflow right now so I can't get the raw file to fix the issue.
Use the onboard mechanism to build a parser with `setWriteDiffOnFailure()` set, so it will write the diff, so I can get a copy so I can fix the problem.
Test Plan: Ran `arc diff --preview` in an SVN repo with a linter.
Reviewers: ender, btrahan
Reviewed By: ender
CC: aran
Differential Revision: https://secure.phabricator.com/D5699
Summary:
This adds a arcconfig setting to allow specifying whether to use the merge
or rebase strategy when doing the feature branch update.
arc.land.update.default can be set to either 'rebase' or 'merge'. The command
line flags will override this setting.
We have had trouble with arc land producing merge commits (introduced
with D4080) in git. They usually appear when arc land fails, and our users
are confused by the presence of a merge commit afterwards. Today it got even
worse since a user managed to get arc land to push the merge commit to the
server. This setting will allow us to turn it off for our uses.
Test Plan:
Verified the following combinations:
update.default not set + arc land (saw git merge in the trace)
update.default = 'rebase' + arc land (saw git rebase)
update.default = 'merge' + arc land (saw git merge)
update.default = 'rebase' + arc land --update-with-merge (saw git merge)
update.default = 'merge' + arc land --update-with-rebase (saw git rebase)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5683
Summary:
The `emailuser` template is a relatively recent addition to Mercurial, and a few users have complained about it. It also doesn't actually do what I thought it did, e.g. in an address like this:
"Abraham Lincoln" <alincoln@whitehouse.gov>
^^^^^^^^^^^^^^^ ^^^^^^^^
(1) (2)
^^^^^^^^^^^^^^^^^^^^^^^
(3)
...I want (1), but `emailuser` means (2). Instead, extract (1) with `getDisplayName()` and (3) with `getAddress()` using PhutilEmailAddress.
The implementation in Mercurial is not particularly sophisticated or magical (it just looks for "@" and "<") so we aren't really missing anything by doing this ourselves, at least today.
Also fix some issues in `arc export`, which literally no one uses, but which is occasionally useful for testing (as here).
Test Plan:
- Ran `arc diff --only` in an `hg` repo, checked DB to see that name/email were correctly extracted.
- Ran `arc export --git` in an `hg` repo, didn't get a long series of fatals.
Reviewers: btrahan, DurhamGoode
Reviewed By: DurhamGoode
CC: aran
Maniphest Tasks: T2866, T2858
Differential Revision: https://secure.phabricator.com/D5539
Summary:
Arc land is a bit magical and some users have gotten bitten by
the fact that it collapses and lands every commit on the branch. To make
it explicit what is being landed, it now shows a list of the commits
that are being landed. I also added a --preview flag that will just
print the commits that would be landed, but does nothing else.
Hopefully this make arc land a little less magical for people.
Test Plan:
arc land in the following scenarios:
- Landing one change
- Landing no changes
- Landing a stack of changes
Did it with hg and git.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, sid0, dschleimer, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5460
Summary:
Previously, arc patch would create a new commit under the existing
current bookmark in mercurial. There have been two discussions about what the
right behavior should be (D3334 and D3658). One side wants no commit at all,
and one side wants a commit under a new bookmark. The current implementation is
the worst of both worlds :(
This change makes it create a new bookmark at the revision's base before commiting,
same as the --bookmark flag used to do (which is now obsolete). That way the
existing bookmark doesn't move (in mercurial >=1.8). This is the same behavior
git has, which is convienent for groups migrating between the two.
Also makes hg's getCanonicalRevision handle svn revisions just like git. This way
arc patch will try to apply the patch to the appropriate revision in the history.
Test Plan:
Ran:
arc patch - Verified it created a new bookmark and commited on top of the
revision's base commit.
arc patch --nobranch - Verified it put the new commit on top of the current
bookmark without a new bookmark.
arc patch --nocommit - Verified it left all the changes in the working copy.
Also verified arc patch still works with git.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5408
Summary:
- Added arc.autostash option to have this behaviour off by
default (but configurable on a per-project basis).
- Automatic stashing of changes now informs the user of how
to restore their working directory if Arcanist unexpectedly
terminates.
- Fixed an issue with finalizeWorkingCopy when the workflow
didn't require a clean working copy.
Test Plan:
Test `arc diff` when there are changes in the working
directory; by default it should tell you to stash or commit.
Turn on the arc.autostash option and try again; it should
automatically stash with a message on how to recover, and
it should restore the working directory automatically under
almost any circumstances (other than an unrecoverable error).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5385
Summary:
Adds error handling for several kinds of failure in arc land for
mercurial. Previously it would often leave the repo in a confusing state
if something failed.
- Aborts the land if pull brings in a diverged 'onto' branch.
- Aborts the rebase if there is a conflict. This leaves the repo exactly as
it was before, so the user is not left with a half finished rebase.
- Don't delete the original non-squashed branch until the push succeeds.
- If the push fails, strip the temporary squashed commit. This leaves the
'onto' branch back on the latest commit from the server, and leaves the users
original nonsquashed branch around.
- Always leave the user back on their original branch after an error.
Test Plan:
Ran arc land:
- with pull causing a diverged 'onto' bookmark
- with the 'onto' bookmark already diverged
- with the rebase causing conflicts
- with a push that failed due to a commit hook
- with a successful land
- with a successful collapse and land
In all failure cases the repo was left exactly as it was before arc land,
except for the push-failed case, where the only change was that the branch
was correctly on top of the destination branch due to a successful rebase.
Used bookmark name "foo bar-gah" to test that crazy bookmark names still work.
Reviewers: epriestley, dschleimer, sid0
Reviewed By: epriestley
CC: nh, wez, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5394
Summary:
Arc patch was committing with -A (--addremove) which meant any random
files that were sitting around in the repo (like conflict .orig files) were
added to the commit. The -A isn't even necessary since the hg import
adds and removes all the appropriate files for you.
Test Plan:
touch foo
arc patch --diff some-diff-id
Verified that foo was not added to the commit
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: dschleimer, bos, sid0, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5396
Summary:
Changes arc feature to read 'arc.feature.start.default' instead
of 'arc.land.onto.default'. In our usage we actually need to fork off
a different branch than we land to, so separating these is useful.
Test Plan:
Set arc.land.onto.default = master
Set arc.feature.start.default = bar
arc feature foo
cat .git/config
Verified the foo branch tracked bar
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: wez, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5336
Summary: If user changes the file contents during linting (usually when prompted to apply a patch) then we save the old messages to the new file contents. Fix that by computing the hash before linting (or after applying patch).
Test Plan: Changed the file during linting, verified that the file hash didn't change.
Reviewers: epriestley
Reviewed By: epriestley
CC: ptarjan, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5320
Summary: I looked at the pros & cons at adding hooks in git/hg vs arc land and I prefer arc land.
Test Plan:
* added an event listener and made sure I could handle the event.
* made sure things get reverted when the event handler throws an exception.
Reviewers: vrana, epriestley, pieter
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5268
Summary: The message suggests that only one revision would land.
Test Plan: Read it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5277
Summary: Have arc land inspect the revision if it depends on some other revisions which haven't been closed yet. If yes, then warn users.
Test Plan: Will test them locally.
Reviewers: epriestley, AnhNhan
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5262
Summary:
Adds arc.feature.start.default arcconfig setting to specify
a default value for 'start' in 'arc feature name start'. This lets
users always branch from origin/master (or whatever the main branch is).
Also cleaned up the 'feature' help text a little. The stuff about sorting
and closed/abandoned revisions is explained via the options list already.
Test Plan:
Ran arc feature with/without a start and with/without the config
setting set.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: bos, sid0, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5184
Summary:
When using arc feature, it should set up the tracking branch to be
master.
Test Plan:
./bin/arc feature tracking
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5169
Summary: These are the errors I really do.
Test Plan:
$ arc diff --no-lint
(Assuming '--no-lint' is the British spelling of '--nolint'.)
$ arc diff --reviewer a
(Assuming '--reviewer' is the British spelling of '--reviewers'.)
New unit test.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5185
Summary: Changes message for `arc land` that displays current branch or bookmark (if none is specified) to appropriately use the term 'bookmark' when on a bookmark in an hg repository.
Test Plan:
Ran `arc land` on new git and hg repositories, checking for correct identification of 'branch' or 'bookmark'.
~/test$ mkdir hg-test
~/test$ mkdir git-test
~/test$ cd hg-test
~/test/hg-test$ hg init
~/test/hg-test$ hg branch
default
~/test/hg-test$ hg bookmarks
no bookmarks set
~/test/hg-test$ arc land
Landing current branch 'default'.
Usage Exception: You can not land a branch onto itself -- you are trying to land 'default' onto 'default'. For more information on how to push changes, see 'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the documentation. You may be able to 'arc amend' instead.
~/test/hg-test$ hg bookmark testmark
~/test/hg-test$ hg bookmarks
* testmark -1:000000000000
~/test/hg-test$ hg branch
default
~/test/hg-test$ arc land
Landing current bookmark 'testmark'.
Usage Exception: Source testmark is a bookmark but destination default is not a bookmark. When landing a bookmark, the destination must also be a bookmark. Use --onto to specify a bookmark, or set arc.land.onto.default in .arcconfig.
Confirm still works on a git branch:
~/test/hg-test$ cd ../git-test/
~/test/git-test$ ls
~/test/git-test$ git init
Initialized empty Git repository in ~/test/git-test/.git/
~/test/git-test$ touch testfile
~/test/git-test$ git commit -am 'Test file'
~/test/git-test$ git branch
* master
~/test/git-test$ arc land
Landing current branch 'master'.
Usage Exception: You can not land a branch onto itself -- you are trying to land 'master' onto 'master'. For more information on how to push changes, see 'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the documentation. You may be able to 'arc amend' instead.
Reviewers: DurhamGoode, epriestley
Reviewed By: DurhamGoode
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D5163
Summary:
Some tests take longer (fixtures usually around 1 second for me) and also FB runs all tests on deploy.
I want to see all results immediately.
Test Plan:
Added `usleep(200000)` to `resultTest()`, then:
$ arc unit
Saw results printed one by one.
Also didn't pass `$renderer` to `ArcanistPhutilTestCase` and saw empty output.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5141
Summary:
Previously, running arc lint on a set of changes that only
existed in your working copy threw an exception in mercurial repos.
It was trying to use the revset "...." (i.e. the range from . to .),
which didn't parse. Even if I fix that it still doesn't work because
getRawDiffText did not include the working copy changes (which it does
in git). I removed the check so the function now acts the same as in
git and arc lint works on working copy changes. I've seen this error before
in other places so hopefully this change will also fix any other areas,
that depended on getRawDiffText working the same as git.
The logic I removed was added in D1954 to support diffing against
uncommited changes. That workflow should be unchanged. arc diff will
still prompt the user if there are uncommited changes, and the user can
still choose to abort or continue.
Let me know if I missed something important which makes this a bad idea.
Test Plan:
Edited a file in the working directory of a hg repo.
arc lint
Verified lint ran successfully.
Also ran arc diff and land with and without working copy changes to make sure
they still work. I'd kill for some tests in this area...
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: dschleimer, bos, sid0, aran, Korvin
Maniphest Tasks: T1631
Differential Revision: https://secure.phabricator.com/D5130
Summary:
I want to run lint on background and I'm interested only in side effect of caching (and maybe exit status).
This is better than discarding stdout later because we don't do unnecessary work and error conditions are still printed.
Test Plan:
$ arc lint --output none # with error
$ echo $?
$ arc lint --output none # with no lintable paths
$ arc lint --output none # witout errors
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5106
Summary: If there's other revision in last commit message and I said `--create` then it is clear that I want to create a new commit.
Test Plan:
$ arc diff --create # in dirty working copy on top of my open revision
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5078
Summary: If I have //Differential Revision// in my commit message then `arc diff --create` updates that revision instead of creating a new one.
Test Plan:
$ arc diff --create # on top of commit message with Differential Revision
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5077
Summary:
The arc feature command wasn't actually creating bookmarks
on mercurial. It needs to call 'hg bookmark' instead of 'hg update'
Also removed an unnecessary hgsprintf since there were no arguments.
Test Plan:
Ran 'arc feature foo' and 'arc feature bar tip^'.
The former created a bookmark at my current location.
The latter created a bookmark at tip^ and moved me to that revision.
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5023
Summary:
Mercurial 'arc land --hold' was taking 90+ seconds on our large
repository. Since most of arc land doesn't require any particular working
directory, I've changed the mercurial logic to avoid all updates except for
two: the one prior to finding the revision (only applies if the user specified
--branch), and the one at the end to leave the user in a good state.
Also got rid of a 'hg outgoing' call when phases are supported. Also changed
the hg-subversion detection to just look for .hg/svn instead of running 'hg
svn info', which was taking 4 seconds.
Now arc land takes about 50 seconds. Still much worse than git's 25 seconds.
One big hot spot is in the two 'hg rebase' calls, which account for 25 seconds
(versus 11 seconds of git).
Test Plan:
Tested arc land with mercurial and git. Tested with and without the --branch
options.
Reviewers: epriestley, bos, sid0, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5014
Summary:
Arc diff will hash file data and try to upload the file using upload by hash rather than transferring data. If it is unable, it defaults to its normal behavior
Attempts to upload file by hash, use regular upload method otherwise
Test Plan: Figure out how to arc diff to my local install and look at the behavior
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, mehrapulkit
Differential Revision: https://secure.phabricator.com/D4968
Summary: Followup to D4703. When we give paths to `svn`, we need to escape them if they contain an `@`.
Test Plan:
Created a patch full of modifications to files with `@` in their names, and applied it:
$ arc patch --diff 192
A A@2xcopy2
A A@2xcopy
D A@2x
OKAY Successfully applied patch to the working copy.
Reviewers: chad, mbishopim3
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4977
Test Plan:
$ arc diff -a
$ arc diff -a # saw amend instead of a new commit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4947
Summary:
We save repository version to lint cache but ignore it when reading the cache.
Fix it.
Test Plan: Made an error for linter with repo granularity, deleted the error from the cache. Relinted, didn't see the error. Changed another file and relinted, saw the error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4841
Summary:
Mercurial 'arc land' uses the 'hg strip' command to clean up after
itself, but this command isn't available unless the mq extension is enabled.
The fix is to enable it for that particular command only.
Test Plan: Ran 'arc land' with the mq extension disabled. It worked.
Reviewers: epriestley, bos, sid0, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4955
Summary:
Previously 'arc diff X' with mercurial meant to use X as the base
to diff against. Now it means use gca(X,working directory) as the base to
diff against. This matches the git behavior.
Test Plan:
Ran 'arc diff master' on a repo where master was ahead of the feature branch.
Verified that the diff result included only the diffs in the feature branch.
Reviewers: epriestley, sid0, bos, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4865
Summary: Makes sense after D2471.
Test Plan:
Swapped two binary files, ran `arc diff --only`.
Saw time 3.797 s instead of 4.361 s.
Changed `file.upload` to `file.uploa`, saw proper error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4847
Summary: If provided, have `arc patch` use `authorName` / `authorEmail`. This simplifies handling and makes patches more portable between version control systems (previously, information was generated in the diff's VCS, regardless of which VCS it was being applied to).
Test Plan: Created a diff with author `derp <derp@derp.com>`, ran `arc patch --diff x`, got a local commit with that author.
Reviewers: btrahan, edward, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4827
Test Plan: Copied the code in a script, changed `phutil_passthru()` to `echo csprintf()` and ran it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4805
Summary:
FB currently starts Sandcastle push before starting the diff workflow.
If `arc diff` commits something then we need to restart the push.
I want to avoid this by starting the push after commit.
Test Plan: Will test after implementing the listener.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4785
Summary: Currently, we don't expose these at top level, so you can't disable coverage if your coverage is explosively broken. Expose them as passthrough arguments.
Test Plan:
- Touched a file in `arc` which triggered unit tests.
- Without `xdebug` installed:
- Ran `arc diff --preview`, `arc diff --preview --no-coverage` (both fine).
- Ran `arc diff --preview --coverage`, got exception about coverage not being available.
- Installed `xdebug`.
- Ran `arc diff --preview`, got coverage.
- Ran `arc diff --preview --coverage`, got coverage.
- Ran `arc diff --preview --no-coverage`, no coverage.
Reviewers: indiefan, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4745
Summary: `git pull` may fail in git-svn after rebasing (which is a side effect of dcommit).
Test Plan:
$ git svn rebase
$ git log trunk..master
$ git pull --ff-only; echo $?
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4716
Summary: Some people have 2GB+ untracked files in repo which significantly slows down this or even crashes it.
Test Plan: Added a debug output here and linted repo with untracked path.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, arudolph
Differential Revision: https://secure.phabricator.com/D4713
Summary: Fixes T2438. We currently escape everything with '@', but SVN rejects that for '.'
Test Plan:
Unit tests. Performed this commit:
$ svn st
M .
A x@123
$ arc commit --conduit-uri=http://local.aphront.com:8080/ --revision 53
Revision 'D53: asdf' has not been accepted. Commit this revision anyway?
[y/N] y
Committing 'D53: asdf'...
Sending .
Adding x@123
Transmitting file data .
Committed revision 37.
Done.
I grepped for more '@' adding but couldn't find any. It's a bit tricky to grep for though, so it's possible I missed some.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T2438
Differential Revision: https://secure.phabricator.com/D4703
Summary:
arc land on a hg-svn repository would fail because arc land
uses 'hg push -r' to specify which revs to push which is not supported
by hg-svn. Now we just use 'hg push' which, when used against svn, only
pushes the current branch (which happens to be the branch we're trying to land).
We can't use standard 'hg push' for non-svn repos though because when used
against a vanilla hg repo 'hg push' pushes all branches.
Also remove --new-branch from 'hg push' because it's extremely
unlikely that a person wants to create a new branch on the server via
arc land.
Test Plan:
Ran arc land on a normal hg repo, verified it used 'hg push -r'.
Ran arc land on a hg-svn repo, verified it used 'hg push' and it pushed the
correct changes.
Reviewers: epriestley, sid0, dschleimer
Reviewed By: epriestley
CC: bos, aran, Korvin
Maniphest Tasks: T2403
Differential Revision: https://secure.phabricator.com/D4653
Summary:
The cache key is repository base revision and hash of all modified files.
It isn't perfect in SVN where every file might have a different revision.
It's also suboptimal as just committing or amending changes the cache key even if the files contents weren't modified.
We can improve it later, perhaps by using previous revision and files modified since it.
Test Plan:
Changed granularity of XHPAST linter to repository.
Linted the same repo twice, verified that it was read from cache the second file.
Changed a single file, verified that all files were re-linted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4608
Summary: The main reason for this is to not exit with 1 when no paths are lintable (which is more success than failure).
Test Plan:
Returned empty array from `buildLinters()`, then:
$ arc lint
$ echo $? # 0
Reviewers: epriestley
Reviewed By: epriestley
CC: wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4625
Summary: Currently, we get an exception on empty %Ls for `arc diff --no-ansi` or similar (see P698).
Test Plan: Ran `arc diff --no-ansi`.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4624
Summary:
Previously, trying to arc land in a mercurial repo would
fail if the local branch was already at the tip of the onto branch
(since hg rebase exited with code 1). This change makes it check
if a rebase is needed before executing the rebase.
Test Plan:
hg init foo
cd foo
hg bookmark master
touch a && hg add a && hg commit -ma
// setup your .arcconfig
cd ..
hg clone foo foo2
cd foo2
hg bookmark mybook
touch b && hg add b && hg commit -mb
arc land --onto master --revision <your rev number>
Arc land should succeed. I also tried landing when a rebase was
necessary and it still worked.
Reviewers: epriestley, dschleimer, bos
Reviewed By: epriestley
CC: sid0, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4588
Summary:
A git submodule looks a lot like a normal git repo, but the .git
directory is replaced with a file that git reads to find the real
location of the git directory. When arcanist tries to write a file into
a directory inside of there, it was failing silently, and then crashing
silently when it couldn't read results back out. Instead of assuming the
git directory is a directory named .git at the toplevel of the tree, we
use the appropriate git command to get the correct git directory.
Test Plan: submit a diff from a submodule
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4482
Summary:
If linter with file cache granularity stops linting then we don't run it on the same path next time.
But we still run linters with non-file cache granularity causing that they run even on the paths that would be stopped otherwise.
Test Plan:
Put global granularity linter after Generated linter.
Caused an error from this linter but in ignored path.
Verified that 'stopped' is saved in `lint-cache.json`.
Linted the same files second time, verified that the path is still skipped (wasn't before).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4446
Summary: Add a comma because it was going to annoy the crap out of me.
Test Plan: See the comma. :)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4365
Summary: We want to use TYPE_DIFF_DIDBUILDMESSAGE to abort arc diff when a message doesn't fit some
Test Plan: Created an EventListener subscribed to TYPE_DIFF_DIDBUILDMESSAGE, validated the 'message' field was filled in
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D4361
Summary:
Currently, this spawns 125 concurrent processes on my machine, which overflows some limit and gives me an error:
PHP Warning: proc_open(): unable to create pipe Too many open files in /INSECURE/devtools/libphutil/src/future/exec/ExecFuture.php on line 491
Instead, limit parallelism to 16. The runtime is approximately the same for me, and dominated by other concerns (conduit calls).
Test Plan: Ran `arc branch` successfully. Ran `arc branch --trace`, observed behavior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4336
Summary: see title
Test Plan: ran arc install-certificate <uri> in a directory without an .arcconfig and it worked! ran arc install-certificate in a directory with an .arcconfig and it worked! ran arc install-certificate <uri> in a directory with an .arcconfig and noted it correctly overrode the .arcconfig
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2251
Differential Revision: https://secure.phabricator.com/D4332
Summary:
Fixes T2138.
- When a pull fails, restore the original branch.
- When a push fails, complain about it really loudly.
NOTE: No test plan for push yet since I'm not sure this is the right remedy, see T2138 for discsusion.
Test Plan:
- Tested pull by changing "git pull" to "git xxpull" and running "arc land". Saw the pull fail and my original branch restored.
Reviewers: vrana, aran
Reviewed By: vrana
Maniphest Tasks: T2138
Differential Revision: https://secure.phabricator.com/D4265
Summary: If you are explicit then there is no need to ask you.
Test Plan:
$ touch a
$ arc diff
$ arc diff a
$ arc diff existing
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4186
Summary:
Recently, in D4097 or one of the precursors I refactored this. However, when $rev is null parseBaseCommitArgument() throws ("This VCS does not support commit ranges."). Shield the call so it only happens if if $rev is nonempty (we still want to make the call, so "arc lint --rev x" on SVN will throw and inform the user that "--rev" is incorrect usage).
(@vrana, this was reported by FB and might be worth pushing.)
Test Plan: Ran "arc diff --preview <path>". Grepped for other parseBaseCommitArgument() callsites and verified they don't have similar issues.
Reviewers: vrana, btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4241
Summary: I don't know how to not be strict here plus we (Arcanist developers) don't have access to user's error log.
Test Plan:
Undeclared `ArcanistDiffWorkflow::$console`, then:
$ arc diff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3607
Summary:
The main added value is loading the branch name from revision.
Sometimes I know the revision ID but I don't know the branch name.
The missing piece is the starting point of the branch.
I was thinking about using `arc.land.onto.default` but we also need to get 'origin'.
This is also the last step of a simple workflow where underlying VCS is not abstracted away.
In future, we can implement this for other APIs.
Test Plan:
$ arc branch new_branch
$ arc branch new_branch
$ arc branch D4168
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4170
Summary: After D4191 this is a fatal.
Test Plan: Created this revision.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4219
Summary: Adds "arc unit --everything", which runs every available test, provided the test engine supports it. Also add JSON output.
Test Plan: Ran `arc unit --everything` in arcanist/, libphutil/ and phabricator/. Saw all tests run.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2065
Differential Revision: https://secure.phabricator.com/D4214
Summary:
See D4049, D4096.
- Move commit range storage from Mercurial and Git APIs to the base API.
- Move caching up to the base level.
- Store symbolic name and resolved name separately, so we can re-resolve the correct commit from the symbolic name after dirtying caches.
- Rename `supportsRelativeLocalCommit()` to `supportsCommitRanges()` (old name wasn't very good, and not consistent with new terminology like the `--base` flag).
- Rename `getRelativeCommit()` and `setRelativeCommit()` to `getBaseCommit()` and `setBaseCommit()`.
- Introduce `reloadCommitRange()` and call it from `reloadWorkingCopy()`.
I think this fixes the problem in D4049, and provides a general solution for the class of problems we're running into here, with D4096. Specifically:
- We no longer get dirty caches, as long as you call reloadWorkingCopy() after changing the working copy (or call a method which calls it for you).
- We no longer get order-of-parsing-things problems, because setBaseCommit() reloads the appropriate caches.
- We no longer get nasty effects from calling `requireCleanWorkingCopy()` too early.
Test Plan: This is pretty far-reaching and hard to test. Unit tests; ran various arc commands. :/
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4097
Summary:
This method is used in three cases:
# For unit tests, to set the range to 'HEAD^' or '.^' in an agnostic way.
# For "amend", to set the range to the commit to be amended (also 'HEAD^' or '.^').
# For "patch" and "upgrade" so we don't fail just because there's an invalid "base" rule somewhere in the config when doing clean-working-copy tests.
For cases (1) and (2), introduce an "arc:this" rule to mean "the current commit". For case (3), remove the call; it is no longer necessary to check the commit range in order to do tests for the working copy state after D4095.
Test Plan: Ran unit tests, "arc upgrade", "arc patch", "arc amend".
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4096
Summary:
This allows using new methods without the need for bumping version number.
The usage is not neccessary because we already bumped the version number for this but I wanted to have a callsite.
Test Plan:
Made a typo in method name, then:
$ arc lint --only-new 1
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4193
Summary:
This changes arc land's hg support to allow you to land a branch or bookmark that has nested branches/bookmarks. Example:
// Initial state:
// -a--------b master
// \
// w--x mybranch
// \--y subbranch1
// \--z subbranch2
//
// arc land --branch mybranch --onto master :
// -a--b--wx master
// \--y subbranch1
// \--z subbranch2
Test Plan:
Created several repos like in the summary and ran 'arc land' and 'arc land --keep-branch'. Did this with both bookmarks and named branches. Scenarios tested:
- mybranch having no child commits
- mybranch having a child branch with several commits
- mybranch having two child branches
- mybranch having a child branch which has two more child branches
No code was added outside of a "if ($this->isHg)" so I didn't run git arc land.
Reviewers: epriestley, dschleimer, bos, sid0
Reviewed By: dschleimer
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4089
Summary:
See chatlog from https://secure.phabricator.com/chatlog/channel/%23phabricator/?at=37257
Basically, the install's developers sometimes use "git merge master" instead of "git rebase master" to pull changes from master into their branch. When we run "git rebase master" at the end, this creates a lot of conflicts.
Instead, optionally run "git merge master". This should be equivalent in our case (where we always rebase) and much better in their case (where they sometimes merge).
We're both going to use it for a bit and see if it creates problems. If it improves the "sometimes-merge" workflow without affecting the "always rebase" workflow, we can make it a default. If it improves theirs but damages ours, we can keep it a flag/option. If it's just terrible, we can figure out something else.
Test Plan:
Ran `arc land --mergeup --trace <feature> --keep-branch --hold`, see P624 for output. Observed merge update strategy and general success.
Also verified the conflict:
$ arc land --mergeup --merge
Usage Exception: Arguments '--mergeup' and '--merge' are mutually exclusive: The --merge strategy does not update the feature branch.
Reviewers: btrahan, vrana, DurhamGoode
Reviewed By: btrahan
CC: aran, keir
Differential Revision: https://secure.phabricator.com/D4080
Test Plan: Added a debug output there and ran.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4147
Summary:
arc browse will open a browser to the Diffusion view of a file. Convenient if you like
Diffusion for reading source. Naturally, it fixes relative filenames.
Combined with git-grep it can be an easy replacement for server-side search functions.
Test Plan:
use feature with and without 'browser' configured.
I've only tested this on Linux, because that's all I have right now, but the principle is sound.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4127
