Summary:
When doing an arc diff with pending changes in your working copy
it was creating a new commit with the pending changes instead of amending
the existing one. The problem was the author comparison was comparing
values like "John Smith <john@foo.com>" with "John Smith". The fix changes
$api->getAuthor() to return "John Smith" instead of the full string. This
matches the behavior (and implementation) found in the git api.
Test Plan:
hg book foo
touch a && hg commit -Ama
touch b && hg add b
arc diff
When prompted, amend the pending changes to the existing commit.
Verified that the changes were amended instead of making a new commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5706
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: Detect and fix unconventional spellings of `true`, `false`, `null` and `array` (these are the only keywords I've seen spelled unconventionally in the wild).
Test Plan: Unit tests.
Reviewers: DurhamGoode, btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2985
Differential Revision: https://secure.phabricator.com/D5686
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:
Changes arc diff to choose the base commit as the first ancestor
that has a diff. So if your tree looks like master->A->B->C->D, if you
have a diff on B (which will include A), when you run arc diff on D it will
only include C and D.
This makes the scenario for stacked diffs nicer. A user can commit A, commit B,
arc diff, commit C, commit D, arc diff, arc land B, arc land D.
Test Plan:
Commit A on top of master
Commit B on top of A
arc diff
Commit C on top of B
Commit D on top of C
arc diff
Verify the second diff contains the changes in C and D, but not A and B.
hg up B
arc land --preview
Verify that arc land shows A and B
hg up D
arc land --preview
Verify that arc land shows A, B, C, and D
(arc land should be unaffected by this change.
It always tries to land the entire branch)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5639
Summary:
If we're removing a binary file that didn't have svn:mime-type set properly,
we can't propset it (because the file doesn't exist locally). Instead, just
return a synthetic diff for the removed file.
Test Plan:
run arc diff in an svn working copy where I ran svn rm on a binary file that
doesn't have svn:mime-type set, and the diff correctly gets uploaded to
phabricator instead of erroring when trying to set properties.
Reviewers: epriestley, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5655
Summary:
Previously arc diff for hg only allowed bookmark names, rev numbers,
and commit hashes as the input base commit. This was because it escaped all
inputs and treated them as raw identifiers.
This change makes it treat the input as a revset if the escaped version fails.
This allows users to do things like "arc diff .^" when they only want to diff
the top commit.
Test Plan:
Created a stack of commits, master->A->B.
hg up B
arc diff .^
Verified the diff message only showed B as part of the diff.
arc diff .^~
Verified an error occurred ("Commit '.^~' is not a valid Mercurial commit id.")
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2888
Differential Revision: https://secure.phabricator.com/D5638
Summary:
I'm guessing this was refactored somewhere down the line and it meant
that Python and Perl files were no longer considered text files.
I'm imagining the old regex was: p(hp|y|l). Therefore I blame CSS.
Test Plan:
Perform an arc lint on a Python or Perl file that has trailing whitespace
on a line. It should prompt you for an autocorrecting lint.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5637
Summary:
sry 2 hear of ur lint trubles
(You may need to `arc liberate --force` or delete all your `src/.phutil_module_cache` after updating, see T1486.)
Test Plan: shrug shrug
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5583
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:
This adds a hook to allow external parties to provide config settings at runtime.
The hook is technically for when a RepositoryAPI is created, but that moment can
be used to set new config settings using the new setRuntimeConfig() api.
For example you could have a external hook that looks for keys like 'git:foo.bar'
or 'hg:foo.bar' and writes the value of 'foo.bar' based on whether the repo is a
git or a hg repo.
Test Plan:
Created a hook that looks for hg/git prefix versions of config keys.
Set hg:arc.feature.start.default to be "master" and set arc.feature.start.default
to be "trunk".
Ran arc feature in the hg repo. It made a bookmark on master.
Ran arc feature in the git repo. It made a branch on trunk.
Did it again, but with git:arc.feature... set instead.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5357
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:
On systems with an ancient version of python, the pep8 linter won't run.
Instead of blowing up in the user's face, we should display a nice error
message.
Test Plan:
Put /usr/bin (where the ancient version of python is) at the beginning of
my path and tried to lint some python. I got a nice error instead of a
stack trace.
Reviewers: epriestley, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5356
Summary: Improves Windows compatibility.
Test Plan: Ran failing unit test on Windows.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5344
Summary: Added some sample rcsdiffs for adding and deleting a line from a file. Wrote some test cases to be tested by ArcanistDiffParser.
Test Plan: By making all the test cases pass.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5324
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:
This probably indicates some none fatal error, e.g.:
> remote: Certificate invalid: name is not a listed principal
The best thing here would be to avoid the error but we shouldn't explode even if it is there.
I tried to mute the error from the output but didn't find a switch or config option to do it.
Test Plan:
$ hg outgoing --branch default --style default
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5267
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: This shell script needs more quotes.
Test Plan: Ran "arc" with `arcanist/` inside a directory called `s p a c e s`.
Reviewers: irinav, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5265
Summary: unlike the hooks I copy/pasted, these hooks have parameters
Test Plan: read it
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5264
Summary: These hooks allow test cases to build shared resources -- notably, database fixtures.
Test Plan: See next diff.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5258
Summary: We don't set $paths when running --everything.
Test Plan: Ran `arc unit --everything` with coverage.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5260
Summary: Also allow `sTaTiC::$x` which is a valid PHP (contrary to `sElF::$x` which is invalid PHP but I allowed that too).
Test Plan:
new self;
Reviewers: epriestley
Reviewed By: epriestley
CC: wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5241
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:
Added a lint rule that warns about reusing iterator reference
variables.
Test Plan:
- Add a file with examples found in with https://secure.phabricator.com/T2536
- Did not make a unit test yet
Reviewers: vrana, bill, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Maniphest Tasks: T2536
Differential Revision: https://secure.phabricator.com/D5179
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:
There was an accidental ! in the phase vs outgoing condition
which caused it to use 'hg outgoing' when it should have used the draft()
phase. Fixing this shaves 4.5 seconds off 'arc diff' on large repos.
Test Plan:
Ran arc diff --trace. Noted that the draft() was used and that the diff
contained the correct files and commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5182
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:
arc diff called 'hg cat' twice for every image binary in the diff.
This turns out to take 1 second per call on a large repo because mercurial
has to parse the manifest every time.
Now arc diff batches up all the files and does only two 'hg cat'
commands. This makes the cost constant relative to the number of
images being uploaded.
Test Plan:
Ran arc diff on a diff with 30 images on both git and hg.
Verified that it was fast and that the images showed up in the web ui.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, dschleimer, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5144
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:
People constantly forget to bump the linter version and I don't see a way how to stop it.
This may bump the version even if it wouldn't be required but let's rather undercache than overcache.
Test Plan: `var_dump($version)`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5129
Test Plan: Threw in `didRunLinters()` of one linter, still saw the result of other linters and "Some linters failed" at the end.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5124
