Merge branch '329100-redirect-last-default-branch' into 'master'

Redirect to the new default branch from a deleted default branch

See merge request gitlab-org/gitlab!65469

app/controllers/projects/blob_controller.rb

@@ -23,6 +23,10 @@ class Projects::BlobController < Projects::ApplicationController
   # We need to assign the blob vars before `authorize_edit_tree!` so we can
   # validate access to a specific ref.
   before_action :assign_blob_vars
+
+  # Since BlobController doesn't use assign_ref_vars, we have to call this explicitly
+  before_action :rectify_renamed_default_branch!, only: [:show]
+
   before_action :authorize_edit_tree!, only: [:new, :create, :update, :destroy]
 
   before_action :commit, except: [:new, :create]
@@ -140,11 +144,15 @@ class Projects::BlobController < Projects::ApplicationController
   end
 
   def commit
-    @commit = @repository.commit(@ref)
+    @commit ||= @repository.commit(@ref)
 
     return render_404 unless @commit
   end
 
+  def redirect_renamed_default_branch?
+    action_name == 'show'
+  end
+
   def assign_blob_vars
     @id = params[:id]
     @ref, @path = extract_ref(@id)
@@ -152,6 +160,12 @@ class Projects::BlobController < Projects::ApplicationController
     render_404
   end
 
+  def rectify_renamed_default_branch!
+    @commit ||= @repository.commit(@ref)
+
+    super
+  end
+
   # rubocop: disable CodeReuse/ActiveRecord
   def after_edit_path
     from_merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).find_by(iid: params[:from_merge_request_iid])

app/controllers/projects/tree_controller.rb

@@ -39,6 +39,10 @@ class Projects::TreeController < Projects::ApplicationController
 
   private
 
+  def redirect_renamed_default_branch?
+    action_name == 'show'
+  end
+
   def assign_dir_vars
     @branch_name = params[:branch_name]
 

app/models/project.rb

@@ -416,6 +416,7 @@ class Project < ApplicationRecord
     prefix: :import, to: :import_state, allow_nil: true
   delegate :squash_always?, :squash_never?, :squash_enabled_by_default?, :squash_readonly?, to: :project_setting
   delegate :squash_option, to: :project_setting
+  delegate :previous_default_branch, :previous_default_branch=, to: :project_setting
   delegate :no_import?, to: :import_state, allow_nil: true
   delegate :name, to: :owner, allow_nil: true, prefix: true
   delegate :members, to: :team, prefix: true

app/services/projects/update_service.rb

@@ -66,6 +66,8 @@ module Projects
       previous_default_branch = project.default_branch
 
       if project.change_head(params[:default_branch])
+        params[:previous_default_branch] = previous_default_branch
+
         after_default_branch_change(previous_default_branch)
       else
         raise ValidationError, s_("UpdateProject|Could not set the default branch")

db/migrate/20210705124128_add_project_settings_previous_default_branch.rb

+# frozen_string_literal: true
+
+class AddProjectSettingsPreviousDefaultBranch < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  # rubocop:disable Migration/AddLimitToTextColumns
+  # limit is added in 20210707173645_add_project_settings_previous_default_branch_text_limit
+  def up
+    with_lock_retries do
+      add_column :project_settings, :previous_default_branch, :text
+    end
+  end
+  # rubocop:enable Migration/AddLimitToTextColumns
+
+  def down
+    with_lock_retries do
+      remove_column :project_settings, :previous_default_branch
+    end
+  end
+end

db/migrate/20210707173645_add_project_settings_previous_default_branch_text_limit.rb

+# frozen_string_literal: true
+
+class AddProjectSettingsPreviousDefaultBranchTextLimit < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit :project_settings, :previous_default_branch, 4096
+  end
+
+  def down
+    remove_text_limit :project_settings, :previous_default_branch
+  end
+end

db/schema_migrations/20210705124128

+02aea8fe759614bc3aa751e023aa508963f8183366f6d6f518bbccc2d85ec1a1
\ No newline at end of file

db/schema_migrations/20210707173645

+e440dac0e14df7309c84e72b98ed6373c712901dc66310a474979e0fce7dc59c
\ No newline at end of file

db/structure.sql

@@ -17084,6 +17084,8 @@ CREATE TABLE project_settings (
     prevent_merge_without_jira_issue boolean DEFAULT false NOT NULL,
     cve_id_request_enabled boolean DEFAULT true NOT NULL,
     mr_default_target_self boolean DEFAULT false NOT NULL,
+    previous_default_branch text,
+    CONSTRAINT check_3a03e7557a CHECK ((char_length(previous_default_branch) <= 4096)),
     CONSTRAINT check_bde223416c CHECK ((show_default_award_emojis IS NOT NULL))
 );
 

doc/user/project/repository/branches/default.md

@@ -152,6 +152,20 @@ renames a Git repository's (`example`) default branch.
 1. Update references to the old branch name in related code and scripts that reside outside
    your repository, such as helper utilities and integrations.
 
+## Default branch rename redirect
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/329100) in GitLab 14.1
+
+URLs for specific files or directories in a project embed the project's default
+branch name, and are often found in documentation or browser bookmarks. When you
+[update the default branch name in your repository](#update-the-default-branch-name-in-your-repository),
+these URLs change, and must be updated.
+
+To ease the transition period, whenever the default branch for a project is
+changed, GitLab records the name of the old default branch. If that branch is
+deleted, attempts to view a file or directory on it are redirected to the
+current default branch, instead of displaying the "not found" page.
+
 ## Resources
 
 - [Discussion of default branch renaming](https://lore.kernel.org/git/pull.656.v4.git.1593009996.gitgitgadget@gmail.com/)

lib/extracts_path.rb

@@ -26,17 +26,17 @@ module ExtractsPath
   # Automatically renders `not_found!` if a valid tree path could not be
   # resolved (e.g., when a user inserts an invalid path or ref).
   #
+  # Automatically redirects to the current default branch if the ref matches a
+  # previous default branch that has subsequently been deleted.
+  #
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   override :assign_ref_vars
   def assign_ref_vars
     super
 
-    if @path.empty? && !@commit && @id.ends_with?('.atom')
-      @id = @ref = extract_ref_without_atom(@id)
-      @commit = @repo.commit(@ref)
+    rectify_atom!
 
-      request.format = :atom if @commit
-    end
+    rectify_renamed_default_branch! && return
 
     raise InvalidPathError unless @commit
 
@@ -59,6 +59,42 @@ module ExtractsPath
 
   private
 
+  # Override in controllers to determine which actions are subject to the redirect
+  def redirect_renamed_default_branch?
+    false
+  end
+
+  # rubocop:disable Gitlab/ModuleWithInstanceVariables
+  def rectify_atom!
+    return if @commit
+    return unless @id.ends_with?('.atom')
+    return unless @path.empty?
+
+    @id = @ref = extract_ref_without_atom(@id)
+    @commit = @repo.commit(@ref)
+
+    request.format = :atom if @commit
+  end
+  # rubocop:enable Gitlab/ModuleWithInstanceVariables
+
+  # For GET/HEAD requests, if the ref doesn't exist in the repository, check
+  # whether we're trying to access a renamed default branch. If we are, we can
+  # redirect to the current default branch instead of rendering a 404.
+  # rubocop:disable Gitlab/ModuleWithInstanceVariables
+  def rectify_renamed_default_branch!
+    return unless redirect_renamed_default_branch?
+    return if @commit
+    return unless @id && @ref && repository_container.respond_to?(:previous_default_branch)
+    return unless repository_container.previous_default_branch == @ref
+    return unless request.get? || request.head?
+
+    flash[:notice] = _('The default branch for this project has been changed. Please update your bookmarks.')
+    redirect_to url_for(id: @id.sub(/\A#{Regexp.escape(@ref)}/, repository_container.default_branch))
+
+    true
+  end
+  # rubocop:enable Gitlab/ModuleWithInstanceVariables
+
   override :repository_container
   def repository_container
     @project

locale/gitlab.pot

@@ -32557,6 +32557,9 @@ msgstr ""
 msgid "The default CI/CD configuration file and path for new projects."
 msgstr ""
 
+msgid "The default branch for this project has been changed. Please update your bookmarks."
+msgstr ""
+
 msgid "The dependency list details information about the components used within your project."
 msgstr ""
 

spec/controllers/projects/blob_controller_spec.rb

@@ -5,7 +5,8 @@ require 'spec_helper'
 RSpec.describe Projects::BlobController do
   include ProjectForksHelper
 
-  let(:project) { create(:project, :public, :repository) }
+  let(:project) { create(:project, :public, :repository, previous_default_branch: previous_default_branch) }
+  let(:previous_default_branch) { nil }
 
   describe "GET show" do
     def request
@@ -42,6 +43,20 @@ RSpec.describe Projects::BlobController do
         it { is_expected.to respond_with(:not_found) }
       end
 
+      context "renamed default branch, valid file" do
+        let(:id) { 'old-default-branch/README.md' }
+        let(:previous_default_branch) { 'old-default-branch' }
+
+        it { is_expected.to redirect_to("/#{project.full_path}/-/blob/#{project.default_branch}/README.md") }
+      end
+
+      context "renamed default branch, invalid file" do
+        let(:id) { 'old-default-branch/invalid-path.rb' }
+        let(:previous_default_branch) { 'old-default-branch' }
+
+        it { is_expected.to redirect_to("/#{project.full_path}/-/blob/#{project.default_branch}/invalid-path.rb") }
+      end
+
       context "binary file" do
         let(:id) { 'binary-encoding/encoding/binary-1.bin' }
 

spec/controllers/projects/tree_controller_spec.rb

@@ -3,8 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Projects::TreeController do
-  let(:project) { create(:project, :repository) }
-  let(:user)    { create(:user) }
+  let(:project) { create(:project, :repository, previous_default_branch: previous_default_branch) }
+  let(:previous_default_branch) { nil }
+  let(:user) { create(:user) }
 
   before do
     sign_in(user)
@@ -55,6 +56,20 @@ RSpec.describe Projects::TreeController do
       it { is_expected.to respond_with(:not_found) }
     end
 
+    context "renamed default branch, valid file" do
+      let(:id) { 'old-default-branch/encoding/' }
+      let(:previous_default_branch) { 'old-default-branch' }
+
+      it { is_expected.to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/encoding/") }
+    end
+
+    context "renamed default branch, invalid file" do
+      let(:id) { 'old-default-branch/invalid-path/' }
+      let(:previous_default_branch) { 'old-default-branch' }
+
+      it { is_expected.to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/invalid-path/") }
+    end
+
     context "valid empty branch, invalid path" do
       let(:id) { 'empty-branch/invalid-path/' }
 

spec/lib/extracts_path_spec.rb

@@ -7,10 +7,17 @@ RSpec.describe ExtractsPath do
   include RepoHelpers
   include Gitlab::Routing
 
+  # Make url_for work
+  def default_url_options
+    { controller: 'projects/blob', action: 'show', namespace_id: @project.namespace.path, project_id: @project.path }
+  end
+
   let_it_be(:owner) { create(:user) }
   let_it_be(:container) { create(:project, :repository, creator: owner) }
 
   let(:request) { double('request') }
+  let(:flash) { {} }
+  let(:redirect_renamed_default_branch?) { true }
 
   before do
     @project = container
@@ -18,11 +25,14 @@ RSpec.describe ExtractsPath do
 
     allow(container.repository).to receive(:ref_names).and_return(ref_names)
     allow(request).to receive(:format=)
+    allow(request).to receive(:get?)
+    allow(request).to receive(:head?)
   end
 
   describe '#assign_ref_vars' do
     let(:ref) { sample_commit[:id] }
-    let(:params) { { path: sample_commit[:line_code_path], ref: ref } }
+    let(:path) { sample_commit[:line_code_path] }
+    let(:params) { { path: path, ref: ref } }
 
     it_behaves_like 'assigns ref vars'
 
@@ -126,6 +136,66 @@ RSpec.describe ExtractsPath do
         expect(@commit).to be_nil
       end
     end
+
+    context 'ref points to a previous default branch' do
+      let(:ref) { 'develop' }
+
+      before do
+        @project.update!(previous_default_branch: ref)
+
+        allow(@project).to receive(:default_branch).and_return('foo')
+      end
+
+      it 'redirects to the new default branch for a GET request' do
+        allow(request).to receive(:get?).and_return(true)
+
+        expect(self).to receive(:redirect_to).with("http://localhost/#{@project.full_path}/-/blob/foo/#{path}")
+        expect(self).not_to receive(:render_404)
+
+        assign_ref_vars
+
+        expect(@commit).to be_nil
+        expect(flash[:notice]).to match(/default branch/)
+      end
+
+      it 'redirects to the new default branch for a HEAD request' do
+        allow(request).to receive(:head?).and_return(true)
+
+        expect(self).to receive(:redirect_to).with("http://localhost/#{@project.full_path}/-/blob/foo/#{path}")
+        expect(self).not_to receive(:render_404)
+
+        assign_ref_vars
+
+        expect(@commit).to be_nil
+        expect(flash[:notice]).to match(/default branch/)
+      end
+
+      it 'returns 404 for any other request type' do
+        expect(self).not_to receive(:redirect_to)
+        expect(self).to receive(:render_404)
+
+        assign_ref_vars
+
+        expect(@commit).to be_nil
+        expect(flash).to be_empty
+      end
+
+      context 'redirect behaviour is disabled' do
+        let(:redirect_renamed_default_branch?) { false }
+
+        it 'returns 404 for a GET request' do
+          allow(request).to receive(:get?).and_return(true)
+
+          expect(self).not_to receive(:redirect_to)
+          expect(self).to receive(:render_404)
+
+          assign_ref_vars
+
+          expect(@commit).to be_nil
+          expect(flash).to be_empty
+        end
+      end
+    end
   end
 
   it_behaves_like 'extracts refs'

spec/requests/api/project_attributes.yml

@@ -137,6 +137,7 @@ project_setting:
     - has_confluence
     - has_vulnerabilities
     - prevent_merge_without_jira_issue
+    - previous_default_branch
     - project_id
     - push_rule_id
     - show_default_award_emojis

spec/services/projects/update_service_spec.rb

@@ -200,17 +200,32 @@ RSpec.describe Projects::UpdateService do
     context 'when updating a default branch' do
       let(:project) { create(:project, :repository) }
 
-      it 'changes a default branch' do
+      it 'changes default branch, tracking the previous branch' do
+        previous_default_branch = project.default_branch
+
         update_project(project, admin, default_branch: 'feature')
 
-        expect(Project.find(project.id).default_branch).to eq 'feature'
+        project.reload
+
+        expect(project.default_branch).to eq('feature')
+        expect(project.previous_default_branch).to eq(previous_default_branch)
+
+        update_project(project, admin, default_branch: previous_default_branch)
+
+        project.reload
+
+        expect(project.default_branch).to eq(previous_default_branch)
+        expect(project.previous_default_branch).to eq('feature')
       end
 
       it 'does not change a default branch' do
         # The branch 'unexisted-branch' does not exist.
         update_project(project, admin, default_branch: 'unexisted-branch')
 
-        expect(Project.find(project.id).default_branch).to eq 'master'
+        project.reload
+
+        expect(project.default_branch).to eq 'master'
+        expect(project.previous_default_branch).to be_nil
       end
     end