Summary: Ref T13481. Some older versions of Git appear to not support "--" in these commands. Just drop it. This can lead to ambiguous results with certain obviously-silly remote names, but doesn't appear to lead to anything dangerous.
Test Plan: Will followup with user on ancient Git.
Maniphest Tasks: T13481
Differential Revision: https://secure.phabricator.com/D20952
Summary: Fixes T13481. When identifying the URI for a remote, fall back from "git remote get-url" to "git ls-remote --get-url" to "git config remote.<name>.url" based on command output and version tests.
Test Plan: Ran `arc land --hold`, rigged the subcommands to fail to try all three fallbacks, ran `arc land --hold --remote asdfasdf` to get an explicit failure.
Maniphest Tasks: T13481
Differential Revision: https://secure.phabricator.com/D20950
Summary: Fixes T13458. Emit an explicit message when "arc close-revision --finalize" bails out because the revision is not "Accepted".
Test Plan: Ran `arc close-revision [--finalize] ...` on various revisions, saw more clear messaging.
Maniphest Tasks: T13458
Differential Revision: https://secure.phabricator.com/D20915
Summary:
Ref T13432. Git has a "diff.suppressBlankEmpty" config option which makes it emit nonstandard diffs with trimmed trailing whitespace on unchanged blank lines.
Currently, we don't parse these diffs correctly. Even if we do in the future, emitting a more standard diff is desirable.
Explicitly disable this option when executing "git diff" so we build more standard diffs.
Test Plan:
- Configured this option.
- Modified a file with a blank line in it without changing the blank line, got this goofy display diff:
{F6985234}
- Applied patch, rediffed the same change, saw "-c diff.suppressBlankEmpty" in "--trace" output and got this sensible diff:
{F6985235}
Maniphest Tasks: T13432
Differential Revision: https://secure.phabricator.com/D20877
Summary: Ref T13434. Since "git p4 submit" gets more complicated when submitting merges, and empty merges (as with "--no-ff") seem to vanish, and it's not clear this is desirable or useful anyway, just make the "merge" strategy an explicit error with Perforce remotes.
Test Plan: Ran "arc land --merge ..." in a Git/Perforce repository, got an explicit error. Ran "arc land --squash ...", got existing working behavior.
Maniphest Tasks: T13434
Differential Revision: https://secure.phabricator.com/D20871
Summary:
Fixes T10650. It's valid to `arc land --remote origin --onto master` without first fetching that ref. If we can't find a local ref for a specified remote branch, try to fetch it before giving up.
(In the long run, this should be valid even if the remote branch does not exist at all and the user intends to create it -- see T12876 -- but this is a step toward that.)
Test Plan:
- Ran `rm .git/refs/remotes/origin/master`, then landed into "master".
- Before: "arc land" bailed out immediately.
- After: "arc land" fetches the missing ref.
```
$ arc land
TARGET Landing onto "master", the default target under git.
REMOTE Using remote "origin", the default remote under Git.
TARGET No local ref exists for branch "master" in remote "origin", attempting fetch...
FETCHED Fetched branch "master" from remote "origin".
...
```
Maniphest Tasks: T10650
Differential Revision: https://secure.phabricator.com/D20870
Summary: Ref T13434. Move some git-engine-specific handling of "arc land" arguments into the Git engine. This prepares to handle Perforce workflows.
Test Plan: Will "arc land" this change.
Maniphest Tasks: T13434
Differential Revision: https://secure.phabricator.com/D20867
Summary:
Ref T13428. Historically, "implode()" accepts arguments in either order. PHP 7.4+ warns about glue not being first.
Add a lint check when the second parameter is a static scalar, implying it is the glue parameter.
Test Plan: Ran unit tests. See next change.
Maniphest Tasks: T13428
Differential Revision: https://secure.phabricator.com/D20857
Summary:
Fixes T10321. Some reasonable but less-common workflows involve running `arc land` from a detached HEAD state.
When users do this, we currently try to delete the raw hash as though it were a branch during cleanup. Instead, detect if the thing we're thinking about deleting is a branch or not, and just leave it alone if it isn't.
Test Plan:
- Ran `git checkout <some hash>`, then `arc land --revision <some revision>`.
- Before, everything worked but cleanup tried to `git branch -D <some hash>`.
- After, everything worked and cleanup skipped branch deletion.
Maniphest Tasks: T10321
Differential Revision: https://secure.phabricator.com/D20786
Summary:
The current code assumes git-svn is always working from a remote called
`trunk`, but if the repository is initialized without the `-T` option it
will instead be called `git-svn`, and if `--prefix` is used (which is
set by default to `origin/` in Git 2+) the remote name will have the
specified prefix as well.
Instead, look at the `fetch` target refspec set in the git-svn config.
Fixes T13293.
Test Plan:
`arc land` without errors (or manually creating a `trunk` branch) from a
checkout made with Git 2.18.0 (verified this manually on a non-`-T`
checkout as well).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13293
Differential Revision: https://secure.phabricator.com/D19681
Summary:
Fixes T6854. The current format for `lint-test` files is somewhat inflexible and does not allow us to make assertions regarding the code or name of the linter messages (of class `ArcanistLintMessage`) that are raised. Specifically, the `${severity}:${line}:${char}` format is hardcoded in `ArcanistLinterTestCase`. In this diff, I extend the this format to achieve the following goals:
- Allow for the lint message code and name to be specified. Specifically, the full format is `${severity}:${line}:${char}:${code}:${name}`.
- Make all fields optional. `error:3:` will match any and all errors occuring on line 3.
- Provide more useful output when assertions fail. Specifically, output //all// lint messages that are missing and/or surplus. Previously, only the first lint message was output.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6854
Differential Revision: https://secure.phabricator.com/D11176
Summary:
I am writing a proxy linter that can be used to wrap any `ArcanistExternalLinter` and execute all commands within a Docker container (see [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php |`ArcanistDockerContainerLinterProxy`]] from [[https://github.com/freelancer/flarc | `flarc`]]). In order for `ArcanistDockerContainerLinterProxy` to behave like the `ArcanistExternalLinter` that is being proxied, `final` needs to be removed from some methods.
I figured this was reasonable to submit upstream as a similar change ({D19630}) was previously accepted.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19730
Summary:
I am writing a proxy linter that can be used to wrap any `ArcanistExternalLinter` and execute all commands within a Docker container (see [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php |`ArcanistDockerContainerLinterProxy`]] from [[https://github.com/freelancer/flarc | `flarc`]]). In order for `ArcanistDockerContainerLinterProxy` to behave like the `ArcanistExternalLinter` that is being proxied, `final` needs to be removed from some methods.
I figured this was reasonable to submit upstream as a similar change ({D19630}) was previously accepted.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20514
Summary: The recently-added, build-plan-behavior-aware check here does all of its own prompting, so we should skip the other prompting if it doesn't throw.
Test Plan: Will `arc land` something sketchy sooner or later.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20494
Summary:
See PHI1238. This "," in the regex can only make the lint binding fail if there is no "," in the version output string. In modern versions of Pylint, there is (apparently) no comma in the version string. Remove it.
See also <https://discourse.phabricator-community.org/t/arcanistpylintlinter-version-regex-issue/2688>
Test Plan:
```
$ pip install pylint
Traceback (most recent call last):
File "/Users/epriestley/Library/Python/2.7/bin/pip", line 6, in <module>
from pip._internal import main
ImportError: No module named pip._internal
```
¯\_(ツ)_/¯
Reviewers: amckinley, joshuaspence
Reviewed By: joshuaspence
Differential Revision: https://secure.phabricator.com/D20505
Summary:
Since I'm in here for PHI1083:
- Add some "--" so we get correct behavior when you have a file named "master", a branch named "README.txt", etc.
- Stop using "%C" unnecessarily.
- Fix some untranslatable strings.
Test Plan: Ran `arc patch` a couple of times, ran the variations of `git` commands to catch anything weird with "--" handling.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20254
Summary:
See PHI1083. Previously, see PHI648 and D19475.
When you apply a submodule patch in Git, it leaves you with a working copy that has the "submodule pointer" dirtied but the actual submodule untouched:
```
$ git status
On branch ...
Changes to be committed:
(use "git reset HEAD <file>..." to unstage)
modified: philter
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git checkout -- <file>..." to discard changes in working directory)
modified: philter (new commits)
```
So, if you're applying `D123` and `submodule/` was previously pointed at commit "A" but `D123` updates it to point at commit "B", you get this after `git apply ...`:
- Git index says "submodule/ = B".
- On disk, "submodule/ = A".
Now, if you `git add --all` or `git commit --all`, git picks up the "change" on disk as an intended modification of the submodule. This puts the submodule back to "A" and overwrites/undoes the "pointer" update that's trying to make it point to "B".
To avoid this, update submodules after applying the patch.
Also, every time we modify the working copy, just update submodules.
Test Plan:
- Add a submodule in branch "B1", pointed at commit "A".
- Branch to create branch "B2". Update the submodule to point at commit "B". Commit this and `arc diff` it.
- Go back to "B1". Use `arc patch D...` to apply the revision you just created.
- Before change:
- "arc patch" applies the submodule change, so "pointer = B", "disk = A".
- "arc patch" runs "git commit --all", which looks at disk and sets "pointer = A".
- This isn't a change, so we fail with an empty commit.
- After change:
- "arc patch" applies the submodule change, so "pointer = B", "disk = A".
- "arc patch" updates submodules, so "pointer = B", "disk = B".
- "arc patch" runs "git commit --all", which now has a change, and commits "submodule = B".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20253
Summary:
Ref T13258. This makes "arc land" respect the new "Warn When Landing" behavior.
This will only work if you have very up-to-date APIs. Just fall back to the older code if the new API calls fail.
Test Plan: Ran `arc land` on a revision with builds in various states and with the different "Warn When Landing" behaviors. Saw appropriate warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20236
Summary:
See PHI1104. The older "differential.querydiffs" method includes the entire raw diff text for all the diffs associated with a revision in its response, but we: only care about the most recent diff; and don't care about the text at all.
For reasonably large changes with several updates, this can be significantly slow.
We can get this same information more efficiently from the modern "differential.diff.search", since D19386 (April 2018). The only trick is that we need a "revisionPHID", which we don't have on hand.
For now, just fetch the revision PHID. In the future, we can likely make adjustments so that we have the revision PHID already by the time we get here.
This may slow down the normal case very slightly (since we now do two calls instead of one), but it speeds up the bad cases dramatically.
Test Plan:
Ran `arc diff` to update a change in a local repository. `var_dump()`'d the old and new algorithm results, saw the same outcome.
Used `arc diff --trace` on an update to a change to verify that `differential.diff.search` is called but `differential.querydiffs` is not.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20221
Summary:
Ref T13161. See D20181. This allows the intraline highlighter to accept new ">" and "<" spans and apply a different style for them.
The input pattern is `list<segment>`. Each segment is `pair<wild kind, int byte_length>`, i.e. wrap the next `byte_length` bytes in a span of kind `kind`.
Before this change, the possible kinds of segements are `0` (no intraline diff, do not highlight) or `1` (intraline diff, highlight in bright color).
D20181 adds `<` (depth decreased) and `>` (depth increased). These are like `1`, but add a different class so the UI can handle them differently.
Test Plan: See D20181.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20182
Summary:
Ref T13249. See PHI810. We currently warn you when //all// reviewers are away, but not when only some reviewers are away.
This makes some amount of sense under the "anyone can accept anything" rules we sort of recommend, but a lot of installs realistically have tons of owner/package rules now.
Instead, if any reviewers are away, show the user exactly who is away and until when, then make sure they don't want to make any adjustments.
(We can do a better job of this after the toolsets change when we can use the new APIs, but this is an easy fix for now.)
Test Plan: Created a revision with multiple reviewers, either some or all of whom were away. Got appropriate output and prompt behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20172
Summary: Ref T13249. We currently allow `if (function_exists('X')) { X(); }` but not `if (defined('X')) { X; }`. Allow the latter.
Test Plan: See D20145, which linted clean with this patch in place.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20146
Summary: See 30 prior patches. This is a fatal in PHP7, let's just hunt these down.
Test Plan: Ran unit tests. See next diff for results.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19931
Summary:
See PHI904. Ref T13210. Ref T13209. Currently, we have an `hg cat` construction which attempts to pass a literal `%p` to Mercurial. This fails because you can't pass `%` through `%s` outside of `wilds`.
It also uses `%C` to pass a list of file paths. This is broadly unsafe and can cause command execution if you modify a file named, e.g., `; rm -rf xyz` or similar. I think it would be difficult to turn this into an attack but it's fairly bad. This dates from D5144 in 2013.
Test Plan: With this patch, created D19757 which has valid binary data (see F5962134).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210, T13209
Differential Revision: https://secure.phabricator.com/D19758
Summary:
Ref T13098. See PHI858. If you write this at the end of a message in `arc diff`:
```
Subscribers:
#projectname
# NEW DIFFERENTIAL REVISION
# Describe the changes in this new revision.
# ...
```
...we'll currently eat the `#projectname` as an instructional comment, even if it is followed by an empty line.
Instead, stop eating stuff once we hit the first empty line. (We escape empty lines in comments already.)
After T13098 I'll maybe adjust this to use a more explicit instruction escape, like `##`, since there's no reason we're bound to `#`.
Test Plan: Added a unit test and made it pass.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19639
