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
