1. 22 Jun, 2019 1 commit
  2. 20 Jun, 2019 2 commits
  3. 28 May, 2019 1 commit
  4. 23 May, 2019 1 commit
    • Asher Baker's avatar
      Fix arc land on odd/modern git-svn checkouts · 7329bc7c
      Asher Baker authored
      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
      7329bc7c
  5. 21 May, 2019 1 commit
    • Joshua Spence's avatar
      Modify the `lint-test` file format to allow for more powerful assertions · dd514e26
      Joshua Spence authored
      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
      dd514e26
  6. 16 May, 2019 1 commit
  7. 15 May, 2019 1 commit
    • Wenzheng Jiang's avatar
      Let lint rules support anonymous classes · 82445bb6
      Wenzheng Jiang authored
      Summary: Ref T4334. Depends on D19740. Improve some lint rules so they can handle anonymous classes.
      
      Test Plan: Ran updated tests
      
      Reviewers: joshuaspence, #blessed_reviewers, epriestley
      
      Reviewed By: joshuaspence, #blessed_reviewers, epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T4334
      
      Differential Revision: https://secure.phabricator.com/D19741
      82445bb6
  8. 14 May, 2019 5 commits
  9. 08 Mar, 2019 1 commit
  10. 07 Mar, 2019 2 commits
    • epriestley's avatar
      Make minor correctness changes to some "arc patch" command execution · 9830c931
      epriestley authored
      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
      9830c931
    • epriestley's avatar
      Fix a case where "arc patch" could skip submodule changes · 73804f00
      epriestley authored
      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
      73804f00
  11. 06 Mar, 2019 1 commit
    • epriestley's avatar
      Implement "Warn When Landing" behavior for Build Plans in Arcanist · f6b8480a
      epriestley authored
      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
      f6b8480a
  12. 28 Feb, 2019 1 commit
    • epriestley's avatar
      Improve performance of "arc diff" updates for changes with large diff text · 96fde137
      epriestley authored
      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
      96fde137
  13. 23 Feb, 2019 1 commit
  14. 19 Feb, 2019 1 commit
    • epriestley's avatar
      Add Arcanist support for highlighting indent change intraline diffs · 9581dd0f
      epriestley authored
      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
      9581dd0f
  15. 16 Feb, 2019 1 commit
  16. 15 Feb, 2019 1 commit
    • epriestley's avatar
      In "arc diff", warn when some reviewers are away even if not everyone is away · 07a208d8
      epriestley authored
      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
      07a208d8
  17. 12 Feb, 2019 1 commit
  18. 28 Dec, 2018 2 commits
  19. 15 Dec, 2018 1 commit
  20. 12 Dec, 2018 1 commit
  21. 12 Nov, 2018 1 commit
  22. 08 Nov, 2018 1 commit
  23. 27 Oct, 2018 1 commit
  24. 26 Oct, 2018 1 commit
    • epriestley's avatar
      Work around a Windows escaping issue and security conecern in "hg cat --output ..." · 83661809
      epriestley authored
      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
      83661809
  25. 15 Sep, 2018 1 commit
  26. 14 Sep, 2018 1 commit
    • epriestley's avatar
      Make the Arcanist comment remover less aggressive about stripping instructional comments · 2650e862
      epriestley authored
      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
      2650e862
  27. 08 Sep, 2018 1 commit
  28. 06 Sep, 2018 1 commit
    • Joshua Spence's avatar
      Allow `willLintPaths` and `didLintPaths` to be overridden · 30b7835c
      Joshua Spence authored
      Summary: I'm not sure if the upstream will be interested in this change, but we are writing a linter which works by running an external command on the entire repository. This can't be done with `ArcanistExternalLinter` at the moment, which meant that we ended up copy-pasting most of `ArcanistFutureLinter`. This would be a lot easier if we could override `willLintPaths` and `didLintPaths`, but these methods are currently marked as `final`. An alternative solution would be some sort of `ArcanistLinter::transformPath` method.
      
      Test Plan: N/A
      
      Reviewers: epriestley, #blessed_reviewers
      
      Reviewed By: epriestley, #blessed_reviewers
      
      Subscribers: faulconbridge, Korvin
      
      Differential Revision: https://secure.phabricator.com/D19630
      30b7835c
  29. 25 Aug, 2018 1 commit
  30. 24 Aug, 2018 1 commit
    • epriestley's avatar
      Make the ArcanistBundle algorithm do what "diff -u" does when hunks are arguably mergeable · e1e93271
      epriestley authored
      Summary:
      Ref T13187. See PHI838. If two hunks are separated by 7 lines of context, we can render them as either:
      
      ```lang=diff
      + Hunk A
        Context 1
        Context 2
        Context 3
        Context 4
        Context 5
        Context 6
        Context 7
      + Hunk B
      ```
      
      ...or:
      
      ```lang=diff
      + Hunk A
        Context 1
        Context 2
        Context 3
      @@ +1,2 -3,4 @@
        Context 5
        Context 6
        Context 7
      + Hunk B
      ```
      
      Since we get the same number of output lines either way and the first one is more human-readable, we picked that one.
      
      However, `diff -u` does the second one. Since human-readability is probably less important than compatibility, change the behavior to be more similar to `diff -u`.
      
      Test Plan: Added unit tests for the edge cases with default parameters (6 context lines, 7 context lines) and made them pass.
      
      Reviewers: amckinley
      
      Maniphest Tasks: T13187
      
      Differential Revision: https://secure.phabricator.com/D19603
      e1e93271
  31. 06 Aug, 2018 1 commit
  32. 03 Aug, 2018 1 commit
  33. 20 Jul, 2018 1 commit