1. 13 Jul, 2016 16 commits
    • Rémy Coutable's avatar
    • Rémy Coutable's avatar
      Merge branch '18193-developers-can-merge' into 'master' · 9ca633eb
      Rémy Coutable authored
      Allow developers to merge into a protected branch without having push access
      
      ## What does this MR do?
      
      Adds a "Developers can merge" checkbox to protected branches much like the "Developers can push" checkbox. When the checkbox is enabled, a developer can merge MRs into that protected branch from the Web UI and from the command-line (any push that is entirely composed of merge commits is allowed).
      
      ## Are there points in the code the reviewer needs to double check?
      
      - This MR refactors the `GitAccess` module, moving parts of it to `UserAccess` and the new `ChangeAccessCheck`.
      - This MR refactors `GitAccessSpec`, which generates a "matrix" of tests.
      - The main logic "developers can merge" should be straightforward enough.
      - The commits are fairly atomic, and the commit messages are descriptive regarding the motivations behind every change.
      
      ## Why was this MR needed?
      
      A significant portion of this feature was implemented in !4220 (thanks, @mvestergaard!) ; I'm wrapping it up.
      
      ## What are the relevant issue numbers?
      
      #18193 
      Closes #967 
      
      ## Screenshots
      
      ![1](/uploads/c636e88ba38628211754e7cf122b0dc4/1.png)
      ![2](/uploads/5ed1e7917e2f36853a479faa565b022a/2.png)
      ![3](/uploads/0d202ba42e8dc6aade7bc6ac8db41ee6/3.png)
      
      ## TODO
      
      - [ ]  #18193 !4892 Add "developers can merge" as an option for protected branches
          - [x]  Review existing code
          - [x]  Fix build
          - [x]  Implementation / refactoring
              - [x]  Clean up `GitAccess`
              - [x]  Clean up `protected_branches.js.coffee`
              - [x]  Figure out authorization issue
                  - If we try to merge code into a protected branch for a user who doesn't have access to that branch, an auth check will fail
                  - We need to get around this, somehow
                  - [x]  Try detecting merge commits and allowing those
              - [x]  A push with regular commits _and_ merge commits should fail
                  - [x]  Figure out a solution
                  - [x]  Extensive tests for `MergeCommitCheck`
          - [x]  Add tests
              - [x]  Untested parts of original MR
          - [x]  Improve the checks in `/allowed`
              -  @dzaporozhets's proposal:
                  - commits in push == commits in merge request
                  - branch to push == target branch of merge request
                  - merge request has required amount of approves (ee only)
                  - merge commit in push == merge commit we created when merged via UI
                  - save merge commit sha in database and compare with `newrev`
              - my proposal
                  - /allowed finds all open merge requests with the appropriate target branch
                  - For each MR, compare the commit SHAs in the MR to the commit SHAs in the change set
                  - If we have a match, compare the diff of the matching MR to the diff of the change set
                  - If we still have a match, the merge is legit
              - [x]  Wait for replies on my proposal
              - [x]  Pick a strategy
              - [x]  Implementation
                  - [x]  Save `in_progress_merge_commit_sha`
                  - [x]  Check `in_progress_merge_commit_sha`
                  - [x]  Clear `in_progress_merge_commit_sha`
              - [x]  Test / refactor
          - [x]  Merge conflicts
          - [x]  Verify workflows
              - [x]  Developer with 'developer can merge' on:
                  - [x]  Can merge an MR from the Web UI
                  - [x]  Error message for conflicts in the Web UI
                  - [x]  Cannot merge an MR from the command line (HTTP)
                  - [x]  Cannot merge an MR from the command line (SSH)
                  - [x]  Cannot modify the branch otherwise
              - [x]  Developer with 'developer can merge' off:
                  - [x]  Cannot merge an MR from the Web UI
                  - [x]  Error message for conflicts in the Web UI
                  - [x]  Cannot merge an MR from the command line (HTTP)
                  - [x]  Cannot merge an MR from the command line (SSH)
                  - [x]  Cannot modify the branch otherwise
              - [x]  New projects created could have have "Developers can merge" turned on automatically for the default branch
          - [x]  CHANGELOG
          - [x]  Fix build
          - [x]  Wait for [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/42624e3d53754064186d4ae9048e310d1d3eed0b/builds) to pass
          - [x]  Screenshots
          - [x]  Assign to endboss
          - [x]  Respond to @dbalexandre's comments
              - [x]  Duplicated line, this is equals to line 26.
              - [x]  We aren't using any of these helpers in this migration, we can remove the include.
              - [x]  What do you think to add a default value for this column to avoid the Three-state Boolean Problem?
              - [x]  group all checks under Gitlab::Checks
              - [x]  You have a default value for developers_can_merge column, but your migration doesn't add it.
              - [x]  What do you think to rename Partially protected to anything else?
          - [x]  Fix conflicts
          - [x]  Make sure [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/b1cfd42f20a78fd7f844288954e97cff32962e20/builds) passes
          - [ ]  Wait for merge
      
      See merge request !4892
      9ca633eb
    • Rémy Coutable's avatar
      Merge branch '19766-dont-crash-when-no-objects-to-cache' into 'master' · fb229bbf
      Rémy Coutable authored
      ObjectRenderer doesn't crash when no objects to cache with Rails.cache.read_multi
      
      ## What does this MR do?
      
      Avoid calls to Rails.cache.read_multi without cache keys so it doesn't raise an exception
      
      ## What are the relevant issue numbers?
      
      Closes #19766
      
      ## Does this MR meet the acceptance criteria?
      
      - [ ] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added. I considered is not needed is a fix over a RC
      - ~~[ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
      - ~~[ ] API support added~~
      - Tests
        - [x] Added for this feature/bug
        - [x] All builds are passing
      - [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
      - [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
      - [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
      
      See merge request !5229
      fb229bbf
    • Paco Guzman's avatar
    • Timothy Andrew's avatar
      Implement last round of review comments from !4892. · bb81f2af
      Timothy Andrew authored
      1. Fix typos, minor styling errors.
      
      2. Use single quotes rather than double quotes in `user_access_spec`.
      
      3. Test formatting.
      bb81f2af
    • Timothy Andrew's avatar
    • Timothy Andrew's avatar
      Appease rubocop. · 4d00ed21
      Timothy Andrew authored
      4d00ed21
    • Timothy Andrew's avatar
    • Timothy Andrew's avatar
      Don't ask the user to "merge this request manually". · af7e7516
      Timothy Andrew authored
      1. If they are a developer with "Developers can Merge" switched on.
      af7e7516
    • Timothy Andrew's avatar
      Clean up `protected_branches.js.coffee` · 959d63ab
      Timothy Andrew authored
      - Only send a param for the currently changed checkbox.
      - Have the controller use strong parameters correctly, so that the PATCH
        works as expected.
      959d63ab
    • Timothy Andrew's avatar
      Refactor `Gitlab::GitAccess` · 60245bbe
      Timothy Andrew authored
      1. Don't use case statements for dispatch anymore. This leads to a lot
         of duplication, and makes the logic harder to follow.
      
      2. Remove duplicated logic.
      
          - For example, the `can_push_to_branch?` exists, but we also have a
            different way of checking the same condition within `change_access_check`.
      
          - This kind of duplication is removed, and the `can_push_to_branch?`
            method is used in both places.
      
      3. Move checks returning true/false to `UserAccess`.
      
          - All public methods in `GitAccess` now return an instance of
            `GitAccessStatus`. Previously, some methods would return
            true/false as well, which was confusing.
      
          - It makes sense for these kinds of checks to be at the level of a
            user, so the `UserAccess` class was repurposed for this. The prior
            `UserAccess.allowed?` classmethod is converted into an instance
            method.
      
          - All external uses of these checks have been migrated to use the
            `UserAccess` class
      
      4. Move the "change_access_check" into a separate class.
      
          - Create the `GitAccess::ChangeAccessCheck` class to run these
            checks, which are quite substantial.
      
          - `ChangeAccessCheck` returns an instance of `GitAccessStatus` as
            well.
      
      5. Break out the boolean logic in `ChangeAccessCheck` into `if/else`
         chains - this seems more readable.
      
      6. I can understand that this might look like overkill for !4892, but I
         think this is a good opportunity to clean it up.
      
          - http://martinfowler.com/bliki/OpportunisticRefactoring.html
      60245bbe
    • Timothy Andrew's avatar
      Enforce "developers can merge" during `pre-receive`. · 495db096
      Timothy Andrew authored
      1. When a merge request is being merged, save the merge commit SHA in
         the `in_progress_merge_commit_sha` database column.
      
      2. The `pre-receive` hook looks for any locked (in progress) merge
         request with `in_progress_merge_commit_sha` matching the `newrev` it
         is passed.
      
      3. If it finds a matching MR, the merge is legitimate.
      
      4. Update `git_access_spec` to test the behaviour we added here. Also
         refactored this spec a bit to make it easier to add more contexts / conditions.
      495db096
    • Mathias Vestergaard's avatar
      Added "developers can merge" setting to protected branches · f0577d83
      Mathias Vestergaard authored
      - Cherry-picked from `mvestergaard:branch-protection-dev-merge`
      - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4220
      f0577d83
    • Dmitriy Zaporozhets's avatar
    • Douwe Maan's avatar
      Merge branch 'multi-line-inline-diff' into 'master' · 5fea640e
      Douwe Maan authored
      Render inline diffs for multiple changed lines following eachother
      
      Before:
      
      ![Screen_Shot_2016-07-11_at_00.08.27](/uploads/b14664211e0f5cef6e77a78eadfcbcdf/Screen_Shot_2016-07-11_at_00.08.27.png)
      
      After:
      
      ![Screen_Shot_2016-07-11_at_00.07.34](/uploads/567be631869a4867a2edf6ff7eda6369/Screen_Shot_2016-07-11_at_00.07.34.png)
      
      See merge request !5174
      5fea640e
    • Douwe Maan's avatar
      Rename constant to be more descriptive · 489e1937
      Douwe Maan authored
      489e1937
  2. 12 Jul, 2016 24 commits